From dc2519c7cab7ef17e4a27fe7470b4c48344b90f6 Mon Sep 17 00:00:00 2001 From: Narcis Beleuzu Date: Fri, 30 Oct 2020 23:08:37 +0200 Subject: [PATCH] Backed out changeset 702857fa260e (bug 1673885) for wpt failures on ttwf-cssom-doc-ext-load-count.html CLOSED TREE --- dom/base/LinkStyle.cpp | 47 +++++++------------ dom/base/LinkStyle.h | 3 -- dom/webidl/LinkStyle.webidl | 2 +- dom/xml/ProcessingInstruction.cpp | 4 +- dom/xml/ProcessingInstruction.h | 2 +- layout/style/Loader.cpp | 45 +++++++----------- layout/style/Loader.h | 2 +- layout/style/SharedStyleSheetCache.cpp | 37 --------------- layout/style/SheetLoadData.h | 4 -- .../test/test_load_events_on_stylesheets.html | 6 ++- modules/libpref/init/StaticPrefList.yaml | 10 ---- 11 files changed, 42 insertions(+), 120 deletions(-) diff --git a/dom/base/LinkStyle.cpp b/dom/base/LinkStyle.cpp index 2efbd5192488..0764c1a0cb11 100644 --- a/dom/base/LinkStyle.cpp +++ b/dom/base/LinkStyle.cpp @@ -69,14 +69,6 @@ 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, @@ -210,18 +202,14 @@ Result 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 (mStyleSheet->IsComplete() || - StaticPrefs::dom_expose_incomplete_stylesheets()) { - 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 (aOldShadowRoot) { + aOldShadowRoot->RemoveStyleSheet(*mStyleSheet); + } else { + aOldDocument->RemoveStyleSheet(*mStyleSheet); } SetStyleSheet(nullptr); @@ -252,20 +240,17 @@ Result LinkStyle::DoUpdateStyleSheet( } if (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); + if (thisContent.IsInShadowTree()) { + ShadowRoot* containingShadow = thisContent.GetContainingShadow(); + // Could be null only during unlink. + if (MOZ_LIKELY(containingShadow)) { + containingShadow->RemoveStyleSheet(*mStyleSheet); } + } else { + doc->RemoveStyleSheet(*mStyleSheet); } - SetStyleSheet(nullptr); + LinkStyle::SetStyleSheet(nullptr); } if (!info) { diff --git a/dom/base/LinkStyle.h b/dom/base/LinkStyle.h index 20e967bf2aae..4bacaa704807 100644 --- a/dom/base/LinkStyle.h +++ b/dom/base/LinkStyle.h @@ -220,9 +220,6 @@ class LinkStyle { StyleSheet* GetSheet() const { return mStyleSheet; } - /** JS can only observe the sheet once fully loaded */ - StyleSheet* GetSheetForBindings() const; - protected: LinkStyle(); virtual ~LinkStyle(); diff --git a/dom/webidl/LinkStyle.webidl b/dom/webidl/LinkStyle.webidl index 27e9a4357cec..6bfde7b256c8 100644 --- a/dom/webidl/LinkStyle.webidl +++ b/dom/webidl/LinkStyle.webidl @@ -8,6 +8,6 @@ */ interface mixin LinkStyle { - [BinaryName="sheetForBindings"] readonly attribute StyleSheet? sheet; + readonly attribute StyleSheet? sheet; }; diff --git a/dom/xml/ProcessingInstruction.cpp b/dom/xml/ProcessingInstruction.cpp index f404697ae100..3383f2213c99 100644 --- a/dom/xml/ProcessingInstruction.cpp +++ b/dom/xml/ProcessingInstruction.cpp @@ -58,9 +58,9 @@ ProcessingInstruction::ProcessingInstruction( ProcessingInstruction::~ProcessingInstruction() = default; -StyleSheet* ProcessingInstruction::GetSheetForBindings() const { +StyleSheet* ProcessingInstruction::GetSheet() const { if (const auto* linkStyle = LinkStyle::FromNode(*this)) { - return linkStyle->GetSheetForBindings(); + return linkStyle->GetSheet(); } return nullptr; } diff --git a/dom/xml/ProcessingInstruction.h b/dom/xml/ProcessingInstruction.h index 47a4e3d1f202..b1a59ffcef4a 100644 --- a/dom/xml/ProcessingInstruction.h +++ b/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* GetSheetForBindings() const; + StyleSheet* GetSheet() const; NS_IMPL_FROMNODE_HELPER(ProcessingInstruction, IsProcessingInstruction()) diff --git a/layout/style/Loader.cpp b/layout/style/Loader.cpp index 4ad30627896a..a9efa5f82504 100644 --- a/layout/style/Loader.cpp +++ b/layout/style/Loader.cpp @@ -276,7 +276,6 @@ SheetLoadData::SheetLoadData(Loader* aLoader, const nsAString& aTitle, mPendingChildren(0), mSyncLoad(aSyncLoad), mIsNonDocumentSheet(false), - mIsChildSheet(aSheet->GetParentSheet()), mIsLoading(false), mIsBeingParsed(false), mIsCancelled(false), @@ -318,7 +317,6 @@ 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), @@ -343,7 +341,6 @@ 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( @@ -361,7 +358,6 @@ SheetLoadData::SheetLoadData( mPendingChildren(0), mSyncLoad(aSyncLoad), mIsNonDocumentSheet(true), - mIsChildSheet(false), mIsLoading(false), mIsBeingParsed(false), mIsCancelled(false), @@ -386,7 +382,6 @@ 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() { @@ -443,11 +438,7 @@ void SheetLoadData::FireLoadEvent(nsIThreadInternal* aThread) { RefPtr kungFuDeathGrip(this); aThread->RemoveObserver(this); - // 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... + // Now fire the event nsCOMPtr node = mOwningNode; MOZ_ASSERT(node, "How did that happen???"); @@ -1043,14 +1034,20 @@ Loader::MediaMatched Loader::PrepareSheet( * 3) Sheets with linking elements are ordered based on document order * as determined by CompareDocumentPosition. */ -void Loader::InsertSheetInTree(StyleSheet& aSheet, nsINode* aOwningNode) { +void Loader::InsertSheetInTree(StyleSheet& aSheet, + nsIContent* aLinkingContent) { LOG(("css::Loader::InsertSheetInTree")); MOZ_ASSERT(mDocument, "Must have a document to insert into"); - MOZ_ASSERT(!aOwningNode || aOwningNode->IsInUncomposedDoc() || - aOwningNode->IsInShadowTree(), + MOZ_ASSERT(!aLinkingContent || aLinkingContent->IsInUncomposedDoc() || + aLinkingContent->IsInShadowTree(), "Why would we insert it anywhere?"); + + if (auto* linkStyle = LinkStyle::FromNodeOrNull(aLinkingContent)) { + linkStyle->SetStyleSheet(&aSheet); + } + ShadowRoot* shadow = - aOwningNode ? aOwningNode->GetContainingShadow() : nullptr; + aLinkingContent ? aLinkingContent->GetContainingShadow() : nullptr; auto& target = shadow ? static_cast(*shadow) : static_cast(*mDocument); @@ -1070,7 +1067,7 @@ void Loader::InsertSheetInTree(StyleSheet& aSheet, nsINode* aOwningNode) { int32_t insertionPoint = sheetCount - 1; for (; insertionPoint >= 0; --insertionPoint) { nsINode* sheetOwner = target.SheetAt(insertionPoint)->GetOwnerNode(); - if (sheetOwner && !aOwningNode) { + if (sheetOwner && !aLinkingContent) { // Keep moving; all sheets with a sheetOwner come after all // sheets without a linkingNode continue; @@ -1082,11 +1079,11 @@ void Loader::InsertSheetInTree(StyleSheet& aSheet, nsINode* aOwningNode) { break; } - MOZ_ASSERT(aOwningNode != sheetOwner, + MOZ_ASSERT(aLinkingContent != sheetOwner, "Why do we still have our old sheet?"); // Have to compare - if (nsContentUtils::PositionIsBefore(sheetOwner, aOwningNode)) { + if (nsContentUtils::PositionIsBefore(sheetOwner, aLinkingContent)) { // The current sheet comes before us, and it better be the first // such, because now we break break; @@ -1698,12 +1695,7 @@ Result Loader::LoadInlineStyle( auto matched = PrepareSheet(*sheet, aInfo.mTitle, aInfo.mMedia, nullptr, isAlternate, aInfo.mIsExplicitlyEnabled); - if (auto* linkStyle = LinkStyle::FromNodeOrNull(aInfo.mContent)) { - linkStyle->SetStyleSheet(sheet); - } - if (StaticPrefs::dom_expose_incomplete_stylesheets() || sheet->IsComplete()) { - InsertSheetInTree(*sheet, aInfo.mContent); - } + InsertSheetInTree(*sheet, aInfo.mContent); Completed completed; if (sheetFromCache) { @@ -1801,12 +1793,7 @@ Result Loader::LoadStyleLink( auto matched = PrepareSheet(*sheet, aInfo.mTitle, aInfo.mMedia, nullptr, isAlternate, aInfo.mIsExplicitlyEnabled); - if (auto* linkStyle = LinkStyle::FromNodeOrNull(aInfo.mContent)) { - linkStyle->SetStyleSheet(sheet); - } - if (StaticPrefs::dom_expose_incomplete_stylesheets() || sheet->IsComplete()) { - InsertSheetInTree(*sheet, aInfo.mContent); - } + InsertSheetInTree(*sheet, aInfo.mContent); // We may get here with no content for Link: headers for example. MOZ_ASSERT(!aInfo.mContent || LinkStyle::FromNode(*aInfo.mContent), diff --git a/layout/style/Loader.h b/layout/style/Loader.h index a9f5647ea94f..597d17d7d13a 100644 --- a/layout/style/Loader.h +++ b/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, nsINode* aOwningNode); + void InsertSheetInTree(StyleSheet& aSheet, nsIContent* aLinkingContent); // Inserts a style sheet into a parent style sheet. void InsertChildSheet(StyleSheet& aSheet, StyleSheet& aParentSheet); diff --git a/layout/style/SharedStyleSheetCache.cpp b/layout/style/SharedStyleSheetCache.cpp index 4bbcdc7ffe68..04d4ec456ad4 100644 --- a/layout/style/SharedStyleSheetCache.cpp +++ b/layout/style/SharedStyleSheetCache.cpp @@ -337,43 +337,6 @@ 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()) { diff --git a/layout/style/SheetLoadData.h b/layout/style/SheetLoadData.h index 8b1d7932accd..fcb2ef061904 100644 --- a/layout/style/SheetLoadData.h +++ b/layout/style/SheetLoadData.h @@ -137,10 +137,6 @@ 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 diff --git a/layout/style/test/test_load_events_on_stylesheets.html b/layout/style/test/test_load_events_on_stylesheets.html index 34d73f9ad6fc..78251f1ba385 100644 --- a/layout/style/test/test_load_events_on_stylesheets.html +++ b/layout/style/test/test_load_events_on_stylesheets.html @@ -119,9 +119,13 @@ 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(link.sheet, 1); +checkSheetComplete(document.styleSheets[6], 1); ++pendingEventCounter; link = document.createElement("link"); diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 7c149d08e452..1a6ec3e532f7 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -2088,16 +2088,6 @@ 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