Bug 1855861 - Make stylesheet load events only fire if the sheet has an owner node. r=smaug

This matches the behavior of other browsers, and avoids having to keep
alive the link element and thus associated document etc for too long.

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

dom/base/Document.cpp

@@ -8710,15 +8710,11 @@ already_AddRefed<nsSimpleContentList> Document::BlockedNodesByClassifier()
     const {
   RefPtr<nsSimpleContentList> list = new nsSimpleContentList(nullptr);
 
-  const nsTArray<nsWeakPtr> blockedNodes = mBlockedNodesByClassifier.Clone();
-
-  for (unsigned long i = 0; i < blockedNodes.Length(); i++) {
-    nsWeakPtr weakNode = blockedNodes[i];
-    nsCOMPtr<nsIContent> node = do_QueryReferent(weakNode);
+  for (const nsWeakPtr& weakNode : mBlockedNodesByClassifier) {
+    if (nsCOMPtr<nsIContent> node = do_QueryReferent(weakNode)) {
       // Consider only nodes to which we have managed to get strong references.
       // Coping with nullptrs since it's expected for nodes to disappear when
       // nobody else is referring to them.
-    if (node) {
       list->AppendElement(node);
     }
   }

dom/base/Document.h

@@ -1344,6 +1344,16 @@ class Document : public nsINode,
   Maybe<ClientState> GetClientState() const;
   Maybe<ServiceWorkerDescriptor> GetController() const;
 
+  //  Given a node, get a weak reference to it and append that reference to
+  //  mBlockedNodesByClassifier. Can be used later on to look up a node in it.
+  //  (e.g., by the UI)
+  // /
+  void AddBlockedNodeByClassifier(nsINode* aNode) {
+    if (aNode) {
+      mBlockedNodesByClassifier.AppendElement(do_GetWeakReference(aNode));
+    }
+  }
+
   // Returns the size of the mBlockedNodesByClassifier array.
   //
   // This array contains nodes that have been blocked to prevent user tracking,
@@ -1359,17 +1369,14 @@ class Document : public nsINode,
   // Weak references to blocked nodes are added in the mBlockedNodesByClassifier
   // array but they are not removed when those nodes are removed from the tree
   // or even garbage collected.
-  long BlockedNodeByClassifierCount() const {
+  size_t BlockedNodeByClassifierCount() const {
     return mBlockedNodesByClassifier.Length();
   }
 
-  //
   // Returns strong references to mBlockedNodesByClassifier. (Document.h)
-  //
   // This array contains nodes that have been blocked to prevent
   // user tracking. They most likely have had their nsIChannel
   // canceled by the URL classifier (Safebrowsing).
-  //
   already_AddRefed<nsSimpleContentList> BlockedNodesByClassifier() const;
 
   // Helper method that returns true if the document has storage-access sandbox
@@ -3566,23 +3573,6 @@ class Document : public nsINode,
   inline ImageDocument* AsImageDocument();
   inline const ImageDocument* AsImageDocument() const;
 
-  /*
-   * Given a node, get a weak reference to it and append that reference to
-   * mBlockedNodesByClassifier. Can be used later on to look up a node in it.
-   * (e.g., by the UI)
-   */
-  void AddBlockedNodeByClassifier(nsINode* node) {
-    if (!node) {
-      return;
-    }
-
-    nsWeakPtr weakNode = do_GetWeakReference(node);
-
-    if (weakNode) {
-      mBlockedNodesByClassifier.AppendElement(weakNode);
-    }
-  }
-
   gfxUserFontSet* GetUserFontSet();
   void FlushUserFontSet();
   void MarkUserFontSetDirty();

layout/style/Loader.cpp

@@ -291,6 +291,7 @@ SheetLoadData::SheetLoadData(
       mIsLoading(false),
       mIsCancelled(false),
       mMustNotify(false),
+      mHadOwnerNode(!!aOwningNode),
       mWasAlternate(aIsAlternate == IsAlternate::Yes),
       mMediaMatched(aMediaMatches == MediaMatched::Yes),
       mUseSystemPrincipal(false),
@@ -299,15 +300,13 @@ SheetLoadData::SheetLoadData(
       mBlockResourceTiming(false),
       mLoadFailed(false),
       mPreloadKind(aPreloadKind),
-      mOwningNodeBeforeLoadEvent(aOwningNode),
       mObserver(aObserver),
       mTriggeringPrincipal(aTriggeringPrincipal),
       mReferrerInfo(aReferrerInfo),
       mNonce(aNonce),
       mGuessedEncoding(GetFallbackEncoding(*aLoader, aOwningNode, nullptr)),
       mCompatMode(aLoader->CompatMode(aPreloadKind)) {
-  MOZ_ASSERT(!mOwningNodeBeforeLoadEvent ||
-                 dom::LinkStyle::FromNode(*mOwningNodeBeforeLoadEvent),
+  MOZ_ASSERT(!aOwningNode || dom::LinkStyle::FromNode(*aOwningNode),
              "Must implement LinkStyle");
   MOZ_ASSERT(mTriggeringPrincipal);
   MOZ_ASSERT(mLoader, "Must have a loader!");
@@ -331,6 +330,7 @@ SheetLoadData::SheetLoadData(css::Loader* aLoader, nsIURI* aURI,
       mIsLoading(false),
       mIsCancelled(false),
       mMustNotify(false),
+      mHadOwnerNode(false),
       mWasAlternate(false),
       mMediaMatched(true),
       mUseSystemPrincipal(aParentData && aParentData->mUseSystemPrincipal),
@@ -371,6 +371,7 @@ SheetLoadData::SheetLoadData(
       mIsLoading(false),
       mIsCancelled(false),
       mMustNotify(false),
+      mHadOwnerNode(false),
       mWasAlternate(false),
       mMediaMatched(true),
       mUseSystemPrincipal(aUseSystemPrincipal == UseSystemPrincipal::Yes),
@@ -422,7 +423,7 @@ void SheetLoadData::StartPendingLoad() {
 
 already_AddRefed<AsyncEventDispatcher>
 SheetLoadData::PrepareLoadEventIfNeeded() {
-  nsCOMPtr<nsINode> node = std::move(mOwningNodeBeforeLoadEvent);
+  nsCOMPtr<nsINode> node = mSheet->GetOwnerNode();
   if (!node) {
     return nullptr;
   }
@@ -660,9 +661,8 @@ nsresult SheetLoadData::VerifySheetReadyToParse(nsresult aStatus,
             aStatus)) {
       if (Document* doc = mLoader->GetDocument()) {
         for (SheetLoadData* data = this; data; data = data->mNext) {
-          // mOwningNode may be null but AddBlockTrackingNode can cope
-          doc->AddBlockedNodeByClassifier(
-              nsIContent::FromNodeOrNull(data->mOwningNodeBeforeLoadEvent));
+          // owner node may be null but AddBlockTrackingNode can cope
+          doc->AddBlockedNodeByClassifier(data->mSheet->GetOwnerNode());
         }
       }
     }

layout/style/SharedStyleSheetCache.cpp

@@ -145,7 +145,7 @@ void SharedStyleSheetCache::LoadCompletedInternal(
           // insert them into the tree.
           return false;
         }
-        if (data->mOwningNodeBeforeLoadEvent != data->mSheet->GetOwnerNode()) {
+        if (data->mHadOwnerNode != !!data->mSheet->GetOwnerNode()) {
           // The sheet was already removed from the tree and is no longer the
           // current sheet of the owning node, we can bail.
           return false;

layout/style/SheetLoadData.h

@@ -13,8 +13,6 @@
 #include "mozilla/PreloaderBase.h"
 #include "mozilla/SharedSubResourceCache.h"
 #include "mozilla/NotNull.h"
-#include "mozilla/UniquePtr.h"
-#include "nsIThreadInternal.h"
 #include "nsProxyRelease.h"
 
 namespace mozilla {
@@ -153,14 +151,19 @@ class SheetLoadData final
   // sheets that have been cancelled and such.
   bool mIsCancelled : 1;
 
-  // mMustNotify is true if the load data is being loaded async and
-  // the original function call that started the load has returned.
-  // This applies only to observer notifications; load/error events
-  // are fired for any SheetLoadData that has a non-null
-  // mOwningNodeBeforeLoadEvent (though mMustNotify is used to avoid an event
-  // loop round-trip in that case).
+  // mMustNotify is true if the load data is being loaded async and the original
+  // function call that started the load has returned.
+  //
+  // This applies only to observer notifications; load/error events are fired
+  // for any SheetLoadData that has a non-null owner node (though mMustNotify is
+  // used to avoid an event loop round-trip in that case).
   bool mMustNotify : 1;
 
+  // Whether we had an owner node at the point of creation. This allows
+  // differentiating between "Link" header stylesheets and LinkStyle-owned
+  // stylesheets.
+  const bool mHadOwnerNode : 1;
+
   // mWasAlternate is true if the sheet was an alternate
   // (https://html.spec.whatwg.org/#rel-alternate) when the load data was
   // created.
@@ -202,14 +205,6 @@ class SheetLoadData final
   // which causes a false positive warning here.
   const StylePreloadKind mPreloadKind;
 
-  // This is the node that imported the sheet. Cleared after the load/error
-  // event fires, as we don't need it anymore. Needed to get the charset on it,
-  // and to fire load/error events. Must implement LinkStyle.
-  //
-  // This is only set for top-level sheets (e.g., for an @import-ed sheet it'll
-  // be null).
-  nsCOMPtr<nsINode> mOwningNodeBeforeLoadEvent;
-
   nsINode* GetRequestingNode() const;
 
   // The observer that wishes to be notified of load completion
@@ -232,6 +227,7 @@ class SheetLoadData final
 
   // Whether SheetComplete was called.
   bool mSheetCompleteCalled = false;
+
   // Whether we intentionally are not calling SheetComplete because nobody is
   // listening for the load.
   bool mIntentionallyDropped = false;

layout/style/StreamLoader.cpp

@@ -16,8 +16,6 @@
 #include "nsNetCID.h"
 #include "nsServiceManagerUtils.h"
 
-#include <limits>
-
 namespace mozilla::css {
 
 StreamLoader::StreamLoader(SheetLoadData& aSheetLoadData)

testing/web-platform/tests/css/cssom/HTMLLinkElement-load-event-002.html

@@ -0,0 +1,60 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Link element load event doesn't block the parser.</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<link rel="help" href="https://html.spec.whatwg.org/#link-type-stylesheet">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+  let NUM_LOADS = 0;
+
+  function sheetUrl(token) {
+    return "stylesheet-same-origin.css?" + token;
+  }
+
+  function createLink(token) {
+    let link = document.createElement("link");
+    link.rel = "stylesheet";
+    link.href = sheetUrl(token);
+    return link;
+  }
+
+  function waitForEnoughTimeToLoadSheet(token) {
+    return new Promise(resolve => {
+      let link = createLink(token);
+      link.onload = resolve;
+      document.head.appendChild(link);
+    });
+  }
+
+  promise_test(async function (t) {
+    let link = createLink("removed");
+    link.addEventListener("load", t.unreached_func("got unexpected load event"));
+    link.addEventListener("error", t.unreached_func("got unexpected error event"));
+    document.head.appendChild(link);
+    link.remove();
+
+    await waitForEnoughTimeToLoadSheet("removed-wait");
+  }, "Load event doesn't fire on removed link");
+
+  promise_test(async function (t) {
+    let link = createLink("changed-initial");
+    let sawLoad = false;
+    let load = new Promise(resolve => {
+      link.addEventListener("load", function(e) {
+        assert_false(sawLoad, "Should only see load event once");
+        sawLoad = true;
+        resolve();
+      });
+    });
+    link.addEventListener("error", t.unreached_func("got unexpected error event"));
+    document.head.appendChild(link);
+    link.href = sheetUrl("changed-change");
+
+    await waitForEnoughTimeToLoadSheet("changed-wait");
+    await load;
+
+    assert_true(sawLoad, "Should've seen the load event only once");
+  }, "Load event doesn't fire for removed sheet");
+</script>