Bug 192461

Summary:

Remove children of ShadowRoot before deleting it

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

ASSIGNED

Severity:

Normal

CC:

cdumez, darin, dbates, esprehn+autocc, ews-watchlist, ggaren, kangil.han, mjs, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

192575

Bug Blocks:

Attachments:

Description	Flags
WIP	none
WIP - fixed non-debug builds	none
Cleanup	none
Archive of layout-test-results from ews201 for win-future	none
Updated for trunk	mjs: review-, ews-watchlist: commit-queue-
Archive of layout-test-results from ews202 for win-future	none

Ryosuke Niwa

Reported 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>

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2018-12-05 23:20:23 PST

Created attachment 356718 [details] WIP

Ryosuke Niwa

Comment 2 2018-12-06 13:01:26 PST

Created attachment 356742 [details] WIP - fixed non-debug builds

Ryosuke Niwa

Comment 3 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.

Ryosuke Niwa

Comment 4 2018-12-10 21:02:11 PST

Created attachment 357033 [details] Cleanup

EWS Watchlist

Comment 5 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

EWS Watchlist

Comment 6 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

Ryosuke Niwa

Comment 7 2019-01-14 13:17:55 PST

Created attachment 359073 [details] Updated for trunk

EWS Watchlist

Comment 8 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

EWS Watchlist

Comment 9 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

Maciej Stachowiak

Comment 10 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,

Note You need to log in before you can comment on or make changes to this bug.