Avoid creating JS wrapper on a removed node when the subtree is not observable

rniwa · rniwa · commit b1c3e6b9bc6d · 2021-02-05T00:10:30.000Z
https://bugs.webkit.org/show_bug.cgi?id=221243 <rdar://problem/73719386> Reviewed by Geoffrey Garen. Prior to this patch, WebKit forced the creation the JS wrapper on the root DOM node of a removed subtree to avoid script observable deletion of nodes. This is necessary because DOM nodes are reference counted in the C++ side, and while a DOM node keeps its child nodes alive, it won't keep its parent node alive to avoid reference cycles (leaks). If we didn't force the creation of the JS wrapper and the root node of the removed subtree didn't have any external reference to it in C++ side, we would happily delete it and all its descendant nodes above any subtree with a JS wrapper or an external C++ reference. While this turned out to be an effective strategy for implementing DOM nodes' GC semantics correctly, it has a significant runtime and memory cost - the latter is because we don't collect the JS wrappers of DOM nodes once they're created until they're ready to be destructed. This patch introduces a new optimization to avoid creating these JS wrappers when the removed subtree won't be observable by scripts in the future. The current heuristic is to check if the removed node has any external reference to it (i.e. refCount() > 0 excluding any reference counting that happens within our algorithm). This is sufficient because a given node should not be observable at a later time unless it has an external reference to it. This is ~0.5% progression on Speedometer-2.0 on MacBookAir7,2. To do this, we take advantage of the fact notifyChildNodeRemoved already traverses each removed subtree, and check if any of them has a reference count greater than 1 (greater than 1 because notifyChildNodeRemoved itself increments node's reference count before calling itself on child nodes). Note that we exclude the root node of the removed subtree as these JS wrapper creation is only needed to keep the root node alive. If the root node is already kept alive by some external reference to it, there is no need to keep it alive again by creating a JS wrapper on it. No new tests since existing tests such as fast/dom/gc-3.html covers it. * dom/ContainerNode.cpp: (WebCore::ContainerNode::removeAllChildrenWithScriptAssertion): Call willCreatePossiblyOrphanedTreeByRemoval if the removed subtree contains an observable node. (WebCore::ContainerNode::removeNodeWithScriptAssertion): Ditto. (WebCore::ContainerNode::removeSelfOrChildNodesForInsertion): Renamed from collectChildrenAndRemoveFromOldParent. Avoid collecting the removed nodes in this function in addition to removeAllChildrenWithScriptAssertion. (WebCore::ContainerNode::insertBefore): (WebCore::ContainerNode::replaceChild): (WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck): (WebCore::dispatchChildRemovalEvents): Don't call willCreatePossiblyOrphanedTreeByRemoval here. * dom/ContainerNode.h: * dom/ContainerNodeAlgorithms.cpp: (WebCore::observabilityOfRemovedNode): Added. Checks the observability of a node excluding the root node of the removed subtree since it needs a special case in removeAllChildrenWithScriptAssertion. As notifyNodeRemovedFromDocument and notifyNodeRemovedFromTree ref's each child node before recursing on itself, we check refCount() > 1 here. (WebCore::updateObservability): Added. A helper function to update RemovedSubtreeObservability. (WebCore::notifyNodeRemovedFromDocument): Now returns RemovedSubtreeObservability, (WebCore::notifyNodeRemovedFromTree): Ditto. (WebCore::notifyChildNodeRemoved): Ditto. * dom/ContainerNodeAlgorithms.h: Canonical link: https://commits.webkit.org/233719@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@272394 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,59 @@
+2021-02-04  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Avoid creating JS wrapper on a removed node when the subtree is not observable
+        https://bugs.webkit.org/show_bug.cgi?id=221243
+        <rdar://problem/73719386>
+
+        Reviewed by Geoffrey Garen.
+
+        Prior to this patch, WebKit forced the creation the JS wrapper on the root DOM node of a removed subtree
+        to avoid script observable deletion of nodes. This is necessary because DOM nodes are reference counted
+        in the C++ side, and while a DOM node keeps its child nodes alive, it won't keep its parent node alive
+        to avoid reference cycles (leaks). If we didn't force the creation of the JS wrapper and the root node
+        of the removed subtree didn't have any external reference to it in C++ side, we would happily delete it
+        and all its descendant nodes above any subtree with a JS wrapper or an external C++ reference.
+
+        While this turned out to be an effective strategy for implementing DOM nodes' GC semantics correctly,
+        it has a significant runtime and memory cost - the latter is because we don't collect the JS wrappers
+        of DOM nodes once they're created until they're ready to be destructed.
+
+        This patch introduces a new optimization to avoid creating these JS wrappers when the removed subtree
+        won't be observable by scripts in the future. The current heuristic is to check if the removed node
+        has any external reference to it (i.e. refCount() > 0 excluding any reference counting that happens within
+        our algorithm). This is sufficient because a given node should not be observable at a later time unless
+        it has an external reference to it. This is ~0.5% progression on Speedometer-2.0 on MacBookAir7,2.
+
+        To do this, we take advantage of the fact notifyChildNodeRemoved already traverses each removed subtree,
+        and check if any of them has a reference count greater than 1 (greater than 1 because notifyChildNodeRemoved
+        itself increments node's reference count before calling itself on child nodes). Note that we exclude the
+        root node of the removed subtree as these JS wrapper creation is only needed to keep the root node alive.
+        If the root node is already kept alive by some external reference to it, there is no need to keep it alive
+        again by creating a JS wrapper on it.
+
+        No new tests since existing tests such as fast/dom/gc-3.html covers it.
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::removeAllChildrenWithScriptAssertion): Call willCreatePossiblyOrphanedTreeByRemoval
+        if the removed subtree contains an observable node.
+        (WebCore::ContainerNode::removeNodeWithScriptAssertion): Ditto.
+        (WebCore::ContainerNode::removeSelfOrChildNodesForInsertion): Renamed from collectChildrenAndRemoveFromOldParent.
+        Avoid collecting the removed nodes in this function in addition to removeAllChildrenWithScriptAssertion.
+        (WebCore::ContainerNode::insertBefore):
+        (WebCore::ContainerNode::replaceChild):
+        (WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck):
+        (WebCore::dispatchChildRemovalEvents): Don't call willCreatePossiblyOrphanedTreeByRemoval here.
+        * dom/ContainerNode.h:
+        * dom/ContainerNodeAlgorithms.cpp:
+        (WebCore::observabilityOfRemovedNode): Added. Checks the observability of a node excluding the root node of
+        the removed subtree since it needs a special case in removeAllChildrenWithScriptAssertion.
+        As notifyNodeRemovedFromDocument and notifyNodeRemovedFromTree ref's each child node before recursing on itself,
+        we check refCount() > 1 here.
+        (WebCore::updateObservability): Added. A helper function to update RemovedSubtreeObservability.
+        (WebCore::notifyNodeRemovedFromDocument): Now returns RemovedSubtreeObservability,
+        (WebCore::notifyNodeRemovedFromTree): Ditto.
+        (WebCore::notifyChildNodeRemoved): Ditto.
+        * dom/ContainerNodeAlgorithms.h:
+
 2021-02-04  Chris Dumez  <cdumez@apple.com>
 
         RELEASE_ASSERT(bigInt) in VM constructor when constructing a WorkletGlobalScope
