Use WeakHashSet<Node> instead of HashSet<Node*>
Created attachment 417271 [details] Patch
Created attachment 417272 [details] Patch
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
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.
<rdar://problem/73252016>
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.
Comment on attachment 417272 [details] Patch True.
Created attachment 428181 [details] Patch
Created attachment 428185 [details] Patch
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.
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.
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.
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].
Re-opened since this is blocked by bug 225622
(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
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.
Created attachment 428231 [details] Patch
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.
Created attachment 428305 [details] Patch
http://trac.webkit.org/r277382