Bug 220464 – Use HashSet<RefPtr<Node>> instead of HashSet<Node*>

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Use WeakHashSet<Node> instead of HashSet<Node*>

[Alex Christensen]

Created attachment 417271 [details]
Patch

[Alex Christensen]

Created attachment 417272 [details]
Patch

[Tim Horton]

Comment on attachment 417272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417272&action=review

> Source/WebCore/ChangeLog:7
> +

Needs more words, this isn't what a WebKit ChangeLog entry should look like

[Alex Christensen]

Comment on attachment 417272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417272&action=review

>> Source/WebCore/ChangeLog:7
>> +
> 
> Needs more words, this isn't what a WebKit ChangeLog entry should look like

Will add before landing:
WeakHashSet is resistent to use-after-free bugs, which can occur when storing raw pointers in a HashSet.

[Radar WebKit Bug Importer]

<rdar://problem/73252016>

[Geoffrey Garen]

RefPtr<Node> can be less expensive than WeakPtr<Node>, since the first WeakPtr for any Node needs to allocate the WeakPtrImpl.

Do we have a specific motivation to allow these nodes to be deleted? (I didn't see a case where we do.)

In any case where we do not specifically want to enable deletion, I think we should use RefPtr<Node> instead. That might be all of these cases.

[Alex Christensen]

Comment on attachment 417272 [details]
Patch

True.

[Alex Christensen]

Created attachment 428181 [details]
Patch

[Alex Christensen]

Created attachment 428185 [details]
Patch

[Chris Dumez]

Comment on attachment 428185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428185&action=review

> Source/WebCore/ChangeLog:3
> +        Use HashSet<RefPtr<Node>> instead of HashSet<Node*>

Why not a WeakHashSet<Node>? Container of ref pointers are often sources of leaks. This seems to be impacting lifetime management in a potentially significant way just to fix potential lifetime bugs.

[Alex Christensen]

That was my initial approach, but Geoff gave the feedback to use RefPtr/Ref instead.  I think it's ok in this case because the containers are basically all created, used, and destroyed within the scope of a function.

[Chris Dumez]

Comment on attachment 428185 [details]
Patch

Fair point. 2 cases seem to be static vectors though. However, they are apparently only used for debugging purposes.

[EWS]

Committed r277281 (237546@main): <https://commits.webkit.org/237546@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428185 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 225622

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #14)
> Re-opened since this is blocked by bug 225622

Link to test run showing crashes:
https://build.webkit.org/results/Apple-BigSur-Debug-WK2-Tests/r277293%20(1735)/results.html

All the ones I clicked appear to be
ASSERTION FAILED: isMainThread()
/Volumes/Data/worker/bigsur-debug/build/Source/WebCore/dom/Node.h(776) : void WebCore::Node::ref() const

[Alex Christensen]

Comment on attachment 428185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428185&action=review

> Source/WebCore/dom/MutationObserver.cpp:172
> +HashSet<Ref<Node>> MutationObserver::observedNodes() const

This is called from the GC thread.  I'll remove this change from this patch to be conservative.  We may want to revisit, though.  Pun intended.

[Alex Christensen]

Created attachment 428231 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 428231 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428231&action=review

> Source/WebCore/dom/Node.cpp:100
> -    static NeverDestroyed<HashSet<Node*>> liveNodes;
> +    static NeverDestroyed<HashSet<Ref<Node>>> liveNodes;

We can't do this. This will leak everything!
We only remove the node from this set inside ~Node.
So if we use Ref<Node> here, then it would never get deleted.
We need to use WeakHashSet instead.

> Source/WebCore/dom/Node.cpp:300
> -    static NeverDestroyed<HashSet<Node*>> ignore;
> +    static NeverDestroyed<HashSet<Ref<Node>>> ignore;

Same issue.

[Alex Christensen]

Created attachment 428305 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r277382