Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: a7a2be07da735d966b1903215333f03deee1a1f9
https://github.com/WebKit/WebKit/commit/a7a2be07da735d966b1903215333f03deee1a1f9
Author: Geoffrey Garen <gga...@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
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
