Bug 1484478 - Use a node bit for connectedness. r=smaug

While trying to repro bug 1484293 I noticed that this assertion failed:

https://searchfox.org/mozilla-central/rev/ef8b3886cb173d5534b954b6fb7eb2d94a9473d0/dom/base/ShadowRoot.cpp#160

(during unlink, while unbinding the kids)

We rely on GetComposedDoc returning the right thing during unbind to cleanup
some stuff (see bug 1473637 for example), so it should probably be correct all
the time, regardless of whether something is unlinked or not.

Also this makes GetComposedDoc() much faster, which is nice too, since we call
it somewhat often.

I removed NodeHasRelevantHoverRules, since it's unused (was used by the old
style system).

I moved the SetIsConnected(false) call for the shadow root to before unbinding
the kids for consistency with what Element does with the uncomposed doc flag,
now that the children's connectedness doesn't depend on the shadow root's.

Differential Revision: https://phabricator.services.mozilla.com/D3715

--HG--
extra : moz-landing-system : lando

dom/base/CharacterData.cpp

@@ -452,6 +452,8 @@ CharacterData::BindToTree(nsIDocument* aDocument,
              "aDocument must be current doc of aParent");
   MOZ_ASSERT(!GetUncomposedDoc() && !IsInUncomposedDoc(),
              "Already have a document.  Unbind first!");
