Bug 1600624 - Make the StyleSheet children setup simpler. r=heycam

Using an array is much better to reason about than a manually linked list, and
allows us to preserve @import order.

Added a test for a bug that we happened not to have, but that it's not covered
by existing WPT tests.

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

--HG--
extra : moz-landing-system : lando

layout/style/GeckoBindings.cpp

@@ -1676,14 +1676,15 @@ static already_AddRefed<StyleSheet> LoadImportSheet(
   nsCOMPtr<nsIURI> uri = aURL.GetURI();
   nsresult rv = uri ? NS_OK : NS_ERROR_FAILURE;
 
-  StyleSheet* previousFirstChild = aParent->GetFirstChild();
+  size_t previousSheetCount = aParent->ChildSheets().Length();
   if (NS_SUCCEEDED(rv)) {
+    // TODO(emilio): We should probably make LoadChildSheet return the
+    // stylesheet rather than the return code.
     rv = aLoader->LoadChildSheet(*aParent, aParentLoadData, uri, media,
                                  aReusableSheets);
   }
 
-  if (NS_FAILED(rv) || !aParent->GetFirstChild() ||
-      aParent->GetFirstChild() == previousFirstChild) {
+  if (NS_FAILED(rv) || previousSheetCount == aParent->ChildSheets().Length()) {
     // Servo and Gecko have different ideas of what a valid URL is, so we might
     // get in here with a URL string that NS_NewURI can't handle.  We may also
     // reach here via an import cycle.  For the import cycle case, we need some
@@ -1703,11 +1704,11 @@ static already_AddRefed<StyleSheet> LoadImportSheet(
         ReferrerInfo::CreateForExternalCSSResources(emptySheet);
     emptySheet->SetReferrerInfo(referrerInfo);
     emptySheet->SetComplete();
-    aParent->PrependStyleSheet(emptySheet);
+    aParent->AppendStyleSheet(*emptySheet);
     return emptySheet.forget();
   }
 
-  RefPtr<StyleSheet> sheet = static_cast<StyleSheet*>(aParent->GetFirstChild());
+  RefPtr<StyleSheet> sheet = aParent->ChildSheets().LastElement();
   return sheet.forget();
 }
 

layout/style/Loader.cpp

@@ -1282,7 +1282,7 @@ void Loader::InsertChildSheet(StyleSheet& aSheet, StyleSheet& aParentSheet) {
   // child sheets should always start out enabled, even if they got
   // cloned off of top-level sheets which were disabled
   aSheet.SetEnabled(true);
-  aParentSheet.PrependStyleSheet(&aSheet);
+  aParentSheet.AppendStyleSheet(aSheet);
 
   LOG(("  Inserting into parent sheet"));
 }

layout/style/ServoStyleSet.cpp

@@ -39,7 +39,9 @@
 #include "nsWindowSizes.h"
 #include "GeckoProfiler.h"
 
-using namespace mozilla::dom;
+namespace mozilla {
+
+using namespace dom;
 
 #ifdef DEBUG
 bool ServoStyleSet::IsCurrentThreadInServoTraversal() {
@@ -47,8 +49,6 @@ bool ServoStyleSet::IsCurrentThreadInServoTraversal() {
 }
 #endif
 
-namespace mozilla {
-
 constexpr const StyleOrigin ServoStyleSet::kOrigins[];
 
 ServoStyleSet* sInServoTraversal = nullptr;
@@ -90,8 +90,6 @@ class MOZ_RAII AutoPrepareTraversal {
   AutoSetInServoTraversal mSetInServoTraversal;
 };
 
-}  // namespace mozilla
-
 ServoStyleSet::ServoStyleSet(Document& aDocument) : mDocument(&aDocument) {
   PreferenceSheet::EnsureInitialized();
   PodArrayZero(mCachedAnonymousContentStyleIndexes);
@@ -993,10 +991,8 @@ bool ServoStyleSet::EnsureUniqueInnerOnCSSSheets() {
     }
 
     // Enqueue all the sheet's children.
-    AutoTArray<StyleSheet*, 3> children;
-    sheet->AppendAllChildSheets(children);
-    for (auto* sheet : children) {
-      queue.AppendElement(MakePair(sheet, owner));
+    for (StyleSheet* child : sheet->ChildSheets()) {
+      queue.AppendElement(MakePair(child, owner));
     }
   }
 
@@ -1277,3 +1273,5 @@ UACacheReporter::CollectReports(nsIHandleReportCallback* aHandleReport,
 
   return NS_OK;
 }
+
+}  // namespace mozilla

layout/style/StyleSheet.cpp

@@ -123,32 +123,17 @@ void StyleSheet::UnlinkInner() {
     return;
   }
 
-  // Have to be a bit careful with child sheets, because we want to
-  // drop their mNext pointers and null out their mParent and
-  // mDocument, but don't want to work with deleted objects.  And we
-  // don't want to do any addrefing in the process, just to make sure
-  // we don't confuse the cycle collector (though on the face of it,
-  // addref/release pairs during unlink should probably be ok).
-  RefPtr<StyleSheet> child;
-  child.swap(Inner().mFirstChild);
-  while (child) {
+  for (StyleSheet* child : ChildSheets()) {
     MOZ_ASSERT(child->mParent == this, "We have a unique inner!");
     child->mParent = nullptr;
     // We (and child) might still think we're owned by a document, because
     // unlink order is non-deterministic, so the document's unlink, which would
-    // tell us it does't own us anymore, may not have happened yet.  But if
+    // tell us it doesn't own us anymore, may not have happened yet.  But if
     // we're being unlinked, clearly we're not owned by a document anymore
     // conceptually!
     child->ClearAssociatedDocumentOrShadowRoot();
-
-    RefPtr<StyleSheet> next;
-    // Null out child->mNext, but don't let it die yet
-    next.swap(child->mNext);
-    // Switch to looking at the old value of child->mNext next iteration
-    child.swap(next);
-    // "next" is now our previous value of child; it'll get released
-    // as we loop around.
   }
+  Inner().mChildren.Clear();
 }
 
 void StyleSheet::TraverseInner(nsCycleCollectionTraversalCallback& cb) {
@@ -158,11 +143,9 @@ void StyleSheet::TraverseInner(nsCycleCollectionTraversalCallback& cb) {
     return;
   }
 
-  StyleSheet* childSheet = GetFirstChild();
-  while (childSheet) {
+  for (StyleSheet* child : ChildSheets()) {
     NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "child sheet");
-    cb.NoteXPCOMChild(childSheet);
-    childSheet = childSheet->mNext;
+    cb.NoteXPCOMChild(child);
   }
 }
 
@@ -290,8 +273,8 @@ StyleSheetInfo::StyleSheetInfo(StyleSheetInfo& aCopy, StyleSheet* aPrimarySheet)
       mCORSMode(aCopy.mCORSMode),
       mReferrerInfo(aCopy.mReferrerInfo),
       mIntegrity(aCopy.mIntegrity),
-      mFirstChild(),  // We don't rebuild the child because we're making a copy
-                      // without children.
+      // We don't rebuild the child because we're making a copy without
+      // children.
       mSourceMapURL(aCopy.mSourceMapURL),
       mSourceMapURLFromComment(aCopy.mSourceMapURLFromComment),
       mSourceURL(aCopy.mSourceURL),
@@ -333,9 +316,13 @@ void StyleSheetInfo::AddSheet(StyleSheet* aSheet) {
 }
 
 void StyleSheetInfo::RemoveSheet(StyleSheet* aSheet) {
-  if ((aSheet == mSheets.ElementAt(0)) && (mSheets.Length() > 1)) {
-    StyleSheet::ChildSheetListBuilder::ReparentChildList(mSheets[1],
-                                                         mFirstChild);
+  if (aSheet == mSheets[0] && mSheets.Length() > 1) {
+    StyleSheet* newParent = mSheets[1];
+    for (StyleSheet* child : mChildren) {
+      child->mParent = newParent;
+      child->SetAssociatedDocumentOrShadowRoot(newParent->mDocumentOrShadowRoot,
+                                               newParent->mAssociationMode);
+    }
   }
 
   if (1 == mSheets.Length()) {
@@ -347,21 +334,6 @@ void StyleSheetInfo::RemoveSheet(StyleSheet* aSheet) {
   mSheets.RemoveElement(aSheet);
 }
 
-void StyleSheet::ChildSheetListBuilder::SetParentLinks(StyleSheet* aSheet) {
-  aSheet->mParent = parent;
-  aSheet->SetAssociatedDocumentOrShadowRoot(parent->mDocumentOrShadowRoot,
-                                            parent->mAssociationMode);
-}
-
-void StyleSheet::ChildSheetListBuilder::ReparentChildList(
-    StyleSheet* aPrimarySheet, StyleSheet* aFirstChild) {
-  for (StyleSheet* child = aFirstChild; child; child = child->mNext) {
-    child->mParent = aPrimarySheet;
-    child->SetAssociatedDocumentOrShadowRoot(
-        aPrimarySheet->mDocumentOrShadowRoot, aPrimarySheet->mAssociationMode);
-  }
-}
-
 void StyleSheet::GetType(nsAString& aType) { aType.AssignLiteral("text/css"); }
 
 void StyleSheet::GetHref(nsAString& aHref, ErrorResult& aRv) {
@@ -463,12 +435,6 @@ void StyleSheet::EnsureUniqueInner() {
   NOTIFY(SheetCloned, (*this));
 }
 
-void StyleSheet::AppendAllChildSheets(nsTArray<StyleSheet*>& aArray) {
-  for (StyleSheet* child = GetFirstChild(); child; child = child->mNext) {
-    aArray.AppendElement(child);
-  }
-}
-
 // WebIDL CSSStyleSheet API
 
 dom::CSSRuleList* StyleSheet::GetCssRules(nsIPrincipal& aSubjectPrincipal,
@@ -669,35 +635,16 @@ void StyleSheet::RemoveFromParent() {
     return;
   }
 
-  // TODO(emilio): It seems we should actually use mozilla::LinkedList or such
-  // for this.
-  //
-  // Also this is still pretty bogus, see where we declare mFirstChild in
-  // StyleSheetInfo.
-  bool found = false;
-  for (auto* child = mParent->GetFirstChild(); child; child = child->mNext) {
-    if (child == this) {
-      // We can only get here if we're the first.
-      found = true;
-      mParent->Inner().mFirstChild = mNext;
-      break;
-    }
-    if (child->mNext == this) {
-      found = true;
-      child->mNext = mNext;
-      break;
-    }
-  }
-  MOZ_DIAGNOSTIC_ASSERT(found, "Should find the rule in the child list.");
+  MOZ_ASSERT(mParent->ChildSheets().Contains(this));
+  mParent->Inner().mChildren.RemoveElement(this);
   mParent = nullptr;
   ClearAssociatedDocumentOrShadowRoot();
-  mNext = nullptr;
 }
 
 void StyleSheet::UnparentChildren() {
   // XXXbz this is a little bogus; see the XXX comment where we
   // declare mFirstChild in StyleSheetInfo.
-  for (StyleSheet* child = GetFirstChild(); child; child = child->mNext) {
+  for (StyleSheet* child : ChildSheets()) {
     if (child->mParent == this) {
       child->mParent = nullptr;
       MOZ_ASSERT(child->mAssociationMode == NotOwnedByDocumentOrShadowRoot,
@@ -760,8 +707,6 @@ bool StyleSheet::AreRulesAvailable(nsIPrincipal& aSubjectPrincipal,
   return true;
 }
 
-StyleSheet* StyleSheet::GetFirstChild() const { return Inner().mFirstChild; }
-
 void StyleSheet::SetAssociatedDocumentOrShadowRoot(
     DocumentOrShadowRoot* aDocOrShadowRoot, AssociationMode aAssociationMode) {
   MOZ_ASSERT(aDocOrShadowRoot ||
@@ -774,7 +719,7 @@ void StyleSheet::SetAssociatedDocumentOrShadowRoot(
   // Now set the same document on all our child sheets....
   // XXXbz this is a little bogus; see the XXX comment where we
   // declare mFirstChild.
-  for (StyleSheet* child = GetFirstChild(); child; child = child->mNext) {
+  for (StyleSheet* child : ChildSheets()) {
     if (child->mParent == this) {
       child->SetAssociatedDocumentOrShadowRoot(aDocOrShadowRoot,
                                                aAssociationMode);
@@ -782,48 +727,40 @@ void StyleSheet::SetAssociatedDocumentOrShadowRoot(
   }
 }
 
-void StyleSheet::PrependStyleSheet(StyleSheet* aSheet) {
+void StyleSheet::AppendStyleSheet(StyleSheet& aSheet) {
   WillDirty();
-  PrependStyleSheetSilently(aSheet);
+  AppendStyleSheetSilently(aSheet);
 }
 
-void StyleSheet::PrependStyleSheetSilently(StyleSheet* aSheet) {
-  MOZ_ASSERT(aSheet);
+void StyleSheet::AppendStyleSheetSilently(StyleSheet& aSheet) {
   MOZ_ASSERT(!IsReadOnly());
 
-  aSheet->mNext = Inner().mFirstChild;
-  Inner().mFirstChild = aSheet;
+  Inner().mChildren.AppendElement(&aSheet);
 
   // This is not reference counted. Our parent tells us when
   // it's going away.
-  aSheet->mParent = this;
-  aSheet->SetAssociatedDocumentOrShadowRoot(mDocumentOrShadowRoot,
-                                            mAssociationMode);
+  aSheet.mParent = this;
+  aSheet.SetAssociatedDocumentOrShadowRoot(mDocumentOrShadowRoot,
+                                           mAssociationMode);
 }
 
 size_t StyleSheet::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const {
   size_t n = 0;
-  const StyleSheet* s = this;
-  while (s) {
-    n += aMallocSizeOf(s);
+  n += aMallocSizeOf(this);
 
-    // See the comment in CSSStyleSheet::SizeOfIncludingThis() for an
-    // explanation of this.
-    //
-    // FIXME(emilio): This comment is gone, someone should go find it.
-    if (s->Inner().mSheets.LastElement() == s) {
-      n += s->Inner().SizeOfIncludingThis(aMallocSizeOf);
-    }
-
-    // Measurement of the following members may be added later if DMD finds it
-    // is worthwhile:
-    // - s->mTitle
-    // - s->mMedia
-    // - s->mStyleSets
-    // - s->mRuleList
-
-    s = s->mNext;
+  // We want to measure the inner with only one of the children, and it makes
+  // sense for it to be the latest as it is the most likely to be reachable.
+  if (Inner().mSheets.LastElement() == this) {
+    n += Inner().SizeOfIncludingThis(aMallocSizeOf);
   }
+
+  // Measurement of the following members may be added later if DMD finds it
+  // is worthwhile:
+  // - mTitle
+  // - mMedia
+  // - mStyleSets
+  // - mRuleList
+
   return n;
 }
 
@@ -853,7 +790,7 @@ void StyleSheet::List(FILE* out, int32_t aIndent) const {
   str.Append('\n');
   fprintf_stderr(out, "%s", str.get());
 
-  for (const StyleSheet* child = GetFirstChild(); child; child = child->mNext) {
+  for (const StyleSheet* child : ChildSheets()) {
     child->List(out, aIndent + 1);
   }
 }
@@ -892,7 +829,7 @@ JSObject* StyleSheet::WrapObject(JSContext* aCx,
 void StyleSheet::BuildChildListAfterInnerClone() {
   MOZ_ASSERT(Inner().mSheets.Length() == 1, "Should've just cloned");
   MOZ_ASSERT(Inner().mSheets[0] == this);
-  MOZ_ASSERT(!Inner().mFirstChild);
+  MOZ_ASSERT(Inner().mChildren.IsEmpty());
 
   auto* contents = Inner().mContents.get();
   RefPtr<ServoCssRules> rules = Servo_StyleSheet_GetRules(contents).Consume();
@@ -910,7 +847,7 @@ void StyleSheet::BuildChildListAfterInnerClone() {
     }
     auto* sheet = const_cast<StyleSheet*>(Servo_ImportRule_GetSheet(import));
     MOZ_ASSERT(sheet);
-    PrependStyleSheetSilently(sheet);
+    AppendStyleSheetSilently(*sheet);
     index++;
   }
 }
@@ -1057,21 +994,18 @@ nsresult StyleSheet::ReparseSheet(const nsAString& aInput) {
 
   // cache child sheets to reuse
   css::LoaderReusableStyleSheets reusableSheets;
-  for (StyleSheet* child = GetFirstChild(); child; child = child->mNext) {
+  for (StyleSheet* child : ChildSheets()) {
     if (child->GetOriginalURI()) {
       reusableSheets.AddReusableSheet(child);
     }
   }
 
-  // clean up child sheets list
-  for (StyleSheet* child = GetFirstChild(); child;) {
-    StyleSheet* next = child->mNext;
+  // Clean up child sheets list.
+  for (StyleSheet* child : ChildSheets()) {
     child->mParent = nullptr;
     child->ClearAssociatedDocumentOrShadowRoot();
-    child->mNext = nullptr;
-    child = next;
   }
-  Inner().mFirstChild = nullptr;
+  Inner().mChildren.Clear();
 
   uint32_t lineNumber = 1;
   if (mOwningNode) {

layout/style/StyleSheet.h

@@ -220,9 +220,6 @@ class StyleSheet final : public nsICSSLoaderObserver, public nsWrapperCache {
 
   void EnsureUniqueInner();
 
-  // Append all of this sheet's child sheets to aArray.
-  void AppendAllChildSheets(nsTArray<StyleSheet*>& aArray);
-
   // style sheet owner info
   enum AssociationMode : uint8_t {
     // OwnedByDocumentOrShadowRoot means mDocumentOrShadowRoot owns us (possibly
@@ -264,16 +261,19 @@ class StyleSheet final : public nsICSSLoaderObserver, public nsWrapperCache {
   }
   dom::CSSImportRule* GetOwnerRule() const { return mOwnerRule; }
 
-  void PrependStyleSheet(StyleSheet* aSheet);
+  void AppendStyleSheet(StyleSheet&);
 
-  // Prepend a stylesheet to the child list without calling WillDirty.
-  void PrependStyleSheetSilently(StyleSheet* aSheet);
+  // Append a stylesheet to the child list without calling WillDirty.
+  void AppendStyleSheetSilently(StyleSheet&);
 
-  StyleSheet* GetFirstChild() const;
-  StyleSheet* GetMostRecentlyAddedChildSheet() const {
-    // New child sheet can only be prepended into the linked list of
-    // child sheets, so the most recently added one is always the first.
-    return GetFirstChild();
+  const nsTArray<RefPtr<StyleSheet>>& ChildSheets() const {
+#ifdef DEBUG
+    for (StyleSheet* child : Inner().mChildren) {
+      MOZ_ASSERT(child->GetParentSheet());
+      MOZ_ASSERT(child->GetParentSheet()->mInner == mInner);
+    }
+#endif
+    return Inner().mChildren;
   }
 
   // Principal() never returns a null pointer.
@@ -374,13 +374,6 @@ class StyleSheet final : public nsICSSLoaderObserver, public nsWrapperCache {
   // Find the ID of the owner inner window.
   uint64_t FindOwningWindowInnerID() const;
 
-  template <typename Func>
-  void EnumerateChildSheets(Func aCallback) {
-    for (StyleSheet* child = GetFirstChild(); child; child = child->mNext) {
-      aCallback(child);
-    }
-  }
-
   // Copy the contents of this style sheet into the shared memory buffer managed
   // by aBuilder.  Returns the pointer into the buffer that the sheet contents
   // were stored at.  (The returned pointer is to an Arc<Locked<Rules>> value.)
@@ -453,16 +446,6 @@ class StyleSheet final : public nsICSSLoaderObserver, public nsWrapperCache {
 
   void ApplicableStateChanged(bool aApplicable);
 
-  struct ChildSheetListBuilder {
-    RefPtr<StyleSheet>* sheetSlot;
-    StyleSheet* parent;
-
-    void SetParentLinks(StyleSheet* aSheet);
-
-    static void ReparentChildList(StyleSheet* aPrimarySheet,
-                                  StyleSheet* aFirstChild);
-  };
-
   void UnparentChildren();
 
   void LastRelease();
@@ -496,8 +479,6 @@ class StyleSheet final : public nsICSSLoaderObserver, public nsWrapperCache {
 
   RefPtr<dom::MediaList> mMedia;
 
-  RefPtr<StyleSheet> mNext;
-
   // mParsingMode controls access to nonstandard style constructs that
   // are not safe for use on the public Web but necessary in UA sheets
   // and/or useful in user sheets.

layout/style/StyleSheetInfo.h

@@ -53,12 +53,13 @@ struct StyleSheetInfo final {
   nsCOMPtr<nsIReferrerInfo> mReferrerInfo;
   dom::SRIMetadata mIntegrity;
 
-  // Pointer to start of linked list of child sheets. This is all fundamentally
-  // broken, because each of the child sheets has a unique parent... We can
-  // only hope (and currently this is the case) that any time page JS can get
-  // its hands on a child sheet that means we've already ensured unique infos
-  // throughout its parent chain and things are good.
-  RefPtr<StyleSheet> mFirstChild;
+  // Pointer to the list of child sheets. This is all fundamentally broken,
+  // because each of the child sheets has a unique parent... We can only hope
+  // (and currently this is the case) that any time page JS can get its hands on
+  // a child sheet that means we've already ensured unique infos throughout its
+  // parent chain and things are good.
+  nsTArray<RefPtr<StyleSheet>> mChildren;
+
   AutoTArray<StyleSheet*, 8> mSheets;
 
   // If a SourceMap or X-SourceMap response header is seen, this is

testing/web-platform/tests/css/cssom/cssimportrule-sheet-identity.html

@@ -0,0 +1,24 @@
+<!doctype html>
+<title>CSSImportRule has different sheets even if referencing the same URL</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="help" href="https://drafts.csswg.org/cssom/#the-cssimportrule-interface">
+<link rel="stylesheet" href="support/import-rule.css">
+<link rel="stylesheet" href="support/import-rule.css">
+<script>
+let t = async_test("CSSImportRule has different sheets even if referencing the same URL");
+window.onload = t.step_func_done(function() {
+  let sheet1 = document.styleSheets[0];
+  let sheet2 = document.styleSheets[1];
+
+  assert_not_equals(sheet1, sheet2);
+
+  let childSheet1 = sheet1.cssRules[0].styleSheet;
+  let childSheet2 = sheet2.cssRules[0].styleSheet;
+
+  assert_not_equals(childSheet1, null);
+  assert_not_equals(childSheet2, null);
+  assert_not_equals(childSheet1, childSheet2, "@import pointing to the same URL shouldn't point to the same StyleSheet object");
+});
+</script>

testing/web-platform/tests/css/cssom/support/import-rule.css

@@ -0,0 +1 @@
+@import "import-red.css";