Bug 1673885 - Don't expose incomplete sheets in LinkStyle.sheet / Document.styleSheets. r=nordzilla

This matches other browsers, and common sense to some extent.

The code is a bit awkward because I want this behind a pref for now, as
it's not precisely a zero-risk change.

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

dom/base/LinkStyle.cpp

@@ -69,6 +69,14 @@ LinkStyle::LinkStyle()
 
 LinkStyle::~LinkStyle() { LinkStyle::SetStyleSheet(nullptr); }
 
+StyleSheet* LinkStyle::GetSheetForBindings() const {
+  if (!StaticPrefs::dom_expose_incomplete_stylesheets() && mStyleSheet &&
+      !mStyleSheet->IsComplete()) {
+    return nullptr;
+  }
+  return mStyleSheet;
+}
+
 void LinkStyle::GetTitleAndMediaForElement(const Element& aSelf,
                                            nsString& aTitle, nsString& aMedia) {
   // Only honor title as stylesheet name for elements in the document (that is,
@@ -202,14 +210,18 @@ Result<LinkStyle::Update, nsresult> LinkStyle::DoUpdateStyleSheet(
                "there should not be a old document and old "
                "ShadowRoot simultaneously.");
 
-    // We're removing the link element from the document or shadow tree,
-    // unload the stylesheet.  We want to do this even if updates are
-    // disabled, since otherwise a sheet with a stale linking element pointer
-    // will be hanging around -- not good!
-    if (aOldShadowRoot) {
-      aOldShadowRoot->RemoveStyleSheet(*mStyleSheet);
-    } else {
-      aOldDocument->RemoveStyleSheet(*mStyleSheet);
+    // We're removing the link element from the document or shadow tree, unload
+    // the stylesheet.
+    //
+    // We want to do this even if updates are disabled, since otherwise a sheet
+    // with a stale linking element pointer will be hanging around -- not good!
+    if (mStyleSheet->IsComplete() ||
+        StaticPrefs::dom_expose_incomplete_stylesheets()) {
+      if (aOldShadowRoot) {
+        aOldShadowRoot->RemoveStyleSheet(*mStyleSheet);
+      } else {
+        aOldDocument->RemoveStyleSheet(*mStyleSheet);
+      }
     }
 
     SetStyleSheet(nullptr);
@@ -240,17 +252,20 @@ Result<LinkStyle::Update, nsresult> LinkStyle::DoUpdateStyleSheet(
   }
 
   if (mStyleSheet) {
-    if (thisContent.IsInShadowTree()) {
-      ShadowRoot* containingShadow = thisContent.GetContainingShadow();
-      // Could be null only during unlink.
-      if (MOZ_LIKELY(containingShadow)) {
-        containingShadow->RemoveStyleSheet(*mStyleSheet);
+    if (mStyleSheet->IsComplete() ||
+        StaticPrefs::dom_expose_incomplete_stylesheets()) {
+      if (thisContent.IsInShadowTree()) {
+        ShadowRoot* containingShadow = thisContent.GetContainingShadow();
+        // Could be null only during unlink.
+        if (MOZ_LIKELY(containingShadow)) {
+          containingShadow->RemoveStyleSheet(*mStyleSheet);
+        }
+      } else {
+        doc->RemoveStyleSheet(*mStyleSheet);
       }
-    } else {
-      doc->RemoveStyleSheet(*mStyleSheet);
     }
 
-    LinkStyle::SetStyleSheet(nullptr);
+    SetStyleSheet(nullptr);
   }
 
   if (!info) {

dom/base/LinkStyle.h

@@ -220,6 +220,9 @@ class LinkStyle {
 
   StyleSheet* GetSheet() const { return mStyleSheet; }
 
+  /** JS can only observe the sheet once fully loaded */
+  StyleSheet* GetSheetForBindings() const;
+
  protected:
   LinkStyle();
   virtual ~LinkStyle();

dom/webidl/LinkStyle.webidl

@@ -8,6 +8,6 @@
  */
 
 interface mixin LinkStyle {
-      readonly attribute StyleSheet? sheet;
+  [BinaryName="sheetForBindings"] readonly attribute StyleSheet? sheet;
 };
 

dom/xml/ProcessingInstruction.cpp

@@ -58,9 +58,9 @@ ProcessingInstruction::ProcessingInstruction(
 
 ProcessingInstruction::~ProcessingInstruction() = default;
 
-StyleSheet* ProcessingInstruction::GetSheet() const {
+StyleSheet* ProcessingInstruction::GetSheetForBindings() const {
   if (const auto* linkStyle = LinkStyle::FromNode(*this)) {
-    return linkStyle->GetSheet();
+    return linkStyle->GetSheetForBindings();
   }
   return nullptr;
 }

dom/xml/ProcessingInstruction.h

@@ -38,7 +38,7 @@ class ProcessingInstruction : public CharacterData {
   void GetTarget(nsAString& aTarget) { aTarget = NodeName(); }
   // This is the WebIDL API for LinkStyle, even though only
   // XMLStylesheetProcessingInstruction actually implements LinkStyle.
-  StyleSheet* GetSheet() const;
+  StyleSheet* GetSheetForBindings() const;
 
   NS_IMPL_FROMNODE_HELPER(ProcessingInstruction, IsProcessingInstruction())
 

layout/style/Loader.cpp

@@ -276,6 +276,7 @@ SheetLoadData::SheetLoadData(Loader* aLoader, const nsAString& aTitle,
       mPendingChildren(0),
       mSyncLoad(aSyncLoad),
       mIsNonDocumentSheet(false),
+      mIsChildSheet(aSheet->GetParentSheet()),
       mIsLoading(false),
       mIsBeingParsed(false),
       mIsCancelled(false),
@@ -317,6 +318,7 @@ SheetLoadData::SheetLoadData(Loader* aLoader, nsIURI* aURI, StyleSheet* aSheet,
       mPendingChildren(0),
       mSyncLoad(aParentData && aParentData->mSyncLoad),
       mIsNonDocumentSheet(aParentData && aParentData->mIsNonDocumentSheet),
+      mIsChildSheet(aSheet->GetParentSheet()),
       mIsLoading(false),
       mIsBeingParsed(false),
       mIsCancelled(false),
@@ -341,6 +343,7 @@ SheetLoadData::SheetLoadData(Loader* aLoader, nsIURI* aURI, StyleSheet* aSheet,
   MOZ_ASSERT(mTriggeringPrincipal);
   MOZ_ASSERT(!mUseSystemPrincipal || mSyncLoad,
              "Shouldn't use system principal for async loads");
+  MOZ_ASSERT_IF(aParentData, mIsChildSheet);
 }
 
 SheetLoadData::SheetLoadData(
@@ -358,6 +361,7 @@ SheetLoadData::SheetLoadData(
       mPendingChildren(0),
       mSyncLoad(aSyncLoad),
       mIsNonDocumentSheet(true),
+      mIsChildSheet(false),
       mIsLoading(false),
       mIsBeingParsed(false),
       mIsCancelled(false),
@@ -382,6 +386,7 @@ SheetLoadData::SheetLoadData(
   MOZ_ASSERT(mLoader, "Must have a loader!");
   MOZ_ASSERT(!mUseSystemPrincipal || mSyncLoad,
              "Shouldn't use system principal for async loads");
+  MOZ_ASSERT(!aSheet->GetParentSheet(), "Shouldn't be used for child loads");
 }
 
 SheetLoadData::~SheetLoadData() {
@@ -438,7 +443,11 @@ void SheetLoadData::FireLoadEvent(nsIThreadInternal* aThread) {
   RefPtr<SheetLoadData> kungFuDeathGrip(this);
   aThread->RemoveObserver(this);
 
-  // Now fire the event
+  // Now fire the event.
+  //
+  // NOTE(emilio): A bit weird that we fire the event even if the node is no
+  // longer in the tree, or the sheet that just loaded / errored is not the
+  // current node.sheet, but...
   nsCOMPtr<nsINode> node = mOwningNode;
   MOZ_ASSERT(node, "How did that happen???");
 
@@ -1034,20 +1043,14 @@ Loader::MediaMatched Loader::PrepareSheet(
  * 3) Sheets with linking elements are ordered based on document order
  *    as determined by CompareDocumentPosition.
  */
-void Loader::InsertSheetInTree(StyleSheet& aSheet,
-                               nsIContent* aLinkingContent) {
+void Loader::InsertSheetInTree(StyleSheet& aSheet, nsINode* aOwningNode) {
   LOG(("css::Loader::InsertSheetInTree"));
   MOZ_ASSERT(mDocument, "Must have a document to insert into");
-  MOZ_ASSERT(!aLinkingContent || aLinkingContent->IsInUncomposedDoc() ||
-                 aLinkingContent->IsInShadowTree(),
+  MOZ_ASSERT(!aOwningNode || aOwningNode->IsInUncomposedDoc() ||
+                 aOwningNode->IsInShadowTree(),
              "Why would we insert it anywhere?");
-
-  if (auto* linkStyle = LinkStyle::FromNodeOrNull(aLinkingContent)) {
-    linkStyle->SetStyleSheet(&aSheet);
-  }
-
   ShadowRoot* shadow =
-      aLinkingContent ? aLinkingContent->GetContainingShadow() : nullptr;
+      aOwningNode ? aOwningNode->GetContainingShadow() : nullptr;
 
   auto& target = shadow ? static_cast<DocumentOrShadowRoot&>(*shadow)
                         : static_cast<DocumentOrShadowRoot&>(*mDocument);
@@ -1067,7 +1070,7 @@ void Loader::InsertSheetInTree(StyleSheet& aSheet,
   int32_t insertionPoint = sheetCount - 1;
   for (; insertionPoint >= 0; --insertionPoint) {
     nsINode* sheetOwner = target.SheetAt(insertionPoint)->GetOwnerNode();
-    if (sheetOwner && !aLinkingContent) {
+    if (sheetOwner && !aOwningNode) {
       // Keep moving; all sheets with a sheetOwner come after all
       // sheets without a linkingNode
       continue;
@@ -1079,11 +1082,11 @@ void Loader::InsertSheetInTree(StyleSheet& aSheet,
       break;
     }
 
-    MOZ_ASSERT(aLinkingContent != sheetOwner,
+    MOZ_ASSERT(aOwningNode != sheetOwner,
                "Why do we still have our old sheet?");
 
     // Have to compare
-    if (nsContentUtils::PositionIsBefore(sheetOwner, aLinkingContent)) {
+    if (nsContentUtils::PositionIsBefore(sheetOwner, aOwningNode)) {
       // The current sheet comes before us, and it better be the first
       // such, because now we break
       break;
@@ -1695,7 +1698,12 @@ Result<Loader::LoadSheetResult, nsresult> Loader::LoadInlineStyle(
   auto matched = PrepareSheet(*sheet, aInfo.mTitle, aInfo.mMedia, nullptr,
                               isAlternate, aInfo.mIsExplicitlyEnabled);
 
-  InsertSheetInTree(*sheet, aInfo.mContent);
+  if (auto* linkStyle = LinkStyle::FromNodeOrNull(aInfo.mContent)) {
+    linkStyle->SetStyleSheet(sheet);
+  }
+  if (StaticPrefs::dom_expose_incomplete_stylesheets() || sheet->IsComplete()) {
+    InsertSheetInTree(*sheet, aInfo.mContent);
+  }
 
   Completed completed;
   if (sheetFromCache) {
@@ -1793,7 +1801,12 @@ Result<Loader::LoadSheetResult, nsresult> Loader::LoadStyleLink(
   auto matched = PrepareSheet(*sheet, aInfo.mTitle, aInfo.mMedia, nullptr,
                               isAlternate, aInfo.mIsExplicitlyEnabled);
 
-  InsertSheetInTree(*sheet, aInfo.mContent);
+  if (auto* linkStyle = LinkStyle::FromNodeOrNull(aInfo.mContent)) {
+    linkStyle->SetStyleSheet(sheet);
+  }
+  if (StaticPrefs::dom_expose_incomplete_stylesheets() || sheet->IsComplete()) {
+    InsertSheetInTree(*sheet, aInfo.mContent);
+  }
 
   // We may get here with no content for Link: headers for example.
   MOZ_ASSERT(!aInfo.mContent || LinkStyle::FromNode(*aInfo.mContent),

layout/style/Loader.h

@@ -515,7 +515,7 @@ class Loader final {
                             IsAlternate, IsExplicitlyEnabled);
 
   // Inserts a style sheet in a document or a ShadowRoot.
-  void InsertSheetInTree(StyleSheet& aSheet, nsIContent* aLinkingContent);
+  void InsertSheetInTree(StyleSheet& aSheet, nsINode* aOwningNode);
   // Inserts a style sheet into a parent style sheet.
   void InsertChildSheet(StyleSheet& aSheet, StyleSheet& aParentSheet);
 

layout/style/SharedStyleSheetCache.cpp

@@ -337,6 +337,43 @@ void SharedStyleSheetCache::LoadCompletedInternal(
       MOZ_ASSERT(data->mSheet->IsConstructed() ||
                      !data->mSheet->HasForcedUniqueInner(),
                  "should not get a forced unique inner during parsing");
+      // Insert the sheet into the tree now the sheet has loaded, but only if
+      // the sheet is still relevant, and if this is a top-level sheet.
+      const bool needInsertIntoTree = [&] {
+        if (StaticPrefs::dom_expose_incomplete_stylesheets()) {
+          // No need to do that, it's already done. This is technically a bit
+          // racy, but having to reload if you hit an in-progress load while
+          // switching the pref from about:config is not a big deal.
+          return false;
+        }
+        if (!data->mLoader->GetDocument()) {
+          // Not a document load, nothing to do.
+          return false;
+        }
+        if (data->mIsPreload != css::Loader::IsPreload::No) {
+          // Preloads are not supposed to be observable.
+          return false;
+        }
+        if (data->mSheet->IsConstructed()) {
+          // Constructable sheets are not in the regular stylesheet tree.
+          return false;
+        }
+        if (data->mIsChildSheet) {
+          // A child sheet, those will get exposed from the parent, no need to
+          // insert them into the tree.
+          return false;
+        }
+        if (data->mOwningNode != 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;
+        }
+        return true;
+      }();
+
+      if (needInsertIntoTree) {
+        data->mLoader->InsertSheetInTree(*data->mSheet, data->mOwningNode);
+      }
       data->mSheet->SetComplete();
       data->ScheduleLoadEventIfNeeded();
     } else if (data->mSheet->IsApplicable()) {

layout/style/SheetLoadData.h

@@ -137,6 +137,10 @@ class SheetLoadData final : public PreloaderBase,
   // proceed even if we have no document.
   const bool mIsNonDocumentSheet : 1;
 
+  // Whether this stylesheet is for a child sheet load. This is necessary
+  // because the sheet could be detached mid-load by CSSOM.
+  const bool mIsChildSheet : 1;
+
   // mIsLoading is true from the moment we are placed in the loader's
   // "loading datas" table (right after the async channel is opened)
   // to the moment we are removed from said table (due to the load

layout/style/test/test_load_events_on_stylesheets.html

@@ -119,13 +119,9 @@ link.onerror = function() { --pendingEventCounter;
 }
 document.body.appendChild(link);
 
-// Make sure we have that last stylesheet
-is(document.styleSheets.length, 7, "Should have seven stylesheets here");
-
-// Make sure that the sixth stylesheet is all loaded
 // If we ever switch away from sync loading of already-complete sheets, this
 // test will need adjusting
-checkSheetComplete(document.styleSheets[6], 1);
+checkSheetComplete(link.sheet, 1);
 
 ++pendingEventCounter;
 link = document.createElement("link");

modules/libpref/init/StaticPrefList.yaml

@@ -2088,6 +2088,16 @@
   value: true
   mirror: always
 
+# Whether the LinkStyle.sheet and DocumentOrShadowRoot.styleSheets accessor
+# exposes in-progress-loading stylesheets.
+#
+# Historical behavior is `true`, but other browsers (and also a bit of common
+# sense) match `false`.
+- name: dom.expose-incomplete-stylesheets
+  type: bool
+  value: false
+  mirror: always
+
 # Whether the Large-Allocation header is enabled.
 - name: dom.largeAllocationHeader.enabled
   type: bool

testing/web-platform/meta/html/semantics/document-metadata/interactions-of-styling-and-scripting/conditionally-block-rendering-on-link-media-attr.html.ini

@@ -1,4 +0,0 @@
-[conditionally-block-rendering-on-link-media-attr.html]
-  [Only the style sheet loaded via a link element whose media attribute matches the environment should block following script execution]
-    expected: FAIL
-

testing/web-platform/tests/css/cssom/ttwf-cssom-doc-ext-load-count.html

@@ -39,21 +39,21 @@
       }, "stylesheet.css should be unloaded and styleSheets.length === 0");
 
       // Verify that the styleSheets list length is back to 1 after loading a new sheet
-      test(function() {
+      async_test(function(t) {
 
         // create a css file reference
         var fileReference = document.createElement("link");
         fileReference.setAttribute("rel", "stylesheet");
         fileReference.setAttribute("type", "text/css");
         fileReference.setAttribute("href", "stylesheet-1.css");
+        fileReference.onerror = t.step_func_done(function() {
+          // assert that there is 1 styleSheet in the styleSheets property
+          assert_equals(styleSheets.length, 1, "styleSheets.length is incorrect:");
+        });
 
         // load the css file reference into the head section
         var head = document.getElementsByTagName("HEAD")[0];
         head.appendChild(fileReference);
-
-        // assert that there is 1 styleSheet in the styleSheets property
-        assert_equals(styleSheets.length, 1, "styleSheets.length is incorrect:");
-
       }, "stylesheet-1.css should be loaded and styleSheets.length === 1");
 
     </script>