Bug 1648736 - Don't mark a load as performed on a given document until it has actually finished. r=heycam

Consider the case where we have an expired entry in the cache, and we
load a new document.

We get an speculative load from the HTML parser. That's great, and we
see the entry is expired and actually fired the load.

But then, we actually get to the load that the <link> element performs,
and we see that we've already performed this load, so instead of peeking
the in-progress load, we go ahead and peek the expired "complete" cache
entry, which is not what we want.

By marking a load as performed only once it has finished, we avoid the
complete sheet cache, and glom onto the existing load instead, which is
the correct thing to do.

Differential Revision: https://phabricator.services.mozilla.com/D81318

layout/style/Loader.cpp

@@ -1266,7 +1266,6 @@ nsresult Loader::LoadSheet(SheetLoadData& aLoadData, SheetState aSheetState,
   }
 
   SheetLoadDataHashKey key(aLoadData);
-  mLoadsPerformed.PutEntry(key);
 
   auto preloadKey = PreloadHashKey::CreateAsStyle(aLoadData);
   bool coalescedLoad = false;
@@ -1528,6 +1527,7 @@ Loader::Completed Loader::ParseSheet(const nsACString& aBytes,
 void Loader::NotifyObservers(SheetLoadData& aData, nsresult aStatus) {
   RecordUseCountersIfNeeded(mDocument, aData.mUseCounters.get());
   if (aData.mURI) {
+    mLoadsPerformed.PutEntry(SheetLoadDataHashKey(aData));
     aData.NotifyStop(aStatus);
     // NOTE(emilio): This needs to happen before notifying observers, as
     // FontFaceSet for example checks for pending sheet loads from the

layout/style/StyleSheet.cpp

@@ -5,6 +5,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/StyleSheet.h"
+#include "mozilla/Assertions.h"
 #include "mozilla/BasePrincipal.h"
 #include "mozilla/ComputedStyleInlines.h"
 #include "mozilla/css/ErrorReporter.h"
@@ -895,7 +896,15 @@ void StyleSheet::UnparentChildren() {
              "by a document?");
   // XXXbz this is a little bogus; see the comment where we
   // declare mChildren in StyleSheetInfo.
-  for (StyleSheet* child : ChildSheets()) {
+  //
+  // FIXME(emilio): StyleSheetInfo::RemoveSheet fixes up the parent list
+  // instead... We should maybe remove this and make that fix up more correct
+  // (right now it only tries to fix them up if you're the first sheet, but
+  // there's no guarantee that the first stylesheet is where the children end up
+  // being inserted in presence of deferred loads).
+  for (StyleSheet* child : Inner().mChildren) {
+    MOZ_ASSERT(!child->GetParentSheet() ||
+               child->GetParentSheet()->mInner == mInner);
     if (child->mParentSheet == this) {
       child->mParentSheet = nullptr;
     }