Bug 1439395: Clear Servo data only when the DOM is in a consistent state. r=bholley

We used to do it this way effectively until I fixed it in bug 1400936.

Per the list of fuzz bugs that bug has in the "Depends on" field, some of those
without a super-clear fix, and others that aren't listed in there, and all the
complexity we had to deal with while receiving restyle requests mid-unbind, etc,
I think this is the right call.

This clears data on RestyleManager::ContentRemoved for non-anonymous nodes, and
on UnbindFromTree for subtrees rooted at anonymous nodes.

This will hopefully yield enforceable invariants.

MozReview-Commit-ID: IMwX5Uh1apv

--HG--
extra : rebase_source : 6cafc4499c9b80cbc96f1c4d1496e524f59e3c4d

dom/base/Element.cpp

@@ -69,6 +69,7 @@
 #include "mozilla/EventStates.h"
 #include "mozilla/InternalMutationEvent.h"
 #include "mozilla/MouseEvents.h"
+#include "mozilla/ServoRestyleManager.h"
 #include "mozilla/SizeOfState.h"
 #include "mozilla/TextEditor.h"
 #include "mozilla/TextEvents.h"
@@ -1925,6 +1926,15 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
     // Fully exit full-screen.
     nsIDocument::ExitFullscreenInDocTree(OwnerDoc());
   }
+
+  if (HasServoData()) {
+    MOZ_ASSERT(IsInAnonymousSubtree());
+    MOZ_ASSERT(document && document->IsStyledByServo());
+    ClearServoData(document);
+  } else if (document && document->GetServoRestyleRoot() == this) {
+    document->ClearServoRestyleRoot();
+  }
+
   if (aNullParent) {
     if (GetParent() && GetParent()->IsInUncomposedDoc()) {
       // Update the editable descendant count in the ancestors before we
@@ -1992,19 +2002,6 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
     }
   }
 
-  // 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();
@@ -2108,24 +2105,8 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
     shadowRoot->SetIsComposedDocParticipant(false);
   }
 
-  // Unbinding of children is the only point in time where we don't enforce the
+  MOZ_ASSERT_IF(IsStyledByServo(), !HasAnyOfFlags(kAllServoDescendantBits));
-  // "child has style data implies parent has it too" invariant.
+  MOZ_ASSERT(!document || document->GetServoRestyleRoot() != this);
-  //
-  // As such, the restyle root tracking may incorrectly end up setting dirty
-  // bits on the parent chain when moving from a not yet unbound root with
-  // already unbound parents to a root higher up in the tree, so we clear those
-  // (again, since they're also cleared in ClearServoData) here.
-  //
-  // This can happen when the element changes the state of some ancestor up in
-  // the tree, for example.
-  //
-  // Note that clearing the data itself here would have its own set of problems,
-  // since the invariant we'd be breaking in that case is "HasServoData()
-  // implies InComposedDoc()", which we rely on in various places.
-  UnsetFlags(kAllServoDescendantBits);
-  if (document && document->GetServoRestyleRoot() == this) {
-    document->ClearServoRestyleRoot();
-  }
 }
 
 nsDOMCSSAttributeDeclaration*
@@ -4516,7 +4497,6 @@ BitsArePropagated(const Element* aElement, uint32_t aBits, nsINode* aRestyleRoot
     nsINode* parentNode = curr->GetParentNode();
     curr = curr->GetFlattenedTreeParentElementForStyle();
     MOZ_ASSERT_IF(!curr,
-                  !parentNode || // can only happen mid-unbind.
                   parentNode == aElement->OwnerDoc() ||
                   parentNode == parentNode->OwnerDoc()->GetRootElement());
   }
@@ -4532,24 +4512,6 @@ AssertNoBitsPropagatedFrom(nsINode* aRoot)
     return;
   }
 
-  // If we are in the middle of unbinding a subtree, then the bits on elements
-  // in that subtree (and which we haven't yet unbound) could be dirty.
-  // Just skip asserting the absence of bits on those element that are in
-  // the unbinding subtree.  (The incorrect bits will only cause us to
-  // incorrectly choose a new restyle root if the newly dirty element is
-  // also within the unbinding subtree, so it is OK to leave them there.)
-  for (nsINode* n = aRoot; n; n = n->GetFlattenedTreeParentElementForStyle()) {
-    if (!n->IsInComposedDoc()) {
-      // Find the top-most element that is marked as no longer in the document,
-      // so we can start checking bits from its parent.
-      do {
-        aRoot = n;
-        n = n->GetFlattenedTreeParentElementForStyle();
-      } while (n && !n->IsInComposedDoc());
-      break;
-    }
-  }
-
   auto* element = aRoot->GetFlattenedTreeParentElementForStyle();
   while (element) {
     MOZ_ASSERT(!element->HasAnyOfFlags(Element::kAllServoDescendantBits));

dom/base/FragmentOrElement.cpp

@@ -1193,38 +1193,21 @@ void
 nsIContent::SetAssignedSlot(HTMLSlotElement* aSlot)
 {
   MOZ_ASSERT(aSlot || GetExistingExtendedContentSlots());
-  nsExtendedContentSlots* slots = ExtendedContentSlots();
+  ExtendedContentSlots()->mAssignedSlot = aSlot;
-
-  RefPtr<HTMLSlotElement> oldSlot = slots->mAssignedSlot.forget();
-  slots->mAssignedSlot = aSlot;
-
-  if (oldSlot != aSlot && IsElement() && AsElement()->HasServoData()) {
-    ServoRestyleManager::ClearServoDataFromSubtree(AsElement());
-  }
 }
 
 void
 nsIContent::SetXBLInsertionPoint(nsIContent* aContent)
 {
-  nsCOMPtr<nsIContent> oldInsertionPoint = nullptr;
   if (aContent) {
     nsExtendedContentSlots* slots = ExtendedContentSlots();
     SetFlags(NODE_MAY_BE_IN_BINDING_MNGR);
-    oldInsertionPoint = slots->mXBLInsertionPoint.forget();
     slots->mXBLInsertionPoint = aContent;
   } else {
     if (nsExtendedContentSlots* slots = GetExistingExtendedContentSlots()) {
-      oldInsertionPoint = slots->mXBLInsertionPoint.forget();
       slots->mXBLInsertionPoint = nullptr;
     }
   }
-
-  // We just changed the flattened tree, so any Servo style data is now invalid.
-  // We rely on nsXBLService::LoadBindings to re-traverse the subtree afterwards.
-  if (oldInsertionPoint != aContent && IsElement() &&
-      AsElement()->HasServoData()) {
-    ServoRestyleManager::ClearServoDataFromSubtree(AsElement());
-  }
 }
 
 nsresult

dom/base/nsIDocumentInlines.h

@@ -63,27 +63,8 @@ nsIDocument::SetServoRestyleRoot(nsINode* aRoot, uint32_t aDirtyBits)
 {
   MOZ_ASSERT(aRoot);
 
-  // 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());
   mServoRestyleRoot = aRoot;

layout/base/RestyleManager.cpp

@@ -252,6 +252,12 @@ RestyleManager::ContentRemoved(nsINode* aContainer,
                                nsIContent* aOldChild,
                                nsIContent* aFollowingSibling)
 {
+  // 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 (IsServo() && aOldChild->IsElement()) {
+    ServoRestyleManager::ClearServoDataFromSubtree(aOldChild->AsElement());
+  }
+
   // The container might be a document or a ShadowRoot.
   if (!aContainer->IsElement()) {
     return;