Bug 1414999: Synchronously clean style data from the DOM tree when the shell goes away. r=bz
☠☠ backed out by ac9bd5ff89bc ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 09 Nov 2017 18:29:36 +0100
changeset 392154 9418c23dfdba36e29cbc263849dd7973733d639f
parent 392153 0f783930e1b231009e96f7cfbbdb2f8324718cef
child 392155 ac9bd5ff89bc7a9b9c64655542e1ff5038c494b6
push id32911
push userrgurzau@mozilla.com
push dateThu, 16 Nov 2017 10:05:12 +0000
treeherdermozilla-central@92235deefc25 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1414999
milestone59.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 1414999: Synchronously clean style data from the DOM tree when the shell goes away. r=bz There's nothing preventing the flat tree from changing while the document doesn't have a shell. In that case, we really really don't want to lose track of elements with stale style data, since then we'll mess up. It's ok to _not_ clear the style data when the document goes into the BFCache though, because the document is thrown away if other document runs script and touches the cached DOM. MozReview-Commit-ID: 4W3xDAnnLPL
dom/base/nsDocument.cpp
dom/base/nsIDocument.h
layout/style/ServoStyleSet.cpp
layout/style/ServoStyleSet.h
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -1530,17 +1530,16 @@ nsIDocument::nsIDocument()
     mFontFaceSetDirty(true),
     mGetUserFontSetCalled(false),
     mPostedFlushUserFontSet(false),
     mDidFireDOMContentLoaded(true),
     mHasScrollLinkedEffect(false),
     mFrameRequestCallbacksScheduled(false),
     mIsTopLevelContentDocument(false),
     mIsContentDocument(false),
-    mMightHaveStaleServoData(false),
     mDidCallBeginLoad(false),
     mBufferingCSPViolations(false),
     mAllowPaymentRequest(false),
     mIsScopedStyleEnabled(eScopedStyle_Unknown),
     mCompatMode(eCompatibility_FullStandards),
     mReadyState(ReadyState::READYSTATE_UNINITIALIZED),
     mStyleBackendType(StyleBackendType::None),
 #ifdef MOZILLA_INTERNAL_API
@@ -4113,18 +4112,27 @@ nsDocument::CreateShell(nsPresContext* a
                         StyleSetHandle aStyleSet)
 {
   NS_ASSERTION(!mPresShell, "We have a presshell already!");
 
   NS_ENSURE_FALSE(GetBFCacheEntry(), nullptr);
 
   FillStyleSet(aStyleSet);
 
-  // Ensure we start with no stale data in the tree.
-  ClearStaleServoDataFromDocument();
+  {
+#ifdef DEBUG
+    for (nsINode* node = static_cast<nsINode*>(this)->GetFirstChild();
+         node;
+         node = node->GetNextNode(this)) {
+      if (node->IsElement()) {
+        MOZ_ASSERT(!node->AsElement()->HasServoData());
+      }
+    }
+#endif
+  }
 
   RefPtr<PresShell> shell = new PresShell;
   shell->Init(this, aContext, aViewManager, aStyleSet);
 
   // Note: we don't hold a ref to the shell (it holds a ref to us)
   mPresShell = shell;
 
   // Make sure to never paint if we belong to an invisible DocShell.
@@ -4239,26 +4247,27 @@ nsDocument::DeleteShell()
   // objects for @font-face rules that came from the style set.
   RebuildUserFontSet();
 
   nsIPresShell* oldShell = mPresShell;
   mPresShell = nullptr;
   UpdateFrameRequestCallbackSchedulingState(oldShell);
   mStyleSetFilled = false;
 
-  // Record that the tree might have stale Servo element data in it
-  // that would need to be cleared if we ever get a new pres shell
-  // or if we call ServoStyleSet style resolving functions on
-  // elements in the document. Most of the time this lazy clearing
-  // of Servo element data saves us work, since it's not often that a
-  // document gets a new pres shell after its old one is destroyed.
-  // In those cases we rely on the data being cleared in UnbindFromTree
-  // and save this additional traversal.
   if (IsStyledByServo()) {
-    mMightHaveStaleServoData = true;
+    ClearStaleServoData();
+#ifdef DEBUG
+    for (nsINode* node = static_cast<nsINode*>(this)->GetFirstChild();
+         node;
+         node = node->GetNextNode(this)) {
+      if (node->IsElement()) {
+        MOZ_ASSERT(!node->AsElement()->HasServoData());
+      }
+    }
+#endif
   }
 }
 
 static void
 SubDocClearEntry(PLDHashTable *table, PLDHashEntryHdr *entry)
 {
   SubDocMapEntry *e = static_cast<SubDocMapEntry *>(entry);
 
@@ -14343,27 +14352,22 @@ nsIDocument::IsScopedStyleEnabled()
                             nsContentUtils::IsScopedStylePrefEnabled()
                               ? eScopedStyle_Enabled
                               : eScopedStyle_Disabled;
   }
   return mIsScopedStyleEnabled == eScopedStyle_Enabled;
 }
 
 void