diff --git a/Source/WebCore/dom/ContainerNode.cpp b/Source/WebCore/dom/ContainerNode.cpp
@@ -111,7 +111,9 @@ ALWAYS_INLINE NodeVector ContainerNode::removeAllChildrenWithScriptAssertion(Chi
 
     while (RefPtr<Node> child = m_firstChild) {
         removeBetween(nullptr, child->nextSibling(), *child);
-        notifyChildNodeRemoved(*this, *child);
+        auto subtreeObservability = notifyChildNodeRemoved(*this, *child);
+        if (source == ChildChange::Source::API && subtreeObservability == RemovedSubtreeObservability::MaybeObservableByRefPtr)
+            willCreatePossiblyOrphanedTreeByRemoval(child.get());
     }
 
     if (deferChildrenChanged == DeferChildrenChanged::No)
@@ -149,6 +151,7 @@ ALWAYS_INLINE bool ContainerNode::removeNodeWithScriptAssertion(Node& childToRem
         return false;
 
     ChildChange change;
+    RemovedSubtreeObservability subtreeObservability;
     {
         WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
         ScriptDisallowedScope::InMainThread scriptDisallowedScope;
@@ -164,7 +167,7 @@ ALWAYS_INLINE bool ContainerNode::removeNodeWithScriptAssertion(Node& childToRem
         RefPtr<Node> previousSibling = childToRemove.previousSibling();
         RefPtr<Node> nextSibling = childToRemove.nextSibling();
         removeBetween(previousSibling.get(), nextSibling.get(), childToRemove);
-        notifyChildNodeRemoved(*this, childToRemove);
+        subtreeObservability = notifyChildNodeRemoved(*this, childToRemove);
 
         change.type = is<Element>(childToRemove) ?
             ChildChange::Type::ElementRemoved :
@@ -176,6 +179,9 @@ ALWAYS_INLINE bool ContainerNode::removeNodeWithScriptAssertion(Node& childToRem
         change.source = source;
     }
 
+    if (source == ChildChange::Source::API && subtreeObservability == RemovedSubtreeObservability::MaybeObservableByRefPtr)
+        willCreatePossiblyOrphanedTreeByRemoval(&childToRemove);
+
     // FIXME: Move childrenChanged into ScriptDisallowedScope block.
     childrenChanged(change);
 
@@ -224,18 +230,26 @@ static ALWAYS_INLINE void executeNodeInsertionWithScriptAssertion(ContainerNode&
         dispatchChildInsertionEvents(child);
 }
 
-static ExceptionOr<void> collectChildrenAndRemoveFromOldParent(Node& node, NodeVector& nodes)
+ExceptionOr<void> ContainerNode::removeSelfOrChildNodesForInsertion(Node& child, NodeVector& nodesForInsertion)
 {
-    if (!is<DocumentFragment>(node)) {
-        nodes.append(node);
-        auto* oldParent = node.parentNode();
+    if (!is<DocumentFragment>(child)) {
+        nodesForInsertion.append(child);
+        auto oldParent = makeRefPtr(child.parentNode());
         if (!oldParent)
             return { };
-        return oldParent->removeChild(node);
+        return oldParent->removeChild(child);
     }
 
-    nodes = collectChildNodes(node);
-    downcast<DocumentFragment>(node).removeChildren();
+    auto& fragment = downcast<DocumentFragment>(child);
+    if (!fragment.hasChildNodes())
+        return { };
+
+    auto removedChildNodes = fragment.removeAllChildrenWithScriptAssertion(ContainerNode::ChildChange::Source::API);
+    nodesForInsertion.swap(removedChildNodes);
+
+    fragment.rebuildSVGExtensionsElementsIfNecessary();
+    fragment.dispatchSubtreeModifiedEvent();
+
     return { };
 }
 
@@ -393,13 +407,13 @@ ExceptionOr<void> ContainerNode::insertBefore(Node& newChild, Node* refChild)
     Ref<Node> next(*refChild);
 
     NodeVector targets;
-    auto removeResult = collectChildrenAndRemoveFromOldParent(newChild, targets);
+    auto removeResult = removeSelfOrChildNodesForInsertion(newChild, targets);
     if (removeResult.hasException())
         return removeResult.releaseException();
     if (targets.isEmpty())
         return { };
 
-    // We need this extra check because collectChildrenAndRemoveFromOldParent() can fire mutation events.
+    // We need this extra check because removeSelfOrChildNodesForInsertion() can fire mutation events.
     for (auto& child : targets) {
         auto checkAcceptResult = checkAcceptChildGuaranteedNodeTypes(*this, child);
         if (checkAcceptResult.hasException())
@@ -508,12 +522,12 @@ ExceptionOr<void> ContainerNode::replaceChild(Node& newChild, Node& oldChild)
     NodeVector targets;
     {
         ChildListMutationScope mutation(*this);
-        auto collectResult = collectChildrenAndRemoveFromOldParent(newChild, targets);
+        auto collectResult = removeSelfOrChildNodesForInsertion(newChild, targets);
         if (collectResult.hasException())
             return collectResult.releaseException();
     }
 
-    // Do this one more time because collectChildrenAndRemoveFromOldParent() fires a MutationEvent.
+    // Do this one more time because removeSelfOrChildNodesForInsertion() fires a MutationEvent.
     for (auto& child : targets) {
         validityResult = checkPreReplacementValidity(*this, child, oldChild, ShouldValidateChildParent::No);
         if (validityResult.hasException())
@@ -707,14 +721,14 @@ ExceptionOr<void> ContainerNode::appendChildWithoutPreInsertionValidityCheck(Nod
     Ref<ContainerNode> protectedThis(*this);
 
     NodeVector targets;
-    auto removeResult = collectChildrenAndRemoveFromOldParent(newChild, targets);
+    auto removeResult = removeSelfOrChildNodesForInsertion(newChild, targets);
     if (removeResult.hasException())
         return removeResult.releaseException();
 
     if (targets.isEmpty())
         return { };
 
-    // We need this extra check because collectChildrenAndRemoveFromOldParent() can fire mutation events.
+    // We need this extra check because removeSelfOrChildNodesForInsertion() can fire mutation events.
     for (auto& child : targets) {
         auto nodeTypeResult = checkAcceptChildGuaranteedNodeTypes(*this, child);
         if (nodeTypeResult.hasException())
@@ -857,10 +871,6 @@ static void dispatchChildRemovalEvents(Ref<Node>& child)
     if (child->isInShadowTree())
         return;
 
-    // FIXME: This doesn't belong in dispatchChildRemovalEvents.
-    // FIXME: Nodes removed from a shadow tree should also be kept alive.
-    willCreatePossiblyOrphanedTreeByRemoval(child.ptr());
-
     Ref<Document> document = child->document();
 
     // dispatch pre-removal mutation events
diff --git a/Source/WebCore/dom/ContainerNode.h b/Source/WebCore/dom/ContainerNode.h
@@ -150,6 +150,7 @@ class ContainerNode : public Node {
     enum class DeferChildrenChanged { Yes, No };
     NodeVector removeAllChildrenWithScriptAssertion(ChildChange::Source, DeferChildrenChanged = DeferChildrenChanged::No);
     bool removeNodeWithScriptAssertion(Node&, ChildChange::Source);
+    ExceptionOr<void> removeSelfOrChildNodesForInsertion(Node&, NodeVector&);
 
     void removeBetween(Node* previousChild, Node* nextChild, Node& oldChild);
     ExceptionOr<void> appendChildWithoutPreInsertionValidityCheck(Node&);
diff --git a/Source/WebCore/dom/ContainerNodeAlgorithms.cpp b/Source/WebCore/dom/ContainerNodeAlgorithms.cpp
@@ -107,49 +107,66 @@ NodeVector notifyChildNodeInserted(ContainerNode& parentOfInsertedTree, Node& no
     return postInsertionNotificationTargets;
 }
 
-static void notifyNodeRemovedFromDocument(ContainerNode& oldParentOfRemovedTree, TreeScopeChange treeScopeChange, Node& node)
+inline RemovedSubtreeObservability observabilityOfRemovedNode(Node& node)
+{
+    bool isRootOfRemovedTree = !node.parentNode();
+    return node.refCount() > 1 && !isRootOfRemovedTree ? RemovedSubtreeObservability::MaybeObservableByRefPtr : RemovedSubtreeObservability::NotObservable;
+}
+
+inline void updateObservability(RemovedSubtreeObservability& currentObservability, RemovedSubtreeObservability newStatus)
+{
+    if (newStatus == RemovedSubtreeObservability::MaybeObservableByRefPtr)
+        currentObservability = newStatus;
+}
+
+static RemovedSubtreeObservability notifyNodeRemovedFromDocument(ContainerNode& oldParentOfRemovedTree, TreeScopeChange treeScopeChange, Node& node)
 {
     ASSERT(oldParentOfRemovedTree.isConnected());
     ASSERT(node.isConnected());
     node.removedFromAncestor(Node::RemovalType { /* disconnectedFromDocument */ true, treeScopeChange == TreeScopeChange::Changed }, oldParentOfRemovedTree);
 
+    auto observability = observabilityOfRemovedNode(node);
     if (!is<ContainerNode>(node))
-        return;
+        return observability;
 
     for (RefPtr<Node> child = downcast<ContainerNode>(node).firstChild(); child; child = child->nextSibling()) {
         RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!node.isConnected() && child->parentNode() == &node);
-        notifyNodeRemovedFromDocument(oldParentOfRemovedTree, treeScopeChange, *child.get());
+        updateObservability(observability, notifyNodeRemovedFromDocument(oldParentOfRemovedTree, treeScopeChange, *child.get()));
     }
 
     if (!is<Element>(node))
-        return;
+        return observability;
 
     if (RefPtr<ShadowRoot> root = downcast<Element>(node).shadowRoot()) {
         RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!node.isConnected() && root->host() == &node);
-        notifyNodeRemovedFromDocument(oldParentOfRemovedTree, TreeScopeChange::DidNotChange, *root.get());
+        updateObservability(observability, notifyNodeRemovedFromDocument(oldParentOfRemovedTree, TreeScopeChange::DidNotChange, *root.get()));
     }
+    return observability;
 }
 
-static void notifyNodeRemovedFromTree(ContainerNode& oldParentOfRemovedTree, TreeScopeChange treeScopeChange, Node& node)
+static RemovedSubtreeObservability notifyNodeRemovedFromTree(ContainerNode& oldParentOfRemovedTree, TreeScopeChange treeScopeChange, Node& node)
 {
     ASSERT(!oldParentOfRemovedTree.isConnected());
 
     node.removedFromAncestor(Node::RemovalType { /* disconnectedFromDocument */ false, treeScopeChange == TreeScopeChange::Changed }, oldParentOfRemovedTree);
 
+    auto observability = observabilityOfRemovedNode(node);
     if (!is<ContainerNode>(node))
-        return;
+        return observability;
 
     for (RefPtr<Node> child = downcast<ContainerNode>(node).firstChild(); child; child = child->nextSibling())
-        notifyNodeRemovedFromTree(oldParentOfRemovedTree, treeScopeChange, *child);
+        updateObservability(observability, notifyNodeRemovedFromTree(oldParentOfRemovedTree, treeScopeChange, *child));
 
     if (!is<Element>(node))
-        return;
+        return observability;
 
     if (RefPtr<ShadowRoot> root = downcast<Element>(node).shadowRoot())
-        notifyNodeRemovedFromTree(oldParentOfRemovedTree, TreeScopeChange::DidNotChange, *root);
+        updateObservability(observability, notifyNodeRemovedFromTree(oldParentOfRemovedTree, TreeScopeChange::DidNotChange, *root));
+
+    return observability;
 }
 
-void notifyChildNodeRemoved(ContainerNode& oldParentOfRemovedTree, Node& child)
+RemovedSubtreeObservability notifyChildNodeRemoved(ContainerNode& oldParentOfRemovedTree, Node& child)
 {
     // Assert that the caller of this function has an instance of ScriptDisallowedScope.
     ASSERT(!isMainThread() || ScriptDisallowedScope::InMainThread::hasDisallowedScope());
@@ -158,9 +175,8 @@ void notifyChildNodeRemoved(ContainerNode& oldParentOfRemovedTree, Node& child)
     // Tree scope has changed if the container node from which "node" is removed is in a document or a shadow root.
     auto treeScopeChange = oldParentOfRemovedTree.isInTreeScope() ? TreeScopeChange::Changed : TreeScopeChange::DidNotChange;
     if (child.isConnected())
-        notifyNodeRemovedFromDocument(oldParentOfRemovedTree, treeScopeChange, child);
-    else
-        notifyNodeRemovedFromTree(oldParentOfRemovedTree, treeScopeChange, child);
+        return notifyNodeRemovedFromDocument(oldParentOfRemovedTree, treeScopeChange, child);
+    return notifyNodeRemovedFromTree(oldParentOfRemovedTree, treeScopeChange, child);
 }
 
 void addChildNodesToDeletionQueue(Node*& head, Node*& tail, ContainerNode& container)
diff --git a/Source/WebCore/dom/ContainerNodeAlgorithms.h b/Source/WebCore/dom/ContainerNodeAlgorithms.h
@@ -62,7 +62,11 @@ class ContainerChildRemovalScope {
 #endif // not ASSERT_ENABLED
 
 NodeVector notifyChildNodeInserted(ContainerNode& parentOfInsertedTree, Node&);
-void notifyChildNodeRemoved(ContainerNode& oldParentOfRemovedTree, Node&);
+enum class RemovedSubtreeObservability {
+    NotObservable,
+    MaybeObservableByRefPtr,
+};
+RemovedSubtreeObservability notifyChildNodeRemoved(ContainerNode& oldParentOfRemovedTree, Node&);
 void removeDetachedChildrenInContainer(ContainerNode&);
 
 enum SubframeDisconnectPolicy {
