Bug 1639262 - Avoid duplicate entries in the stylesheet cache. r=nordzilla,firefox-style-system-reviewers
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 20 May 2020 21:16:04 +0000
changeset 531360 cf1939b293d954d1539818daeb2a2553dcf1d15d
parent 531359 fd7802694d8f5255871f0da15e8057530ed36e41
child 531361 191820bda8f5b3fa245f458c031557f78fa95bda
push id37438
push userabutkovits@mozilla.com
push dateThu, 21 May 2020 09:36:57 +0000
treeherdermozilla-central@2d00a1a6495c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnordzilla, firefox-style-system-reviewers
bugs1639262
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1639262 - Avoid duplicate entries in the stylesheet cache. r=nordzilla,firefox-style-system-reviewers When the loader finds an already-complete stylesheet, it will call PostLoadEvent to fire the load event of the relevant <link> element, etc. For that, it'll mint a SheetLoadData, with various fields hard-coded. That causes us to eventually insert the sheet in mCompleteSheets, but with a different key, which means we'll end up with two copies of the same stylesheet in the cache, than when reporting memory we'll report twice. Fix it by constructing the right load data a bit sooner, and add an assertion to ensure this doesn't happen anymore. Differential Revision: https://phabricator.services.mozilla.com/D76000
layout/style/Loader.cpp
layout/style/Loader.h
layout/style/SheetLoadData.h
--- a/layout/style/Loader.cpp
+++ b/layout/style/Loader.cpp
@@ -386,18 +386,19 @@ SheetLoadData::SheetLoadData(
       mRequestingNode(aRequestingNode),
       mPreloadEncoding(aPreloadEncoding) {
   MOZ_ASSERT(mLoader, "Must have a loader!");
   MOZ_ASSERT(!mUseSystemPrincipal || mSyncLoad,
              "Shouldn't use system principal for async loads");
 }
 
 SheetLoadData::~SheetLoadData() {
-  MOZ_DIAGNOSTIC_ASSERT(mSheetCompleteCalled,
-                        "Should always call SheetComplete");
+  MOZ_DIAGNOSTIC_ASSERT(mSheetCompleteCalled || mIntentionallyDropped,
+                        "Should always call SheetComplete, except when "
+                        "dropping the load");
 
   // Do this iteratively to avoid blowing up the stack.
   RefPtr<SheetLoadData> next = std::move(mNext);
   while (next) {
     next = std::move(next->mNext);
   }
 }
 
@@ -1894,18 +1895,26 @@ void Loader::DoSheetComplete(SheetLoadDa
           // nsXULPrototypeCache::CollectMemoryReports() to stop using
           // SizeOfIncludingThis() because it will no longer own the sheets.
           cache->PutStyleSheet(CloneSheet(*sheet));
         }
       }
     } else {
 #endif
       SheetLoadDataHashKey key(aLoadData);
-      NS_ASSERTION(sheet->IsComplete(),
-                   "Should only be caching complete sheets");
+      MOZ_ASSERT(sheet->IsComplete(), "Should only be caching complete sheets");
+
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+      for (const auto& entry : mSheets->mCompleteSheets) {
+        MOZ_DIAGNOSTIC_ASSERT(
+            entry.GetData() != sheet || key.KeyEquals(entry.GetKey()),
+            "Same sheet, different keys?");
+      }
+#endif
+
       mSheets->mCompleteSheets.Put(&key, RefPtr{sheet});
 #ifdef MOZ_XUL
     }
 #endif
   }
 }
 
 void Loader::MarkLoadTreeFailed(SheetLoadData& aLoadData) {
@@ -2092,36 +2101,45 @@ Result<Loader::LoadSheetResult, nsresult
 
   auto matched = PrepareSheet(*sheet, aInfo.mTitle, aInfo.mMedia, nullptr,
                               isAlternate, aInfo.mIsExplicitlyEnabled);
 
   InsertSheetInTree(*sheet, aInfo.mContent);
 
   nsCOMPtr<nsIStyleSheetLinkingElement> owningElement(
       do_QueryInterface(aInfo.mContent));
+  // We may get here with no content for Link: headers for example.
+  MOZ_ASSERT(!!owningElement == !!aInfo.mContent,
+             "If there is any node, it should be an "
+             "nsIStyleSheetLinkingElement");
 
+  auto data = MakeRefPtr<SheetLoadData>(
+      this, aInfo.mTitle, aInfo.mURI, sheet, syncLoad, owningElement,
+      isAlternate, matched, IsPreload::No, aObserver, principal,
+      aInfo.mReferrerInfo, context);
   if (state == SheetState::Complete) {
     LOG(("  Sheet already complete: 0x%p", sheet.get()));
     if (aObserver || !mObservers.IsEmpty() || owningElement) {
-      rv = PostLoadEvent(aInfo.mURI, sheet, aObserver, isAlternate, matched,
-                         aInfo.mReferrerInfo, owningElement);
+      rv = PostLoadEvent(std::move(data));
       if (NS_FAILED(rv)) {
         return Err(rv);
       }
+    } else {
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+      // We don't have to notify anyone of this load, as it was complete, so
+      // drop it intentionally.
+      data->mIntentionallyDropped = true;
+#endif
     }
 
     // The load hasn't been completed yet, will be done in PostLoadEvent.
     return LoadSheetResult{Completed::No, isAlternate, matched};
   }
 
   // Now we need to actually load it.
-  auto data = MakeRefPtr<SheetLoadData>(
-      this, aInfo.mTitle, aInfo.mURI, sheet, syncLoad, owningElement,
-      isAlternate, matched, IsPreload::No, aObserver, principal,
-      aInfo.mReferrerInfo, context);
 
   auto result = LoadSheetResult{Completed::No, isAlternate, matched};
 
   MOZ_ASSERT(result.ShouldBlock() == !data->ShouldDefer(),
              "These should better match!");
 
   // If we have to parse and it's a non-blocking non-inline sheet, defer it.
   if (!syncLoad && state == SheetState::NeedsParser &&
@@ -2344,88 +2362,78 @@ Result<RefPtr<StyleSheet>, nsresult> Loa
   bool syncLoad = !aObserver;
   auto [sheet, state] =
       CreateSheet(aURL, nullptr, aOriginPrincipal, aParsingMode, aCORSMode,
                   aReferrerInfo, aIntegrity, syncLoad);
 
   PrepareSheet(*sheet, EmptyString(), EmptyString(), nullptr, IsAlternate::No,
                IsExplicitlyEnabled::No);
 
+  auto data = MakeRefPtr<SheetLoadData>(
+      this, aURL, sheet, syncLoad, aUseSystemPrincipal, aIsPreload,
+      aPreloadEncoding, aObserver, aOriginPrincipal, aReferrerInfo, mDocument);
   if (state == SheetState::Complete) {
     LOG(("  Sheet already complete"));
     if (aObserver || !mObservers.IsEmpty()) {
-      rv = PostLoadEvent(aURL, sheet, aObserver, IsAlternate::No,
-                         MediaMatched::Yes, aReferrerInfo, nullptr);
+      rv = PostLoadEvent(std::move(data));
       if (NS_FAILED(rv)) {
         return Err(rv);
       }
+    } else {
+      // We don't have to notify anyone of this load, as it was complete, so
+      // drop it intentionally.
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+      data->mIntentionallyDropped = true;
+#endif
     }
     return sheet;
   }
 
-  auto data = MakeRefPtr<SheetLoadData>(
-      this, aURL, sheet, syncLoad, aUseSystemPrincipal, aIsPreload,
-      aPreloadEncoding, aObserver, aOriginPrincipal, aReferrerInfo, mDocument);
   rv = LoadSheet(*data, state);
   if (NS_FAILED(rv)) {
     return Err(rv);
   }
   if (aObserver) {
     data->mMustNotify = true;
   }
   return sheet;
 }
 
-nsresult Loader::PostLoadEvent(nsIURI* aURI, StyleSheet* aSheet,
-                               nsICSSLoaderObserver* aObserver,
-                               IsAlternate aWasAlternate,
-                               MediaMatched aMediaMatched,
-                               nsIReferrerInfo* aReferrerInfo,
-                               nsIStyleSheetLinkingElement* aElement) {
+nsresult Loader::PostLoadEvent(RefPtr<SheetLoadData> aLoadData) {
   LOG(("css::Loader::PostLoadEvent"));
-  MOZ_ASSERT(aSheet, "Must have sheet");
-  MOZ_ASSERT(aObserver || !mObservers.IsEmpty() || aElement,
-             "Must have observer or element");
-
-  RefPtr<SheetLoadData> evt = new SheetLoadData(
-      this,
-      EmptyString(),  // title doesn't matter here
-      aURI, aSheet, false, aElement, aWasAlternate, aMediaMatched,
-      IsPreload::No, aObserver, nullptr, aReferrerInfo, mDocument);
-
-  mPostedEvents.AppendElement(evt);
+  mPostedEvents.AppendElement(aLoadData);
 
   nsresult rv;
-  RefPtr<SheetLoadData> runnable(evt);
+  RefPtr<SheetLoadData> runnable(aLoadData);
   if (mDocument) {
     rv = mDocument->Dispatch(TaskCategory::Other, runnable.forget());
   } else if (mDocGroup) {
     rv = mDocGroup->Dispatch(TaskCategory::Other, runnable.forget());
   } else {
     rv = SchedulerGroup::Dispatch(TaskCategory::Other, runnable.forget());
   }
 
   if (NS_FAILED(rv)) {
     NS_WARNING("failed to dispatch stylesheet load event");
-    mPostedEvents.RemoveElement(evt);
+    mPostedEvents.RemoveElement(aLoadData);
   } else {
     // We'll unblock onload when we handle the event.
     BlockOnload();
 
     // We want to notify the observer for this data.
-    evt->mMustNotify = true;
-    evt->mSheetAlreadyComplete = true;
+    aLoadData->mMustNotify = true;
+    aLoadData->mSheetAlreadyComplete = true;
 
     // If we get to this code, aSheet loaded correctly at some point, so
     // we can just schedule a load event and don't need to touch the
     // data's mLoadFailed.  Note that we do this here and not from
     // inside our SheetComplete so that we don't end up running the load
     // event async.
-    MOZ_ASSERT(!evt->mLoadFailed, "Why are we marked as failed?");
-    evt->ScheduleLoadEventIfNeeded();
+    MOZ_ASSERT(!aLoadData->mLoadFailed, "Why are we marked as failed?");
+    aLoadData->ScheduleLoadEventIfNeeded();
   }
 
   return rv;
 }
 
 void Loader::HandleLoadEvent(SheetLoadData& aEvent) {
   // XXXbz can't assert this yet.... May not have an observer because
   // we're unblocking the parser
--- a/layout/style/Loader.h
+++ b/layout/style/Loader.h
@@ -377,25 +377,18 @@ class Loader final {
       UseSystemPrincipal, nsIPrincipal* aOriginPrincipal,
       const Encoding* aPreloadEncoding, nsIReferrerInfo* aReferrerInfo,
       nsICSSLoaderObserver* aObserver, CORSMode aCORSMode,
       const nsAString& aIntegrity);
 
   // Post a load event for aObserver to be notified about aSheet.  The
   // notification will be sent with status NS_OK unless the load event is
   // canceled at some point (in which case it will be sent with
-  // NS_BINDING_ABORTED).  aWasAlternate indicates the state when the load was
-  // initiated, not the state at some later time.  aURI should be the URI the
-  // sheet was loaded from (may be null for inline sheets).  aElement is the
-  // owning element for this sheet.
-  nsresult PostLoadEvent(nsIURI* aURI, StyleSheet* aSheet,
-                         nsICSSLoaderObserver* aObserver,
-                         IsAlternate aWasAlternate, MediaMatched aMediaMatched,
-                         nsIReferrerInfo* aReferrerInfo,
-                         nsIStyleSheetLinkingElement* aElement);
+  // NS_BINDING_ABORTED).
+  nsresult PostLoadEvent(RefPtr<SheetLoadData>);
 
   // Start the loads of all the sheets in mPendingDatas
   void StartDeferredLoads();
 
   void HandleLoadEvent(SheetLoadData&);
 
   // Note: LoadSheet is responsible for setting the sheet to complete on
   // failure.
--- a/layout/style/SheetLoadData.h
+++ b/layout/style/SheetLoadData.h
@@ -204,16 +204,19 @@ class SheetLoadData final : public nsIRu
 
   // The encoding to use for preloading Must be empty if mOwningElement
   // is non-null.
   const Encoding* const mPreloadEncoding;
 
 #ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
   // Whether SheetComplete was called.
   bool mSheetCompleteCalled = false;
+  // Whether we intentionally are not calling SheetComplete because nobody is
+  // listening for the load.
+  bool mIntentionallyDropped = false;
 #endif
 
   bool ShouldDefer() const { return mWasAlternate || !mMediaMatched; }
 
   // If there are no child sheets outstanding, mark us as complete.
   // Otherwise, the children are holding strong refs to the data
   // and will call SheetComplete() on it when they complete.
   void SheetFinishedParsingAsync() {