Bug 1400936 - Clear servo data after children data is cleared, and allow setting the root as the document if the tree is mid-unbind. r=bholley, a=sledru
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 23 Sep 2017 00:05:47 +0200
changeset 434252 ca0193447310df73222a91c485f6e41a8debe6f2
parent 434251 6b96a0fab002660434dc96f3ff8cf086cac1f4ac
child 434253 99992ed2b3f326a71044f04106a4c7795c5fdb40
push id1567
push userjlorenzo@mozilla.com
push dateThu, 02 Nov 2017 12:36:05 +0000
treeherdermozilla-release@e512c14a0406 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley, sledru
bugs1400936
milestone57.0
Bug 1400936 - Clear servo data after children data is cleared, and allow setting the root as the document if the tree is mid-unbind. r=bholley, a=sledru This is the actual fix, and makes sure that the state is consistent even if we notify of state changes on a parent during unbind. We potentially do a bit more work than needed given we set the document as the root in that case instead of the parent which could potentially be the root itself, but that's not a huge deal I think, given these cases are rare. If this happens to be a perf problem, we may want to just drop the root during UnbindFromTree if aNullParent == true and the root is a descendant of `this`. MozReview-Commit-ID: A9d2igM0hMr Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
dom/base/Element.cpp
dom/base/nsIDocumentInlines.h
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -1893,29 +1893,16 @@ Element::UnbindFromTree(bool aDeep, bool
     DeleteProperty(nsGkAtoms::transitionsOfBeforeProperty);
     DeleteProperty(nsGkAtoms::transitionsOfAfterProperty);
     DeleteProperty(nsGkAtoms::transitionsProperty);
     DeleteProperty(nsGkAtoms::animationsOfBeforeProperty);
     DeleteProperty(nsGkAtoms::animationsOfAfterProperty);
     DeleteProperty(nsGkAtoms::animationsProperty);
   }
 
-  // Computed style data isn't useful for detached nodes, and we'll need to
-  // recompute it anyway if we ever insert the nodes back into a document.
-  if (IsStyledByServo()) {
-    if (document) {
-      ClearServoData(document);
-    } else {
-      MOZ_ASSERT(!HasServoData());
-      MOZ_ASSERT(!HasAnyOfFlags(kAllServoDescendantBits | NODE_NEEDS_FRAME));
-    }
-  } else {
-    MOZ_ASSERT(!HasServoData());
-  }
-
   // Editable descendant count only counts descendants that
   // are in the uncomposed document.
   ResetEditableDescendantCount();
 
   if (aNullParent || !mParent->IsInShadowTree()) {
     UnsetFlags(NODE_IS_IN_SHADOW_TREE);
 
     // Begin keeping track of our subtree root.
@@ -1999,16 +1986,29 @@ Element::UnbindFromTree(bool aDeep, bool
   if (ShadowRoot* shadowRoot = GetShadowRoot()) {
     for (nsIContent* child = shadowRoot->GetFirstChild(); child;
          child = child->GetNextSibling()) {
       child->UnbindFromTree(true, false);
     }
 
     shadowRoot->SetIsComposedDocParticipant(false);
   }
+
+  // Computed style data isn't useful for detached nodes, and we'll need to
+  // recompute it anyway if we ever insert the nodes back into a document.
+  if (IsStyledByServo()) {
+    if (document) {
+      ClearServoData(document);
+    } else {
+      MOZ_ASSERT(!HasServoData());
+      MOZ_ASSERT(!HasAnyOfFlags(kAllServoDescendantBits | NODE_NEEDS_FRAME));
+    }
+  } else {
+    MOZ_ASSERT(!HasServoData());
+  }
 }
 
 nsICSSDeclaration*
 Element::GetSMILOverrideStyle()
 {
   Element::nsExtendedDOMSlots* slots = ExtendedDOMSlots();
 
   if (!slots->mSMILOverrideStyle) {
--- a/dom/base/nsIDocumentInlines.h
+++ b/dom/base/nsIDocumentInlines.h
@@ -61,18 +61,37 @@ nsIDocument::FindDocStyleSheetInsertionP
 inline void
 nsIDocument::SetServoRestyleRoot(nsINode* aRoot, uint32_t aDirtyBits)
 {
   MOZ_ASSERT(aRoot);
   MOZ_ASSERT(aDirtyBits);
   MOZ_ASSERT((aDirtyBits & ~Element::kAllServoDescendantBits) == 0);
   MOZ_ASSERT((aDirtyBits & mServoRestyleRootDirtyBits) == mServoRestyleRootDirtyBits);
 
+  // NOTE(emilio): The !aRoot->IsElement() check allows us to handle cases where
+  // we change the restyle root during unbinding of a subtree where the root is
+  // not unbound yet (and thus hasn't cleared the restyle root yet).
+  //
+  // In that case the tree can be in a somewhat inconsistent state (with the
+  // document no longer being the subtree root of the current root, but the root
+  // not having being unbound first).
+  //
+  // In that case, given there's no common ancestor, aRoot should be the
+  // document, and we allow that, provided that the previous root will
+  // eventually be unbound and the dirty bits will be cleared.
+  //
+  // If we want to enforce calling into this method with the tree in a
+  // consistent state, we'd need to move all the state changes that happen on
+  // content unbinding for parents, like fieldset validity stuff and ancestor
+  // direction changes off script runners or, alternatively, nulling out the
+  // document and parent node _after_ nulling out the children's, and then
+  // remove that line.
   MOZ_ASSERT(!mServoRestyleRoot ||
              mServoRestyleRoot == aRoot ||
+             !aRoot->IsElement() ||
              nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle(mServoRestyleRoot, aRoot));
   MOZ_ASSERT(aRoot == aRoot->OwnerDocAsNode() ||
              (aRoot->IsElement() && aRoot->IsInComposedDoc()));
   mServoRestyleRoot = aRoot;
   mServoRestyleRootDirtyBits = aDirtyBits;
 }
 
 #endif // nsIDocumentInlines_h