Bug 1645122 - Properly block onload when coalescing loads with other documents. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 23 Jun 2020 08:27:54 +0000
changeset 600836 d07d66ecd6b9ca685444912826f25e9751ef13b5
parent 600835 ce1a126dcf105e348ab891cf285a7ca73b295dcb
child 600837 1adf40ab95b9e04460a4b4e644a1490ed8a86f46
push id13310
push userffxbld-merge
push dateMon, 29 Jun 2020 14:50:06 +0000
treeherdermozilla-beta@15a59a0afa5c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1645122
milestone79.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 1645122 - Properly block onload when coalescing loads with other documents. r=heycam If two loading documents hit the sheet cache and we coalesce the resource load, there's nothing that prevents the load event on the second document from firing right now, and there should be. While at it, also fix the handling of the pending load count, though it has no correctness impact on the particular test we're fixing here... We were never decrementing it, which is of course wrong. However it kinda ended up working because it just causes us to not defer more loads. The new assertions and responsibility of the counter should ensure it stays correct. Differential Revision: https://phabricator.services.mozilla.com/D80583
layout/style/Loader.cpp
layout/style/Loader.h
layout/style/SharedStyleSheetCache.cpp
layout/style/SharedStyleSheetCache.h
--- a/layout/style/Loader.cpp
+++ b/layout/style/Loader.cpp
@@ -1117,29 +1117,37 @@ void Loader::InsertChildSheet(StyleSheet
 /**
  * LoadSheet handles the actual load of a sheet.  If the load is
  * supposed to be synchronous it just opens a channel synchronously
  * using the given uri, wraps the resulting stream in a converter
  * stream and calls ParseSheet.  Otherwise it tries to look for an
  * existing load for this URI and piggyback on it.  Failing all that,
  * a new load is kicked off asynchronously.
  */
-nsresult Loader::LoadSheet(SheetLoadData& aLoadData, SheetState aSheetState) {
+nsresult Loader::LoadSheet(SheetLoadData& aLoadData, SheetState aSheetState,
+                           PendingLoad aPendingLoad) {
   LOG(("css::Loader::LoadSheet"));
   MOZ_ASSERT(aLoadData.mURI, "Need a URI to load");
   MOZ_ASSERT(aLoadData.mSheet, "Need a sheet to load into");
   MOZ_ASSERT(aSheetState != SheetState::Complete, "Why bother?");
   MOZ_ASSERT(!aLoadData.mUseSystemPrincipal || aLoadData.mSyncLoad,
              "Shouldn't use system principal for async loads");
 
   LOG_URI("  Load from: '%s'", aLoadData.mURI);
 
-  ++mOngoingLoadCount;
-  if (aLoadData.mParentData) {
-    ++aLoadData.mParentData->mPendingChildren;
+  // If we're firing a pending load, this load is already accounted for the
+  // first time it went through this function.
+  if (aPendingLoad == PendingLoad::No) {
+    IncrementOngoingLoadCount();
+
+    // We technically never defer non-top-level sheets, so this condition could
+    // be outside the branch, but conceptually it should be here.
+    if (aLoadData.mParentData) {
+      ++aLoadData.mParentData->mPendingChildren;
+    }
   }
 
   nsresult rv = NS_OK;
 
   if (!mDocument && !aLoadData.mIsNonDocumentSheet) {
     // No point starting the load; just release all the data and such.
     LOG_WARN(("  No document and not non-document sheet; pre-dropping load"));
     SheetComplete(aLoadData, NS_BINDING_ABORTED);
@@ -1485,38 +1493,39 @@ Loader::Completed Loader::ParseSheet(con
       SheetComplete(aLoadData, NS_OK);
       return Completed::Yes;
     }
     return Completed::No;
   }
 
   // This parse does not need to be synchronous. \o/
   //
-  // Note that we need to block onload because there may be no network requests
-  // pending.
-  BlockOnload();
+  // Note that load is already blocked from IncrementOngoingLoadCount(), and
+  // will be unblocked from SheetFinishedParsingAsync which will end up in
+  // NotifyObservers as needed.
   nsCOMPtr<nsISerialEventTarget> target = DispatchTarget();
   sheet->ParseSheet(*this, aBytes, aLoadData)
       ->Then(
           target, __func__,
           [loadData = RefPtr<SheetLoadData>(&aLoadData)](bool aDummy) {
             MOZ_ASSERT(NS_IsMainThread());
-            loadData->mLoader->UnblockOnload(/* aFireSync = */ false);
             loadData->SheetFinishedParsingAsync();
           },
           [] { MOZ_CRASH("rejected parse promise"); });
   return Completed::No;
 }
 
 void Loader::NotifyObservers(SheetLoadData& aData, nsresult aStatus) {
   RecordUseCountersIfNeeded(mDocument, aData.mUseCounters.get());
   if (aData.mURI) {
-    MOZ_DIAGNOSTIC_ASSERT(mOngoingLoadCount);
-    --mOngoingLoadCount;
     aData.NotifyStop(aStatus);
+    // NOTE(emilio): This needs to happen before notifying observers, as
+    // FontFaceSet for example checks for pending sheet loads from the
+    // StyleSheetLoaded callback.
+    DecrementOngoingLoadCount();
   }
 
   if (aData.mMustNotify) {
     if (aData.mObserver) {
       LOG(("  Notifying observer %p for data %p.  deferred: %d",
            aData.mObserver.get(), &aData, aData.ShouldDefer()));
       aData.mObserver->StyleSheetLoaded(aData.mSheet, aData.ShouldDefer(),
                                         aStatus);
@@ -2046,20 +2055,17 @@ nsresult Loader::PostLoadEvent(RefPtr<Sh
   } else {
     rv = SchedulerGroup::Dispatch(TaskCategory::Other, runnable.forget());
   }
 
   if (NS_FAILED(rv)) {
     NS_WARNING("failed to dispatch stylesheet load event");
     mPostedEvents.RemoveElement(aLoadData);
   } else {
-    ++mOngoingLoadCount;
-
-    // We'll unblock onload when we handle the event.
-    BlockOnload();
+    IncrementOngoingLoadCount();
 
     // We want to notify the observer for this data.
     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
@@ -2075,18 +2081,16 @@ nsresult Loader::PostLoadEvent(RefPtr<Sh
 void Loader::HandleLoadEvent(SheetLoadData& aEvent) {
   // XXXbz can't assert this yet.... May not have an observer because
   // we're unblocking the parser
   // NS_ASSERTION(aEvent->mObserver, "Must have observer");
   NS_ASSERTION(aEvent.mSheet, "Must have sheet");
 
   mPostedEvents.RemoveElement(&aEvent);
   SheetComplete(aEvent, NS_OK);
-
-  UnblockOnload(true);
 }
 
 void Loader::Stop() {
   if (mSheets) {
     mSheets->CancelLoadsForLoader(*this);
   }
 
   auto arr = std::move(mPostedEvents);
--- a/layout/style/Loader.h
+++ b/layout/style/Loader.h
@@ -444,16 +444,30 @@ class Loader final {
   bool ShouldBypassCache() const;
 
  private:
   friend class mozilla::SharedStyleSheetCache;
   friend class SheetLoadData;
   friend class StreamLoader;
 
   // Helpers to conditionally block onload if mDocument is non-null.
+  void IncrementOngoingLoadCount() {
+    if (!mOngoingLoadCount++) {
+      BlockOnload();
+    }
+  }
+
+  void DecrementOngoingLoadCount() {
+    MOZ_DIAGNOSTIC_ASSERT(mOngoingLoadCount);
+    MOZ_DIAGNOSTIC_ASSERT(mOngoingLoadCount > mPendingLoadCount);
+    if (!--mOngoingLoadCount) {
+      UnblockOnload(false);
+    }
+  }
+
   void BlockOnload();
   void UnblockOnload(bool aFireSync);
 
   // Helper to select the correct dispatch target for asynchronous events for
   // this loader.
   already_AddRefed<nsISerialEventTarget> DispatchTarget();
 
   nsresult CheckContentPolicy(nsIPrincipal* aLoadingPrincipal,
@@ -511,17 +525,18 @@ class Loader final {
 
   // 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.
-  nsresult LoadSheet(SheetLoadData&, SheetState);
+  enum class PendingLoad { No, Yes };
+  nsresult LoadSheet(SheetLoadData&, SheetState, PendingLoad = PendingLoad::No);
 
   enum class AllowAsyncParse {
     Yes,
     No,
   };
 
   // Parse the stylesheet in the load data.
   //
--- a/layout/style/SharedStyleSheetCache.cpp
+++ b/layout/style/SharedStyleSheetCache.cpp
@@ -157,16 +157,25 @@ SharedStyleSheetCache::CacheResult Share
     LOG(("  From pending: %p", data->mSheet.get()));
     AssertIncompleteSheetMatches(*data, aKey);
     return {CloneSheet(*data->mSheet), SheetState::Pending};
   }
 
   return {};
 }
 
+void SharedStyleSheetCache::WillStartPendingLoad(SheetLoadData& aData) {
+  SheetLoadData* curr = &aData;
+  do {
+    MOZ_DIAGNOSTIC_ASSERT(curr->mLoader->mPendingLoadCount,
+                          "Where did this pending load come from?");
+    --curr->mLoader->mPendingLoadCount;
+  } while ((curr = curr->mNext));
+}
+
 bool SharedStyleSheetCache::CoalesceLoad(const SheetLoadDataHashKey& aKey,
                                          SheetLoadData& aNewLoad,
                                          SheetState aExistingLoadState) {
   MOZ_ASSERT(SheetLoadDataHashKey(aNewLoad).KeyEquals(aKey));
   SheetLoadData* existingData = nullptr;
   if (aExistingLoadState == SheetState::Loading) {
     existingData = mLoadingDatas.Get(aKey);
     MOZ_ASSERT(existingData, "CreateSheet lied about the state");
@@ -180,16 +189,18 @@ bool SharedStyleSheetCache::CoalesceLoad
   }
 
   if (aExistingLoadState == SheetState::Pending && !aNewLoad.ShouldDefer()) {
     // Kick the load off; someone cares about it right away
     RefPtr<SheetLoadData> removedData;
     mPendingDatas.Remove(aKey, getter_AddRefs(removedData));
     MOZ_ASSERT(removedData == existingData, "Bad loading table");
 
+    WillStartPendingLoad(*removedData);
+
     // We insert to the front instead of the back, to keep the invariant that
     // the front sheet always is the one that triggers the load.
     aNewLoad.mNext = std::move(removedData);
     LOG(("  Forcing load of pending data"));
     return false;
   }
 
   LOG(("  Glomming on to existing load"));
@@ -405,16 +416,18 @@ void SharedStyleSheetCache::InsertIntoCo
 
     mCompleteSheets.Put(
         key, {aData.mExpirationTime, std::move(counters), std::move(sheet)});
   }
 }
 
 void SharedStyleSheetCache::StartDeferredLoadsForLoader(
     css::Loader& aLoader, StartLoads aStartLoads) {
+  using PendingLoad = css::Loader::PendingLoad;
+
   LoadDataArray arr;
   for (auto iter = mPendingDatas.Iter(); !iter.Done(); iter.Next()) {
     bool startIt = false;
     SheetLoadData* data = iter.Data();
     do {
       if (data->mLoader == &aLoader) {
         // Note that we don't want to affect what the selected style set is, so
         // use true for aHasAlternateRel.
@@ -427,17 +440,18 @@ void SharedStyleSheetCache::StartDeferre
       }
     } while ((data = data->mNext));
     if (startIt) {
       arr.AppendElement(std::move(iter.Data()));
       iter.Remove();
     }
   }
   for (auto& data : arr) {
-    data->mLoader->LoadSheet(*data, SheetState::NeedsParser);
+    WillStartPendingLoad(*data);
+    data->mLoader->LoadSheet(*data, SheetState::NeedsParser, PendingLoad::Yes);
   }
 }
 
 void SharedStyleSheetCache::CancelDeferredLoadsForLoader(css::Loader& aLoader) {
   LoadDataArray arr;
 
   for (auto iter = mPendingDatas.Iter(); !iter.Done(); iter.Next()) {
     RefPtr<SheetLoadData>& first = iter.Data();
--- a/layout/style/SharedStyleSheetCache.h
+++ b/layout/style/SharedStyleSheetCache.h
@@ -115,16 +115,18 @@ class SharedStyleSheetCache final : publ
   struct CompleteSheet {
     uint32_t mExpirationTime = 0;
     UniquePtr<StyleUseCounters> mUseCounters;
     RefPtr<StyleSheet> mSheet;
 
     bool Expired() const;
   };
 
+  void WillStartPendingLoad(css::SheetLoadData&);
+
   nsDataHashtable<SheetLoadDataHashKey, CompleteSheet> mCompleteSheets;
   nsRefPtrHashtable<SheetLoadDataHashKey, css::SheetLoadData> mPendingDatas;
   // The SheetLoadData pointers in mLoadingDatas below are weak references.
   //
   // Note that we hold on to all sheet loads, even if in the end they happen not
   // to be cacheable.
   nsDataHashtable<SheetLoadDataHashKey, css::SheetLoadData*> mLoadingDatas;