Bug 192461 - Remove children of ShadowRoot before deleting it
Summary: Remove children of ShadowRoot before deleting it
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on: 192575
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-05 23:19 PST by Ryosuke Niwa
Modified: 2022-02-09 10:14 PST (History)
9 users (show)

See Also:


Attachments
WIP (1.91 KB, patch)
2018-12-05 23:20 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP - fixed non-debug builds (1.94 KB, patch)
2018-12-06 13:01 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Cleanup (13.37 KB, patch)
2018-12-10 21:02 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.79 MB, application/zip)
2018-12-10 22:59 PST, EWS Watchlist
no flags Details
Updated for trunk (12.00 KB, patch)
2019-01-14 13:17 PST, Ryosuke Niwa
mjs: review-
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (13.46 MB, application/zip)
2019-01-14 15:55 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-12-05 23:19:26 PST
Right now, ~ShadowRoot deletes its children but we should be deleting the children first
so that none of the descendant nodes would try to access ShadowRoot during its destruction.

<rdar://problem/44703658>
Comment 1 Ryosuke Niwa 2018-12-05 23:20:23 PST
Created attachment 356718 [details]
WIP
Comment 2 Ryosuke Niwa 2018-12-06 13:01:26 PST
Created attachment 356742 [details]
WIP - fixed non-debug builds
Comment 3 Ryosuke Niwa 2018-12-06 14:57:37 PST
Hm... this isn't quite going to solve the problem I set out to solve. It's still possible for someone to invoke ref() on ShadowRoot after m_refCount had been decremented to 0. For that matter, that could happen to Document as well inside Document::removedLastRef().

I'm actually not certain asserting that we've never reached removedLastRef() makes sense. What's the issue if someone ref's an object with m_refCount of 0 at that point? It seems to me, that might just work.

Conceptually, what we want to do is to remove all children right before the last ref goes away but deref() -> removedLastRef() might be that timing really since we can't predict when the last deref() would be called sort of adding a function like willDeref(), which will be a massive perf hit.
Comment 4 Ryosuke Niwa 2018-12-10 21:02:11 PST
Created attachment 357033 [details]
Cleanup
Comment 5 EWS Watchlist 2018-12-10 22:58:55 PST
Comment on attachment 357033 [details]
Cleanup

Attachment 357033 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10347736

New failing tests:
fast/frames/lots-of-objects.html
Comment 6 EWS Watchlist 2018-12-10 22:59:07 PST
Created attachment 357036 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 7 Ryosuke Niwa 2019-01-14 13:17:55 PST
Created attachment 359073 [details]
Updated for trunk
Comment 8 EWS Watchlist 2019-01-14 15:54:54 PST
Comment on attachment 359073 [details]
Updated for trunk

Attachment 359073 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10749425

New failing tests:
fast/frames/lots-of-objects.html
fast/frames/lots-of-iframes.html
Comment 9 EWS Watchlist 2019-01-14 15:55:06 PST
Created attachment 359089 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 10 Maciej Stachowiak 2020-05-30 20:18:17 PDT
Comment on attachment 359073 [details]
Updated for trunk

This patch no longer applies. (Seems like a good idea otherwise,