+  MOZ_ASSERT(!IsInComposedDoc(),
+             "Already have a document.  Unbind first!");
   // Note that as we recurse into the kids, they'll have a non-null parent.  So
   // only assert if our parent is _changing_ while we have a parent.
   MOZ_ASSERT(!GetParent() || aParent == GetParent(),
@@ -489,14 +491,14 @@ CharacterData::BindToTree(nsIDocument* aDocument,
     if (HasFlag(NODE_IS_ANONYMOUS_ROOT)) {
       aParent->SetMayHaveAnonymousChildren();
     }
-    if (aParent->IsInShadowTree()) {
+  }
-      ClearSubtreeRootPointer();
+
-      SetFlags(NODE_IS_IN_SHADOW_TREE);
+  if (aParent && aParent->IsInShadowTree()) {
-    }
+    ClearSubtreeRootPointer();
-    ShadowRoot* parentContainingShadow = aParent->GetContainingShadow();
+    SetFlags(NODE_IS_IN_SHADOW_TREE);
-    if (parentContainingShadow) {
+    SetIsConnected(aParent->IsInComposedDoc());
-      ExtendedContentSlots()->mContainingShadow = parentContainingShadow;
+    MOZ_ASSERT(aParent->GetContainingShadow());
-    }
+    ExtendedContentSlots()->mContainingShadow = aParent->GetContainingShadow();
   }
 
   bool hadParent = !!GetParentNode();
@@ -507,8 +509,7 @@ CharacterData::BindToTree(nsIDocument* aDocument,
       NS_ADDREF(aParent);
     }
     mParent = aParent;
-  }
+  } else {
-  else {
     mParent = aDocument;
   }
   SetParentIsContent(aParent);
@@ -523,6 +524,7 @@ CharacterData::BindToTree(nsIDocument* aDocument,
 
     // XXX See the comment in Element::BindToTree
     SetIsInDocument();
+    SetIsConnected(true);
     if (mText.IsBidi()) {
       aDocument->SetBidiEnabled();
     }
@@ -553,8 +555,7 @@ void
 CharacterData::UnbindFromTree(bool aDeep, bool aNullParent)
 {
   // Unset frame flags; if we need them again later, they'll get set again.
-  UnsetFlags(NS_CREATE_FRAME_IF_NON_WHITESPACE |
+  UnsetFlags(NS_CREATE_FRAME_IF_NON_WHITESPACE | NS_REFRAME_IF_WHITESPACE);
-             NS_REFRAME_IF_WHITESPACE);
 
   nsIDocument* document = GetComposedDoc();
 
@@ -570,6 +571,7 @@ CharacterData::UnbindFromTree(bool aDeep, bool aNullParent)
     SetParentIsContent(false);
   }
   ClearInDocument();
+  SetIsConnected(false);
 
   if (aNullParent || !mParent->IsInShadowTree()) {
     UnsetFlags(NODE_IS_IN_SHADOW_TREE);

dom/base/Element.cpp

@@ -1258,8 +1258,6 @@ Element::AttachShadowWithoutNameChecks(ShadowRootMode aMode)
   RefPtr<ShadowRoot> shadowRoot =
     new ShadowRoot(this, aMode, nodeInfo.forget());
 
-  shadowRoot->SetIsComposedDocParticipant(IsInComposedDoc());
-
   if (NodeOrAncestorHasDirAuto()) {
     shadowRoot->SetAncestorHasDirAuto();
   }
@@ -1296,7 +1294,8 @@ Element::UnattachShadow()
 
   nsAutoScriptBlocker scriptBlocker;
 
-  if (nsIDocument* doc = GetComposedDoc()) {
+  nsIDocument* doc = GetComposedDoc();
+  if (doc) {
     if (nsIPresShell* shell = doc->GetShell()) {
       shell->DestroyFramesForAndRestyle(this);
     }
@@ -1304,13 +1303,8 @@ Element::UnattachShadow()
   MOZ_ASSERT(!GetPrimaryFrame());
 
   // Simply unhook the shadow root from the element.
-  MOZ_ASSERT(!GetShadowRoot()->HasSlots(), "Won't work when shadow root has slots!");
+  MOZ_ASSERT(!shadowRoot->HasSlots(), "Won't work when shadow root has slots!");
-  for (nsIContent* child = shadowRoot->GetFirstChild(); child;
+  shadowRoot->Unbind();
-       child = child->GetNextSibling()) {
-    child->UnbindFromTree(true, false);
-  }
-
-  shadowRoot->SetIsComposedDocParticipant(false);
   SetShadowRoot(nullptr);
 }
 
@@ -1625,7 +1619,8 @@ Element::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
              "Must have the same owner document");
   MOZ_ASSERT(!aParent || aDocument == aParent->GetUncomposedDoc(),
              "aDocument must be current doc of aParent");
-  MOZ_ASSERT(!GetUncomposedDoc(), "Already have a document.  Unbind first!");
+  MOZ_ASSERT(!IsInComposedDoc(), "Already have a document.  Unbind first!");
+  MOZ_ASSERT(!IsInUncomposedDoc(), "Already have a document.  Unbind first!");
   // Note that as we recurse into the kids, they'll have a non-null parent.  So
   // only assert if our parent is _changing_ while we have a parent.
   MOZ_ASSERT(!GetParent() || aParent == GetParent(),
@@ -1678,10 +1673,8 @@ Element::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
     if (aParent->IsInShadowTree()) {
       ClearSubtreeRootPointer();
       SetFlags(NODE_IS_IN_SHADOW_TREE);
-    }
+      MOZ_ASSERT(aParent->GetContainingShadow());
-    ShadowRoot* parentContainingShadow = aParent->GetContainingShadow();
+      ExtendedDOMSlots()->mContainingShadow = aParent->GetContainingShadow();
-    if (parentContainingShadow) {
-      ExtendedDOMSlots()->mContainingShadow = parentContainingShadow;
     }
   }
 
@@ -1693,8 +1686,7 @@ Element::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
       NS_ADDREF(aParent);
     }
     mParent = aParent;
-  }
+  } else {
-  else {
     mParent = aDocument;
   }
   SetParentIsContent(aParent);
@@ -1720,10 +1712,12 @@ Element::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
 
     // Being added to a document.
     SetIsInDocument();
+    SetIsConnected(true);
 
     // Clear the lazy frame construction bits.
     UnsetFlags(NODE_NEEDS_FRAME | NODE_DESCENDANTS_NEED_FRAMES);
   } else if (IsInShadowTree()) {
+    SetIsConnected(aParent->IsInComposedDoc());
     // We're not in a document, but we did get inserted into a shadow tree.
     // Since we won't have any restyle data in the document's restyle trackers,
     // don't let us get inserted with restyle bits set incorrectly.
@@ -1828,13 +1822,8 @@ Element::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
 
   // Call BindToTree on shadow root children.
   if (ShadowRoot* shadowRoot = GetShadowRoot()) {
-    shadowRoot->SetIsComposedDocParticipant(IsInComposedDoc());
+    rv = shadowRoot->Bind();
-    MOZ_ASSERT(shadowRoot->GetBindingParent() == this);
+    NS_ENSURE_SUCCESS(rv, rv);
-    for (nsIContent* child = shadowRoot->GetFirstChild(); child;
-         child = child->GetNextSibling()) {
-      rv = child->BindToTree(nullptr, shadowRoot, this);
-      NS_ENSURE_SUCCESS(rv, rv);
-    }
   }
 
   // FIXME(emilio): Why is this needed? The element shouldn't even be styled in
@@ -1994,6 +1983,7 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
 #endif
 
   ClearInDocument();
+  SetIsConnected(false);
 
   // Ensure that CSS transitions don't continue on an element at a
   // different place in the tree (even if reinserted before next
@@ -2104,12 +2094,7 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
 
   // Unbind children of shadow root.
   if (ShadowRoot* shadowRoot = GetShadowRoot()) {
-    for (nsIContent* child = shadowRoot->GetFirstChild(); child;
+    shadowRoot->Unbind();
-         child = child->GetNextSibling()) {
-      child->UnbindFromTree(true, false);
-    }
-
-    shadowRoot->SetIsComposedDocParticipant(false);
   }
 
   MOZ_ASSERT(!HasAnyOfFlags(kAllServoDescendantBits));

dom/base/FragmentOrElement.cpp

@@ -1465,13 +1465,7 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(FragmentOrElement)
   } */
 
   if (ShadowRoot* shadowRoot = tmp->GetShadowRoot()) {
-    for (nsIContent* child = shadowRoot->GetFirstChild();
+    shadowRoot->Unbind();
-         child;
-         child = child->GetNextSibling()) {
-      child->UnbindFromTree(true, false);
-    }
-
-    shadowRoot->SetIsComposedDocParticipant(false);
     tmp->ExtendedDOMSlots()->mShadowRoot = nullptr;
   }
 