-nsIDocument::ClearStaleServoDataFromDocument()
-{
-  if (!mMightHaveStaleServoData) {
-    return;
-  }
-
+nsIDocument::ClearStaleServoData()
+{
   DocumentStyleRootIterator iter(this);
   while (Element* root = iter.GetNextStyleRoot()) {
     ServoRestyleManager::ClearServoDataFromSubtree(root);
   }
-  mMightHaveStaleServoData = false;
 }
 
 Selection*
 nsIDocument::GetSelection(ErrorResult& aRv)
 {
   nsCOMPtr<nsPIDOMWindowInner> window = GetInnerWindow();
   if (!window) {
     return nullptr;
--- a/dom/base/nsIDocument.h
+++ b/dom/base/nsIDocument.h
@@ -1128,16 +1128,21 @@ protected:
   }
 
   void CleanUnloadEventsTimeStamp()
   {
     MOZ_ASSERT(mPageUnloadingEventTimeStamp);
     mPageUnloadingEventTimeStamp = mozilla::TimeStamp();
   }
 
+  /**
+   * Clears any Servo element data stored on Elements in the document.
+   */
+  void ClearStaleServoData();
+
 private:
   class SelectorCacheKey
   {
     public:
       explicit SelectorCacheKey(const nsAString& aString) : mKey(aString)
       {
         MOZ_COUNT_CTOR(SelectorCacheKey);
       }
@@ -1858,28 +1863,16 @@ public:
 
   bool IsContentDocument() const { return mIsContentDocument; }
   void SetIsContentDocument(bool aIsContentDocument)
   {
     mIsContentDocument = aIsContentDocument;
   }
 
   /**
-   * Checks if this document has no pres shell, and if so, clears any Servo
-   * element data stored on Elements in the document.
-   */
-  void ClearStaleServoDataFromDocument();
-
-  /**
-   * Returns true if there may be Servo element data on Elements in the document
-   * that were created for a pres shell that no longer exists.
-   */
-  bool MightHaveStaleServoData() const { return mMightHaveStaleServoData; }
-
-  /**
    * Create an element with the specified name, prefix and namespace ID.
    * Returns null if element name parsing failed.
    */
   virtual already_AddRefed<Element> CreateElem(const nsAString& aName,
                                                nsAtom* aPrefix,
                                                int32_t aNamespaceID,
                                                const nsAString* aIs = nullptr) = 0;
 
@@ -3585,20 +3578,16 @@ protected:
   // This should generally be updated only via
   // UpdateFrameRequestCallbackSchedulingState.
   bool mFrameRequestCallbacksScheduled : 1;
 
   bool mIsTopLevelContentDocument : 1;
 
   bool mIsContentDocument : 1;
 
-  // True if there may be Servo element data on Elements in the document that
-  // were created for a pres shell that no longer exists.
-  bool mMightHaveStaleServoData : 1;
-
   // True if we have called BeginLoad and are expecting a paired EndLoad call.
   bool mDidCallBeginLoad : 1;
 
   // True if any CSP violation reports for this doucment will be buffered in
   // mBufferedCSPViolations instead of being sent immediately.
   bool mBufferingCSPViolations : 1;
 
   // True if the document is allowed to use PaymentRequest.
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -344,55 +344,16 @@ ServoStyleSet::ResolveStyleFor(Element* 
     PreTraverseSync();
     return ResolveStyleLazilyInternal(
         aElement, CSSPseudoElementType::NotPseudo);
   }
 
   return ResolveServoStyle(aElement);
 }
 
-/**
- * Clears any stale Servo element data that might existing in the specified
- * element's document.  Upon destruction, asserts that the element and all
- * its ancestors still have no element data, if the document has no pres shell.
- */
-class MOZ_STACK_CLASS AutoClearStaleData
-{
-public:
-  explicit AutoClearStaleData(Element* aElement)
-#ifdef DEBUG
-    : mElement(aElement)
-#endif
-  {
-    aElement->OwnerDoc()->ClearStaleServoDataFromDocument();
-  }
-
-  ~AutoClearStaleData()
-  {
-#ifdef DEBUG
-    // Assert that the element and its ancestors are all unstyled, if the
-    // document has no pres shell.
-    if (mElement->OwnerDoc()->HasShellOrBFCacheEntry()) {
-      // We must check whether we're in the bfcache because its presence
-      // means we have a "hidden" pres shell with up-to-date data in the
-      // tree.
-      return;
-    }
-    for (Element* e = mElement; e; e = e->GetParentElement()) {
-      MOZ_ASSERT(!e->HasServoData(), "expected element to be unstyled");
-    }
-#endif
-  }
-
-private:
-#ifdef DEBUG
-  Element* mElement;
-#endif
-};
-
 const ServoElementSnapshotTable&
 ServoStyleSet::Snapshots()
 {
   return mPresContext->RestyleManager()->AsServo()->Snapshots();
 }
 
 void
 ServoStyleSet::ResolveMappedAttrDeclarationBlocks()
@@ -596,38 +557,20 @@ ServoStyleSet::ResolvePseudoElementStyle
   return computedValues.forget();
 }
 
 already_AddRefed<ServoStyleContext>
 ServoStyleSet::ResolveStyleLazily(Element* aElement,
                                   CSSPseudoElementType aPseudoType,
                                   StyleRuleInclusion aRuleInclusion)
 {
-  // Lazy style computation avoids storing any new data in the tree.
-  // If the tree has stale data in it, then the AutoClearStaleData below
-  // will ensure it's cleared so we don't use it. But if the document is
-  // in the bfcache, then we will have valid, usable data in the tree,
-  // but we don't want to use it. Instead we want to pretend as if the
-  // document has no pres shell and no styles.
-  //
-  // If we don't do this, then we can very easily mix styles from different
-  // style sets in the tree. For example, calling getComputedStyle on an
-  // element in a display:none iframe (which has no pres shell) will use the
-  // caller's style set for any styling. If we allowed this to re-use any
-  // existing styles in the DOM, then we would do selector matching on the
-  // undisplayed element with the caller's style set's rules, but inherit from
-  // values that were computed with the style set from the target element's
-  // hidden-by-the-bfcache-entry pres shell.
-  bool ignoreExistingStyles = aElement->OwnerDoc()->GetBFCacheEntry();
+  PreTraverseSync();
 
-  AutoClearStaleData guard(aElement);
-  PreTraverseSync();
   return ResolveStyleLazilyInternal(aElement, aPseudoType,
-                                    aRuleInclusion,
-                                    ignoreExistingStyles);
+                                    aRuleInclusion);
 }
 
 already_AddRefed<ServoStyleContext>
 ServoStyleSet::ResolveInheritingAnonymousBoxStyle(nsAtom* aPseudoTag,
                                                   ServoStyleContext* aParentContext)
 {
   MOZ_ASSERT(nsCSSAnonBoxes::IsAnonBox(aPseudoTag) &&
              !nsCSSAnonBoxes::IsNonInheritingAnonBox(aPseudoTag));
@@ -1193,22 +1136,16 @@ ServoStyleSet::GetKeyframesForName(nsAto
 }
 
 nsTArray<ComputedKeyframeValues>
 ServoStyleSet::GetComputedKeyframeValuesFor(
   const nsTArray<Keyframe>& aKeyframes,
   Element* aElement,
   const ServoStyleContext* aContext)
 {
-  // Servo_GetComputedKeyframeValues below won't handle ignoring existing
-  // element data for bfcached documents. (See comment in ResolveStyleLazily
-  // about these bfcache issues.)
-  MOZ_RELEASE_ASSERT(!aElement->OwnerDoc()->GetBFCacheEntry());
-
-  AutoClearStaleData guard(aElement);
   nsTArray<ComputedKeyframeValues> result(aKeyframes.Length());
 
   // Construct each nsTArray<PropertyStyleAnimationValuePair> here.
   result.AppendElements(aKeyframes.Length());
 
   Servo_GetComputedKeyframeValues(&aKeyframes,
                                   aElement,
                                   aContext,
@@ -1222,79 +1159,57 @@ ServoStyleSet::GetAnimationValues(
   RawServoDeclarationBlock* aDeclarations,
   Element* aElement,
   const ServoStyleContext* aStyleContext,
   nsTArray<RefPtr<RawServoAnimationValue>>& aAnimationValues)
 {
   // Servo_GetAnimationValues below won't handle ignoring existing element
   // data for bfcached documents. (See comment in ResolveStyleLazily
   // about these bfcache issues.)
-  MOZ_RELEASE_ASSERT(!aElement->OwnerDoc()->GetBFCacheEntry());
-
-  AutoClearStaleData guard(aElement);
   Servo_GetAnimationValues(aDeclarations,
                            aElement,
                            aStyleContext,
                            mRawSet.get(),
                            &aAnimationValues);
 }
 
 already_AddRefed<ServoStyleContext>
 ServoStyleSet::GetBaseContextForElement(
   Element* aElement,
   nsPresContext* aPresContext,
   CSSPseudoElementType aPseudoType,
   const ServoStyleContext* aStyle)
 {
-  // Servo_StyleSet_GetBaseComputedValuesForElement below won't handle ignoring
-  // existing element data for bfcached documents. (See comment in
-  // ResolveStyleLazily about these bfcache issues.)
-  MOZ_RELEASE_ASSERT(!aElement->OwnerDoc()->GetBFCacheEntry(),
-             "GetBaseContextForElement does not support documents in the "
-             "bfcache");
-
-  AutoClearStaleData guard(aElement);
   return Servo_StyleSet_GetBaseComputedValuesForElement(mRawSet.get(),
                                                         aElement,
                                                         aStyle,
                                                         &Snapshots(),
                                                         aPseudoType).Consume();
 }
 
 already_AddRefed<ServoStyleContext>
 ServoStyleSet::ResolveServoStyleByAddingAnimation(
   Element* aElement,
   const ServoStyleContext* aStyle,
   RawServoAnimationValue* aAnimationValue)
 {
-  MOZ_RELEASE_ASSERT(!aElement->OwnerDoc()->GetBFCacheEntry(),
-                     "ResolveServoStyleByAddingAniamtion does not support "
-                     "documents in the bfcache");
-
-  AutoClearStaleData guard(aElement);
   return Servo_StyleSet_GetComputedValuesByAddingAnimation(
     mRawSet.get(),
     aElement,
     aStyle,
     &Snapshots(),
     aAnimationValue).Consume();
 }
 
 already_AddRefed<RawServoAnimationValue>
 ServoStyleSet::ComputeAnimationValue(
   Element* aElement,
   RawServoDeclarationBlock* aDeclarations,
   const ServoStyleContext* aContext)
 {
-  // Servo_AnimationValue_Compute below won't handle ignoring existing element
-  // data for bfcached documents. (See comment in ResolveStyleLazily about
-  // these bfcache issues.)
-  MOZ_RELEASE_ASSERT(!aElement->OwnerDoc()->GetBFCacheEntry());
-
-  AutoClearStaleData guard(aElement);
   return Servo_AnimationValue_Compute(aElement,
                                       aDeclarations,
                                       aContext,
                                       mRawSet.get()).Consume();
 }
 
 bool
 ServoStyleSet::EnsureUniqueInnerOnCSSSheets()
@@ -1368,18 +1283,17 @@ ServoStyleSet::ClearNonInheritingStyleCo
   for (RefPtr<ServoStyleContext>& ptr : mNonInheritingStyleContexts) {
     ptr = nullptr;
   }
 }
 
 already_AddRefed<ServoStyleContext>
 ServoStyleSet::ResolveStyleLazilyInternal(Element* aElement,
                                           CSSPseudoElementType aPseudoType,
-                                          StyleRuleInclusion aRuleInclusion,
-                                          bool aIgnoreExistingStyles)
+                                          StyleRuleInclusion aRuleInclusion)
 {
   mPresContext->EffectCompositor()->PreTraverse(aElement, aPseudoType);
   MOZ_ASSERT(!StylistNeedsUpdate());
 
   AutoSetInServoTraversal guard(this);
 
   /**
    * NB: This is needed because we process animations and transitions on the
@@ -1407,28 +1321,31 @@ ServoStyleSet::ResolveStyleLazilyInterna
   }
 
   RefPtr<ServoStyleContext> computedValues =
     Servo_ResolveStyleLazily(elementForStyleResolution,
                              pseudoTypeForStyleResolution,
                              aRuleInclusion,
                              &Snapshots(),
                              mRawSet.get(),
-                             aIgnoreExistingStyles).Consume();
+                             /* aIgnoreExistingStyles = */ false).Consume();
 
   if (mPresContext->EffectCompositor()->PreTraverse(aElement, aPseudoType)) {
     computedValues =
       Servo_ResolveStyleLazily(elementForStyleResolution,
                                pseudoTypeForStyleResolution,
                                aRuleInclusion,
                                &Snapshots(),
                                mRawSet.get(),
-                               aIgnoreExistingStyles).Consume();
+                               /* aIgnoreExistingStyles = */ false).Consume();
   }
 
