[webkit-changes] [WebKit/WebKit] a7a2be: Node refcounting should prevent ref()'s that escap...

[geoffreygaren]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: a7a2be07da735d966b1903215333f03deee1a1f9
      https://github.com/WebKit/WebKit/commit/a7a2be07da735d966b1903215333f03deee1a1f9
  Author: Geoffrey Garen <ggaren at apple.com>
  Date:   2025-01-30 (Thu, 30 Jan 2025)

  Changed paths:
    M Source/WebCore/dom/Document.cpp
    M Source/WebCore/dom/Document.h
    M Source/WebCore/dom/Node.cpp
    M Source/WebCore/dom/Node.h

  Log Message:
  -----------
  Node refcounting should prevent ref()'s that escape the destructor [ Part 2 ]
https://bugs.webkit.org/show_bug.cgi?id=286700
rdar://143698424

Reviewed by Chris Dumez.

https://commits.webkit.org/287208@main added checking for ref()'s that escape
the Node destructor.

The model I implemented was a bit relaxed for an edge case in Document
destruction. It a accepted a final refcount of either 0 or 1 (but not more).

This patch makes it strict. Now Node destruction requires a final refcount of
exactly 1.

(Node::deref maintains a final overlooking refcount of 1 to prevent re-entering
our destructor, and also to check whether our destructor adds any more ref's
that escape.)

* Source/WebCore/dom/Document.cpp:
(WebCore::Document::~Document): Since m_referencingNodeCount is akin to a refcount,
also assert that no m_referencingNodeCount escapes destruction.

We can use 0 here because our 1 refcount already prevents re-entering our
destructor.

(WebCore::Document::removedLastRef): No need to incrementReferencingNodeCount()
to protect 'this' inside this function. Our 1 refcount already protects us.

When we resurrect a document after its last ref has been removed (because it
still has referencing nodes), instead of resetting its refcount to 0, drop
its overlooking 1 refcount. This allows new ref's that were added during
removedLastRef to escape, which is consistent with the decision to resurrect.

* Source/WebCore/dom/Document.h:
(WebCore::Document::decrementReferencingNodeCount): Because removedLastRef
dropped our overlooking 1 refcount, restore it here before destruction.

Also added an ASSERT for underflow.

* Source/WebCore/dom/Node.cpp:
(WebCore::Node::Node): Do not inc / dec the referencing node count on the
Document itself. The Document never intends to reference itself (that would be
a leak), and this only worked mechanically for weird reasons, and now it
triggers my new ASSERT, so stop doing that.

(WebCore::Node::~Node): Check for exactly 1. This is the point of this patch.

(WebCore::Node::removedLastRef): Changed an ASSERT to RELEASE_ASSERT because
it's related to other RELEASE_ASSERTs and on a slow path.

* Source/WebCore/dom/Node.h:
(WebCore::Node::deref const): Changed an ASSERT to ASSERT_WITH_SECURITY_IMPLICATION
to help fuzzing look for mistakes in this patch.

Canonical link: https://commits.webkit.org/289569@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications