commit 3959e0d60e63440f0a868a1022dc088023c4ea88
parent aa05b6927eabd1b2e7227dab73d342e79707fb29
Author: Cristina Horotan <chorotan@mozilla.com>
Date: Thu, 18 Dec 2025 04:04:39 +0200
Revert "Bug 2006613 - Pass batch removal state through to unbind / unlink. r=smaug,dom-core" for causing crashtest failures on nsINode.h
This reverts commit 42f606b1668787424c77283bf1512739dbcf1ab9.
Revert "Bug 2006613 - Plumb batch removal state through unbind, and use it to optimize Shadow DOM removal. r=smaug"
This reverts commit 264c0f043745b5aa94a8db704da9fd4defe42b00.
Diffstat:
11 files changed, 51 insertions(+), 67 deletions(-)
diff --git a/dom/base/CharacterData.cpp b/dom/base/CharacterData.cpp
@@ -457,8 +457,7 @@ void CharacterData::UnbindFromTree(UnbindContext& aContext) {
UnsetFlags(NS_CREATE_FRAME_IF_NON_WHITESPACE | NS_REFRAME_IF_WHITESPACE);
const bool nullParent = aContext.IsUnbindRoot(this);
- HandleShadowDOMRelatedRemovalSteps(nullParent,
- !!aContext.GetBatchRemovalState());
+ HandleShadowDOMRelatedRemovalSteps(nullParent);
if (nullParent) {
if (GetParent()) {
diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp
@@ -2854,15 +2854,14 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Document)
nsINode::Unlink(tmp);
- BatchRemovalState state{};
- while (nsCOMPtr<nsIContent> child = tmp->GetLastChild()) {
+ while (tmp->HasChildren()) {
// Hold a strong ref to the node when we remove it, because we may be
// the last reference to it.
// If this code changes, change the corresponding code in Document's
// unlink impl and ContentUnbinder::UnbindSubtree.
+ nsCOMPtr<nsIContent> child = tmp->GetLastChild();
tmp->DisconnectChild(child);
- child->UnbindFromTree(/* aNewParent=*/nullptr, &state);
- state.mIsFirst = false;
+ child->UnbindFromTree();
}
tmp->UnlinkOriginalDocumentIfStatic();
diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
@@ -2500,8 +2500,7 @@ static bool WillDetachFromShadowOnUnbind(const Element& aElement,
void Element::UnbindFromTree(UnbindContext& aContext) {
const bool nullParent = aContext.IsUnbindRoot(this);
- HandleShadowDOMRelatedRemovalSteps(nullParent,
- !!aContext.GetBatchRemovalState());
+ HandleShadowDOMRelatedRemovalSteps(nullParent);
if (HasFlag(ELEMENT_IN_CONTENT_IDENTIFIER_FOR_LCP)) {
OwnerDoc()->ContentIdentifiersForLCP().Remove(this);
diff --git a/dom/base/FragmentOrElement.cpp b/dom/base/FragmentOrElement.cpp
@@ -148,9 +148,8 @@ nsIContent* nsIContent::FindFirstNonChromeOnlyAccessContent() const {
return nullptr;
}
-void nsIContent::UnbindFromTree(nsINode* aNewParent,
- const BatchRemovalState* aBatchState) {
- UnbindContext context(*this, aBatchState);
+void nsIContent::UnbindFromTree(nsINode* aNewParent) {
+ UnbindContext context(*this);
context.SetIsMove(aNewParent != nullptr);
UnbindFromTree(context);
}
@@ -1232,28 +1231,28 @@ class ContentUnbinder : public Runnable {
~ContentUnbinder() { Run(); }
void UnbindSubtree(nsIContent* aNode) {
- if (!aNode->HasChildren()) {
- return;
- }
if (aNode->NodeType() != nsINode::ELEMENT_NODE &&
aNode->NodeType() != nsINode::DOCUMENT_FRAGMENT_NODE) {
return;
}
- auto* container = static_cast<FragmentOrElement*>(aNode);
- // Invalidate cached array of child nodes
- container->InvalidateChildNodes();
- BatchRemovalState state{};
- while (nsCOMPtr<nsIContent> child = container->GetLastChild()) {
- // Hold a strong ref to the node when we remove it, because we may be
- // the last reference to it. We need to call DisconnectChild()
- // before calling UnbindFromTree, since this last can notify various
- // observers and they should really see consistent tree state.
- // If this code changes, change the corresponding code in
- // FragmentOrElement's and Document's unlink impls.
- container->DisconnectChild(child);
- UnbindSubtree(child);
- child->UnbindFromTree(/* aNewParent = */ nullptr, &state);
- state.mIsFirst = false;
+ FragmentOrElement* container = static_cast<FragmentOrElement*>(aNode);
+ if (container->HasChildren()) {
+ // Invalidate cached array of child nodes
+ container->InvalidateChildNodes();
+
+ while (container->HasChildren()) {
+ // Hold a strong ref to the node when we remove it, because we may be
+ // the last reference to it. We need to call DisconnectChild()
+ // before calling UnbindFromTree, since this last can notify various
+ // observers and they should really see consistent
+ // tree state.
+ // If this code changes, change the corresponding code in
+ // FragmentOrElement's and Document's unlink impls.
+ nsCOMPtr<nsIContent> child = container->GetLastChild();
+ container->DisconnectChild(child);
+ UnbindSubtree(child);
+ child->UnbindFromTree();
+ }
}
}
@@ -1330,15 +1329,14 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(FragmentOrElement)
if (tmp->UnoptimizableCCNode() || !nsCCUncollectableMarker::sGeneration) {
// Don't allow script to run while we're unbinding everything.
nsAutoScriptBlocker scriptBlocker;
- BatchRemovalState state{};
- while (nsCOMPtr<nsIContent> child = tmp->GetLastChild()) {
+ while (tmp->HasChildren()) {
// Hold a strong ref to the node when we remove it, because we may be
// the last reference to it.
// If this code changes, change the corresponding code in Document's
// unlink impl and ContentUnbinder::UnbindSubtree.
+ nsCOMPtr<nsIContent> child = tmp->GetLastChild();
tmp->DisconnectChild(child);
child->UnbindFromTree();
- state.mIsFirst = false;
}
} else if (!tmp->GetParent() && tmp->HasChildren()) {
ContentUnbinder::Append(tmp);
diff --git a/dom/base/ShadowRoot.cpp b/dom/base/ShadowRoot.cpp
@@ -184,7 +184,7 @@ void ShadowRoot::Unbind() {
OwnerDoc()->RemoveComposedDocShadowRoot(*this);
}
- UnbindContext context(*this, /* aBatchState = */ nullptr);
+ UnbindContext context(*this);
for (nsIContent* child = GetFirstChild(); child;
child = child->GetNextSibling()) {
child->UnbindFromTree(context);
@@ -824,7 +824,7 @@ nsINode* ShadowRoot::CreateElementAndAppendChildAt(nsINode& aParentNode,
return aParentNode.AppendChild(*node, rv);
}
-void ShadowRoot::MaybeUnslotHostChild(nsIContent& aChild, bool aInBatch) {
+void ShadowRoot::MaybeUnslotHostChild(nsIContent& aChild) {
// Need to null-check the host because we may be unlinked already.
MOZ_ASSERT(!GetHost() || aChild.GetParent() == GetHost());
@@ -837,19 +837,15 @@ void ShadowRoot::MaybeUnslotHostChild(nsIContent& aChild, bool aInBatch) {
"How did aChild end up assigned to a slot?");
// If the slot is going to start showing fallback content, we need to tell
// layout about it.
- if ((aInBatch || slot->AssignedNodes().Length() == 1) &&
- slot->HasChildren()) {
+ if (slot->AssignedNodes().Length() == 1 && slot->HasChildren()) {
InvalidateStyleAndLayoutOnSubtree(slot);
}
+ slot->RemoveAssignedNode(aChild);
slot->EnqueueSlotChangeEvent();
- if (aInBatch) {
- slot->ClearAssignedNodes();
- } else {
- slot->RemoveAssignedNode(aChild);
- if (mIsDetailsShadowTree && aChild.IsHTMLElement(nsGkAtoms::summary)) {
- MaybeReassignMainSummary(SummaryChangeReason::Deletion);
- }
+
+ if (mIsDetailsShadowTree && aChild.IsHTMLElement(nsGkAtoms::summary)) {
+ MaybeReassignMainSummary(SummaryChangeReason::Deletion);
}
}
diff --git a/dom/base/ShadowRoot.h b/dom/base/ShadowRoot.h
@@ -76,9 +76,7 @@ class ShadowRoot final : public DocumentFragment, public DocumentOrShadowRoot {
void MaybeSlotHostChild(nsIContent&);
// Called when a direct child of our host is removed. Tries to un-slot the
// child from the currently-assigned slot, if any.
- // If aInBatch is true, we know all the host kids are getting removed (and
- // thus we can just unassign all the kids at once).
- void MaybeUnslotHostChild(nsIContent&, bool aInBatch);
+ void MaybeUnslotHostChild(nsIContent&);
// Shadow DOM v1
Element* Host() const {
diff --git a/dom/base/UnbindContext.h b/dom/base/UnbindContext.h
@@ -22,21 +22,16 @@ struct MOZ_STACK_CLASS UnbindContext final {
// The parent node of the subtree we're unbinding from.
nsINode* GetOriginalSubtreeParent() const { return mOriginalParent; }
- explicit UnbindContext(nsINode& aRoot, const BatchRemovalState* aBatchState)
- : mRoot(aRoot),
- mOriginalParent(aRoot.GetParentNode()),
- mBatchState(aBatchState) {}
+ explicit UnbindContext(nsINode& aRoot)
+ : mRoot(aRoot), mOriginalParent(aRoot.GetParentNode()) {}
void SetIsMove(bool aIsMove) { mIsMove = aIsMove; }
bool IsMove() const { return mIsMove; }
- const BatchRemovalState* GetBatchRemovalState() const { return mBatchState; }
-
private:
nsINode& mRoot;
nsINode* const mOriginalParent;
- const BatchRemovalState* const mBatchState = nullptr;
// If set, we're moving the shadow-including inclusive ancestor.
bool mIsMove = false;
diff --git a/dom/base/nsIContent.h b/dom/base/nsIContent.h
@@ -114,8 +114,7 @@ class nsIContent : public nsINode {
* @note This method is safe to call on nodes that are not bound to a tree.
*/
virtual void UnbindFromTree(UnbindContext&) = 0;
- void UnbindFromTree(nsINode* aNewParent = nullptr,
- const BatchRemovalState* aBatchState = nullptr);
+ void UnbindFromTree(nsINode* aNewParent = nullptr);
enum {
/**
@@ -388,8 +387,7 @@ class nsIContent : public nsINode {
// Handles Shadow-DOM related state tracking. Meant to be called near the
// beginning of UnbindFromTree(), before the node has lost the reference to
// its parent.
- inline void HandleShadowDOMRelatedRemovalSteps(bool aNullParent,
- bool aInBatch);
+ inline void HandleShadowDOMRelatedRemovalSteps(bool aNullParent);
public:
/**
diff --git a/dom/base/nsIContentInlines.h b/dom/base/nsIContentInlines.h
@@ -251,15 +251,17 @@ inline void nsIContent::HandleShadowDOMRelatedInsertionSteps(bool aHadParent) {
}
}
-inline void nsIContent::HandleShadowDOMRelatedRemovalSteps(bool aNullParent,
- bool aInBatch) {
+inline void nsIContent::HandleShadowDOMRelatedRemovalSteps(bool aNullParent) {
using mozilla::dom::Element;
using mozilla::dom::ShadowRoot;
if (aNullParent) {
- if (Element* parentElement = Element::FromNode(mParent)) {
+ // FIXME(emilio, bug 1577141): FromNodeOrNull rather than just FromNode
+ // because XBL likes to call UnbindFromTree at very odd times (with already
+ // disconnected anonymous content subtrees).
+ if (Element* parentElement = Element::FromNodeOrNull(mParent)) {
if (ShadowRoot* shadow = parentElement->GetShadowRoot()) {
- shadow->MaybeUnslotHostChild(*this, aInBatch);
+ shadow->MaybeUnslotHostChild(*this);
}
HandleInsertionToOrRemovalFromSlot();
}
diff --git a/dom/base/nsINode.cpp b/dom/base/nsINode.cpp
@@ -2527,7 +2527,7 @@ void nsINode::RemoveChildNode(nsIContent* aKid, bool aNotify,
// Invalidate cached array of child nodes
InvalidateChildNodes();
- aKid->UnbindFromTree(aNewParent, aState);
+ aKid->UnbindFromTree(aNewParent);
}
// When replacing, aRefChild is the content being replaced; when
diff --git a/dom/base/nsINode.h b/dom/base/nsINode.h
@@ -251,9 +251,6 @@ enum class BatchRemovalOrder {
};
struct BatchRemovalState {
- // Whether we're the fist kid getting removed in the batch. Note that that's
- // different to whether we're the first _child_, if we're removing
- // back-to-front.
bool mIsFirst = true;
};
@@ -1050,15 +1047,18 @@ class nsINode : public mozilla::dom::EventTarget {
template <BatchRemovalOrder aOrder = BatchRemovalOrder::FrontToBack>
void RemoveAllChildren(bool aNotify) {
+ if (!HasChildren()) {
+ return;
+ }
BatchRemovalState state{};
- while (HasChildren()) {
+ do {
nsIContent* nodeToRemove = aOrder == BatchRemovalOrder::FrontToBack
? GetFirstChild()
: GetLastChild();
RemoveChildNode(nodeToRemove, aNotify, &state, nullptr,
MutationEffectOnScript::KeepTrustWorthiness);
state.mIsFirst = false;
- }
+ } while (HasChildren());
}
/**