+  MOZ_DIAGNOSTIC_ASSERT(computedValues->PresContext() == mPresContext ||
+                        aElement->OwnerDoc()->GetBFCacheEntry());
+
   return computedValues.forget();
 }
 
 bool
 ServoStyleSet::AppendFontFaceRules(nsTArray<nsFontFaceRuleContainer>& aArray)
 {
   UpdateStylistIfNeeded();
   Servo_StyleSet_GetFontFaceRules(mRawSet.get(), &aArray);
--- a/layout/style/ServoStyleSet.h
+++ b/layout/style/ServoStyleSet.h
@@ -592,18 +592,17 @@ private:
    * This should only be called if StylistNeedsUpdate returns true.
    */
   void UpdateStylist();
 
   already_AddRefed<ServoStyleContext>
     ResolveStyleLazilyInternal(dom::Element* aElement,
                                CSSPseudoElementType aPseudoType,
                                StyleRuleInclusion aRules =
-                                 StyleRuleInclusion::All,
-                               bool aIgnoreExistingStyles = false);
+                                 StyleRuleInclusion::All);
 
   void RunPostTraversalTasks();
 
   void PrependSheetOfType(SheetType aType,
                           ServoStyleSheet* aSheet);
 
   void AppendSheetOfType(SheetType aType,
                          ServoStyleSheet* aSheet);