dom/base/ShadowRoot.cpp

@@ -63,7 +63,6 @@ ShadowRoot::ShadowRoot(Element* aElement, ShadowRootMode aMode,
   , DocumentOrShadowRoot(*this)
   , mMode(aMode)
   , mServoStyles(Servo_AuthorStyles_Create())
-  , mIsComposedDocParticipant(false)
   , mIsUAWidget(false)
 {
   SetHost(aElement);
@@ -74,6 +73,7 @@ ShadowRoot::ShadowRoot(Element* aElement, ShadowRootMode aMode,
   ClearSubtreeRootPointer();
 
   SetFlags(NODE_IS_IN_SHADOW_TREE);
+  Bind();
 
   ExtendedDOMSlots()->mBindingParent = aElement;
   ExtendedDOMSlots()->mContainingShadow = this;
@@ -91,7 +91,7 @@ ShadowRoot::~ShadowRoot()
     host->RemoveMutationObserver(this);
   }
 
-  if (IsComposedDocParticipant()) {
+  if (IsInComposedDoc()) {
     OwnerDoc()->RemoveComposedDocShadowRoot(*this);
   }
 
@@ -103,23 +103,6 @@ ShadowRoot::~ShadowRoot()
   SetSubtreeRootPointer(this);
 }
 
-void
-ShadowRoot::SetIsComposedDocParticipant(bool aIsComposedDocParticipant)
-{
-  bool changed = mIsComposedDocParticipant != aIsComposedDocParticipant;
-  mIsComposedDocParticipant = aIsComposedDocParticipant;
-  if (!changed) {
-    return;
-  }
-
-  nsIDocument* doc = OwnerDoc();
-  if (IsComposedDocParticipant()) {
-    doc->AddComposedDocShadowRoot(*this);
-  } else {
-    doc->RemoveComposedDocShadowRoot(*this);
-  }
-}
-
 JSObject*
 ShadowRoot::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
 {
@@ -142,18 +125,50 @@ ShadowRoot::CloneInternalDataFrom(ShadowRoot* aOther)
   }
 }
 
+nsresult
+ShadowRoot::Bind()
+{
+  MOZ_ASSERT(!IsInComposedDoc(), "Forgot to unbind?");
+  if (Host()->IsInComposedDoc()) {
+    SetIsConnected(true);
+    OwnerDoc()->AddComposedDocShadowRoot(*this);
+  }
+
+  for (nsIContent* child = GetFirstChild();
+       child;
+       child = child->GetNextSibling()) {
+    nsresult rv = child->BindToTree(nullptr, this, Host());
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  return NS_OK;
+}
+
+void
+ShadowRoot::Unbind()
+{
+  if (IsInComposedDoc()) {
+    SetIsConnected(false);
+    OwnerDoc()->RemoveComposedDocShadowRoot(*this);
+  }
+
+  for (nsIContent* child = GetFirstChild();
+       child;
+       child = child->GetNextSibling()) {
+    child->UnbindFromTree(true, false);
+  }
+}
+
 void
 ShadowRoot::InvalidateStyleAndLayoutOnSubtree(Element* aElement)
 {
   MOZ_ASSERT(aElement);
-
+  nsIDocument* doc = GetComposedDoc();
-  if (!IsComposedDocParticipant()) {
+  if (!doc) {
     return;
   }
 
-  MOZ_ASSERT(GetComposedDoc() == OwnerDoc());
+  nsIPresShell* shell = doc->GetShell();
-
-  nsIPresShell* shell = OwnerDoc()->GetShell();
   if (!shell) {
     return;
   }
@@ -310,11 +325,11 @@ ShadowRoot::RuleChanged(StyleSheet&, css::Rule*) {
 void
 ShadowRoot::ApplicableRulesChanged()
 {
-  if (!IsComposedDocParticipant()) {
+  nsIDocument* doc = GetComposedDoc();
+  if (!doc) {
     return;
   }
 
-  nsIDocument* doc = OwnerDoc();
   if (nsIPresShell* shell = doc->GetShell()) {
     shell->RecordShadowStyleChange(*this);
   }
@@ -508,8 +523,8 @@ ShadowRoot::MaybeReassignElement(Element* aElement)
     return;
   }
 
-  if (IsComposedDocParticipant()) {
+  if (nsIDocument* doc = GetComposedDoc()) {
-    if (nsIPresShell* shell = OwnerDoc()->GetShell()) {
+    if (nsIPresShell* shell = doc->GetShell()) {
       shell->SlotAssignmentWillChange(*aElement, oldSlot, assignment.mSlot);
     }
   }

dom/base/ShadowRoot.h

@@ -89,6 +89,14 @@ public:
   void CloneInternalDataFrom(ShadowRoot* aOther);
   void InsertSheetAt(size_t aIndex, StyleSheet&);
 
+  // Calls UnbindFromTree for each of our kids, and also flags us as no longer
+  // being connected.
+  void Unbind();
+
+  // Calls BindToTree on each of our kids, and also maybe flags us as being
+  // connected.
+  nsresult Bind();
+
 private:
   void InsertSheetIntoAuthorData(size_t aIndex, StyleSheet&);
 
@@ -185,13 +193,6 @@ public:
                                          const nsAString& aTagName,
                                          mozilla::ErrorResult& rv);
 
-  bool IsComposedDocParticipant() const
-  {
-    return mIsComposedDocParticipant;
-  }
-
-  void SetIsComposedDocParticipant(bool aIsComposedDocParticipant);
-
   bool IsUAWidget() const
   {
     return mIsUAWidget;
@@ -222,12 +223,6 @@ protected:
   // are in the shadow tree and should be kept alive by its parent.
   nsClassHashtable<nsStringHashKey, SlotArray> mSlotMap;
 
-  // Flag to indicate whether the descendants of this shadow root are part of the
-  // composed document. Ideally, we would use a node flag on nodes to
-  // mark whether it is in the composed document, but we have run out of flags
-  // so instead we track it here.
-  bool mIsComposedDocParticipant;
-
   bool mIsUAWidget;
 
   nsresult Clone(dom::NodeInfo*, nsINode** aResult) const override;

dom/base/nsDocument.cpp

@@ -1519,6 +1519,7 @@ nsIDocument::nsIDocument()
     mNumTrackersBlocked(0)
 {
   SetIsInDocument();
+  SetIsConnected(true);
 }
 
 nsDocument::nsDocument(const char* aContentType)

dom/base/nsIDocument.h

@@ -3817,7 +3817,7 @@ protected:
 
   // A hashtable of ShadowRoots belonging to the composed doc.
   //
-  // See ShadowRoot::SetIsComposedDocParticipant.
+  // See ShadowRoot::Bind and ShadowRoot::Unbind.
   ShadowRootSet mComposedShadowRoots;
 
   using SVGUseElementSet =

dom/base/nsINode.cpp

@@ -251,8 +251,8 @@ nsINode::GetTextEditorRootContent(TextEditor** aTextEditor)
 nsINode* nsINode::GetRootNode(const GetRootNodeOptions& aOptions)
 {
   if (aOptions.mComposed) {
-    if (IsInComposedDoc() && GetComposedDoc()) {
+    if (nsIDocument* doc = GetComposedDoc()) {
-      return OwnerDoc();
+      return doc;
     }
 
     nsINode* node = this;
@@ -457,17 +457,6 @@ nsINode::GetTextContentInternal(nsAString& aTextContent, OOMReporter& aError)
   SetDOMStringToNull(aTextContent);
 }
 
-nsIDocument*
-nsINode::GetComposedDocInternal() const
-{
-  MOZ_ASSERT(HasFlag(NODE_IS_IN_SHADOW_TREE) && IsContent(),
-             "Should only be caled on nodes in the shadow tree.");
-
-  ShadowRoot* containingShadow = AsContent()->GetContainingShadow();
-  return containingShadow && containingShadow->IsComposedDocParticipant() ?
-    OwnerDoc() : nullptr;
-}
-
 DocumentOrShadowRoot*
 nsINode::GetUncomposedDocOrConnectedShadowRoot() const
 {
@@ -475,7 +464,7 @@ nsINode::GetUncomposedDocOrConnectedShadowRoot() const
     return OwnerDoc();
   }
 
-  if (IsInComposedDoc() && HasFlag(NODE_IS_IN_SHADOW_TREE)) {
+  if (IsInComposedDoc() && IsInShadowTree()) {
     return AsContent()->GetContainingShadow();
   }
 

dom/base/nsINode.h

@@ -642,6 +642,15 @@ public:
     return IsInUncomposedDoc() ? OwnerDoc() : nullptr;
   }
 
+  /**
+   * Returns true if we're connected, and thus GetComposedDoc() would return a
+   * non-null value.
+   */
+  bool IsInComposedDoc() const
+  {
+    return GetBoolFlag(IsConnected);
+  }
+
   /**
    * This method returns the owner document if the node is connected to it
    * (as defined in the DOM spec), otherwise it returns null.
@@ -651,16 +660,7 @@ public:
    */
   nsIDocument* GetComposedDoc() const
   {
-    return IsInShadowTree() ?
+    return IsInComposedDoc() ? OwnerDoc() : nullptr;
-      GetComposedDocInternal() : GetUncomposedDoc();
-  }
-
-  /**
-   * Returns true if GetComposedDoc() would return a non-null value.
-   */
-  bool IsInComposedDoc() const
-  {
-    return IsInUncomposedDoc() || (IsInShadowTree() && GetComposedDocInternal());
   }
 
   /**
@@ -1418,8 +1418,6 @@ private:
 
   mozilla::dom::SVGUseElement* DoGetContainingSVGUseShadowHost() const;
 
-  nsIDocument* GetComposedDocInternal() const;
-
   nsIContent* GetNextNodeImpl(const nsINode* aRoot,
                               const bool aSkipChildren) const
   {
@@ -1501,6 +1499,9 @@ private:
     // Set if our parent chain (including this node itself) terminates
     // in a document
     IsInDocument,
+    // Set if we're part of the composed doc.
+    // https://dom.spec.whatwg.org/#connected
+    IsConnected,
     // Set if mParent is an nsIContent
     ParentIsContent,
     // Set if this node is an Element
@@ -1555,8 +1556,6 @@ private:
     NodeAncestorHasDirAuto,
     // Set if the node is handling a click.
     NodeHandlingClick,
-    // Set if the node has had :hover selectors matched against it
-    NodeHasRelevantHoverRules,
     // Set if the element has a parser insertion mode other than "in body",
     // per the HTML5 "Parse state" section.
     ElementHasWeirdParserInsertionMode,
@@ -1680,8 +1679,6 @@ public:
   // Implemented in nsIContentInlines.h.
   inline bool NodeOrAncestorHasDirAuto() const;
 
-  bool HasRelevantHoverRules() const { return GetBoolFlag(NodeHasRelevantHoverRules); }
-  void SetHasRelevantHoverRules() { SetBoolFlag(NodeHasRelevantHoverRules); }
   void SetParserHasNotified() { SetBoolFlag(ParserHasNotified); };
   bool HasParserNotified() { return GetBoolFlag(ParserHasNotified); }
 
@@ -1700,8 +1697,9 @@ public:
 protected:
   void SetParentIsContent(bool aValue) { SetBoolFlag(ParentIsContent, aValue); }
   void SetIsInDocument() { SetBoolFlag(IsInDocument); }
-  void SetNodeIsContent() { SetBoolFlag(NodeIsContent); }
   void ClearInDocument() { ClearBoolFlag(IsInDocument); }
+  void SetIsConnected(bool aConnected) { SetBoolFlag(IsConnected, aConnected); }
+  void SetNodeIsContent() { SetBoolFlag(NodeIsContent); }
   void SetIsElement() { SetBoolFlag(NodeIsElement); }
   void SetHasID() { SetBoolFlag(ElementHasID); }
   void ClearHasID() { ClearBoolFlag(ElementHasID); }

dom/webidl/Node.webidl

@@ -34,7 +34,7 @@ interface Node : EventTarget {
   [Pure, Throws, NeedsCallerType, BinaryName="baseURIFromJS"]
   readonly attribute DOMString? baseURI;
 
-  [Pure, BinaryName=getComposedDoc]
+  [Pure, BinaryName=isInComposedDoc]
   readonly attribute boolean isConnected;
   [Pure]
   readonly attribute Document? ownerDocument;

layout/style/ServoStyleSet.cpp

@@ -176,7 +176,7 @@ EnumerateShadowRoots(const nsIDocument& aDoc, const Functor& aCb)
   for (auto iter = shadowRoots.ConstIter(); !iter.Done(); iter.Next()) {
     ShadowRoot* root = iter.Get()->GetKey();
     MOZ_ASSERT(root);
-    MOZ_DIAGNOSTIC_ASSERT(root->IsComposedDocParticipant());
+    MOZ_DIAGNOSTIC_ASSERT(root->IsInComposedDoc());
     aCb(*root);
   }
 }