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
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 23 Sep 2017 00:05:47 +0200
changeset 382560 dba974e6b1b4ecf1d0ee788e0279fd2b9f410b75
parent 382559 9ef85da5aea4dfa3a22436fe4386a7bd25183cbe
child 382561 7f9883dd37feac26fb95b629ad1010107f04603c
push id32561
push userarchaeopteryx@coole-files.de
push dateSat, 23 Sep 2017 09:36:26 +0000
treeherdermozilla-central@8c3a15583223 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1400936
milestone58.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 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 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