| Summary: | Use HashSet<RefPtr<Node>> instead of HashSet<Node*> | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, benjamin, cdumez, cfleizach, cmarcelo, commit-queue, dmazzoni, esprehn+autocc, ews-watchlist, ggaren, hi, jcraig, jdiggs, joepeck, kangil.han, mifenton, rniwa, samuel_white, thorton, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | 225622 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Alex Christensen
2021-01-08 09:42:13 PST
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. 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
|