commit 09b0c2e50449d8c16ad4500a8db1991366319b96
parent 46e9071cf48d5582fcd66b9692e6b6ca232eccdd
Author: Emilio Cobos Álvarez <emilio@crisal.io>
Date: Thu, 18 Dec 2025 09:42:56 +0000
Bug 2006613 - Plumb batch removal state through unbind, and use it to optimize Shadow DOM removal. r=smaug
This shows up in bug 2000414, where the front-end clears all the
menuitems of a <menupopup> at once, and they are all slotted, and
clearing the assigned node list from the front is more expensive than
necessary. See the profile in: https://share.firefox.dev/493zcYj
Of course, it'd be better to remove back to front, but that would be
observable, and it seems we can improve our batch removal machinery to
deal with this case quite easily, since if all the host kids are going
away, we know we will end up with no assigned nodes at all.
Differential Revision: https://phabricator.services.mozilla.com/D276828
Diffstat:
10 files changed, 40 insertions(+), 26 deletions(-)
diff --git a/dom/base/CharacterData.cpp b/dom/base/CharacterData.cpp
@@ -457,7 +457,8 @@ void CharacterData::UnbindFromTree(UnbindContext& aContext) {
UnsetFlags(NS_CREATE_FRAME_IF_NON_WHITESPACE | NS_REFRAME_IF_WHITESPACE);
const bool nullParent = aContext.IsUnbindRoot(this);
- HandleShadowDOMRelatedRemovalSteps(nullParent);
+ HandleShadowDOMRelatedRemovalSteps(nullParent,
+ !!aContext.GetBatchRemovalState());
if (nullParent) {
if (GetParent()) {
diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
@@ -2500,7 +2500,8 @@ static bool WillDetachFromShadowOnUnbind(const Element& aElement,
void Element::UnbindFromTree(UnbindContext& aContext) {
const bool nullParent = aContext.IsUnbindRoot(this);
- HandleShadowDOMRelatedRemovalSteps(nullParent);
+ HandleShadowDOMRelatedRemovalSteps(nullParent,
+ !!aContext.GetBatchRemovalState());
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,8 +148,9 @@ nsIContent* nsIContent::FindFirstNonChromeOnlyAccessContent() const {
return nullptr;
}
-void nsIContent::UnbindFromTree(nsINode* aNewParent) {
- UnbindContext context(*this);
+void nsIContent::UnbindFromTree(nsINode* aNewParent,
+ const BatchRemovalState* aBatchState) {
+ UnbindContext context(*this, aBatchState);
context.SetIsMove(aNewParent != nullptr);
UnbindFromTree(context);
}
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);
+ UnbindContext context(*this, /* aBatchState = */ nullptr);
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) {
+void ShadowRoot::MaybeUnslotHostChild(nsIContent& aChild, bool aInBatch) {
// Need to null-check the host because we may be unlinked already.
MOZ_ASSERT(!GetHost() || aChild.GetParent() == GetHost());
@@ -837,15 +837,19 @@ void ShadowRoot::MaybeUnslotHostChild(nsIContent& aChild) {
"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 (slot->AssignedNodes().Length() == 1 && slot->HasChildren()) {
+ if ((aInBatch || slot->AssignedNodes().Length() == 1) &&
+ slot->HasChildren()) {
InvalidateStyleAndLayoutOnSubtree(slot);
}
- slot->RemoveAssignedNode(aChild);
slot->EnqueueSlotChangeEvent();
-
- if (mIsDetailsShadowTree && aChild.IsHTMLElement(nsGkAtoms::summary)) {
- MaybeReassignMainSummary(SummaryChangeReason::Deletion);
+ if (aInBatch) {
+ slot->ClearAssignedNodes();
+ } else {
+ slot->RemoveAssignedNode(aChild);
+ if (mIsDetailsShadowTree && aChild.IsHTMLElement(nsGkAtoms::summary)) {
+ MaybeReassignMainSummary(SummaryChangeReason::Deletion);
+ }
}
}
diff --git a/dom/base/ShadowRoot.h b/dom/base/ShadowRoot.h
@@ -76,7 +76,9 @@ 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.
- void MaybeUnslotHostChild(nsIContent&);
+ // 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);
// Shadow DOM v1
Element* Host() const {
diff --git a/dom/base/UnbindContext.h b/dom/base/UnbindContext.h
@@ -22,16 +22,21 @@ 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)
- : mRoot(aRoot), mOriginalParent(aRoot.GetParentNode()) {}
+ explicit UnbindContext(nsINode& aRoot, const BatchRemovalState* aBatchState)
+ : mRoot(aRoot),
+ mOriginalParent(aRoot.GetParentNode()),
+ mBatchState(aBatchState) {}
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,7 +114,8 @@ 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);
+ void UnbindFromTree(nsINode* aNewParent = nullptr,
+ const BatchRemovalState* aBatchState = nullptr);
enum {
/**
@@ -387,7 +388,8 @@ 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);
+ inline void HandleShadowDOMRelatedRemovalSteps(bool aNullParent,
+ bool aInBatch);
public:
/**
diff --git a/dom/base/nsIContentInlines.h b/dom/base/nsIContentInlines.h
@@ -251,17 +251,18 @@ inline void nsIContent::HandleShadowDOMRelatedInsertionSteps(bool aHadParent) {
}
}
-inline void nsIContent::HandleShadowDOMRelatedRemovalSteps(bool aNullParent) {
+inline void nsIContent::HandleShadowDOMRelatedRemovalSteps(bool aNullParent,
+ bool aInBatch) {
using mozilla::dom::Element;
using mozilla::dom::ShadowRoot;
if (aNullParent) {
// 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).
+ // because frame destruction 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);
+ shadow->MaybeUnslotHostChild(*this, aInBatch);
}
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);
+ aKid->UnbindFromTree(aNewParent, aState);
}
// When replacing, aRefChild is the content being replaced; when
diff --git a/dom/base/nsINode.h b/dom/base/nsINode.h
@@ -1047,18 +1047,15 @@ class nsINode : public mozilla::dom::EventTarget {
template <BatchRemovalOrder aOrder = BatchRemovalOrder::FrontToBack>
void RemoveAllChildren(bool aNotify) {
- if (!HasChildren()) {
- return;
- }
BatchRemovalState state{};
- do {
+ while (HasChildren()) {
nsIContent* nodeToRemove = aOrder == BatchRemovalOrder::FrontToBack
? GetFirstChild()
: GetLastChild();
RemoveChildNode(nodeToRemove, aNotify, &state, nullptr,
MutationEffectOnScript::KeepTrustWorthiness);
state.mIsFirst = false;
- } while (HasChildren());
+ }
}
/**
