Bug 102893

Summary:

[Shadow DOM][V8] Assertion failure when shadow host is reclaimed before ShadowRoot

Product:

WebKit

Reporter:

Hajime Morrita <morrita>

Component:

DOM

Assignee:

Hajime Morrita <morrita>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

haraken, ojan, webcomponents-bugzilla, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Hajime Morrita

Reported 2012-11-21 00:38:16 PST

Repro: <!DOCTYPE html> <html> <body> <script> if (window.testRunner) window.testRunner.dumpAsText(); function makeOrphanShadow() { var host = document.createElement("div"); var shadow = new WebKitShadowRoot(host); return shadow; }; var shadow = makeOrphanShadow(); gc(true); shadow.innerHTML = "Hello"; console.log(shadow); </script> </body> </html> ----- This is another consequence of Bug 88834. Haraken told me that the bug will be fixed eventually so I'll make a short-term workaround.

Attachments
Patch (5.33 KB, patch) 2012-11-21 01:10 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (5.32 KB, patch) 2012-11-21 01:34 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (5.25 KB, patch) 2012-11-21 02:17 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Hajime Morrita

Comment 1 2012-11-21 01:10:25 PST

Created attachment 175367 [details] Patch

Kentaro Hara

Comment 2 2012-11-21 01:18:54 PST

Comment on attachment 175367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175367&action=review I discussed with morrita offline. The change looks reasonable as a temporary fix. The real fix will be done in bug 88834 shortly. > LayoutTests/fast/dom/shadow/host-wrapper-reclaimed.html:4 > +<script> You can use js-test-pre.js > LayoutTests/fast/dom/shadow/host-wrapper-reclaimed.html:16 > +if (window.gc) > + gc(); Then you can just use gc(). If window.gc is not exposed, it falls back to another code. js-test-pre.js cares that. > LayoutTests/fast/dom/shadow/host-wrapper-reclaimed.html:17 > +try { shadow.innerHTML = "Hello"; } catch(e) { } Let's print something in the catch block. Otherwise, we miss INVALID_ACCESS_ERR that might be thrown by your change. > LayoutTests/fast/dom/shadow/host-wrapper-reclaimed.html:18 > +shadow.appendChild(document.createElement("span")); Is it needed? (Would you explain where this test can crash without your change?)

Hajime Morrita

Comment 3 2012-11-21 01:34:36 PST

Created attachment 175375 [details] Patch

Hajime Morrita

Comment 4 2012-11-21 01:38:54 PST

Comment on attachment 175367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175367&action=review Hi haraken, thanks for the quick review! I updated the patch. >> LayoutTests/fast/dom/shadow/host-wrapper-reclaimed.html:4 >> +<script> > > You can use js-test-pre.js I tried but then the crash is mysteriously disappeared... I'd keep current way. >> LayoutTests/fast/dom/shadow/host-wrapper-reclaimed.html:16 >> + gc(); > > Then you can just use gc(). If window.gc is not exposed, it falls back to another code. js-test-pre.js cares that. I noticed that this test needs gc() anyway. So I removed the if clause. >> LayoutTests/fast/dom/shadow/host-wrapper-reclaimed.html:17 >> +try { shadow.innerHTML = "Hello"; } catch(e) { } > > Let's print something in the catch block. Otherwise, we miss INVALID_ACCESS_ERR that might be thrown by your change. I don't want to cover that part by this test since We won't do this once Bug 88834 is fixed. It is not an exact expectation (which test should cover) but an artifact of current workaround. >> LayoutTests/fast/dom/shadow/host-wrapper-reclaimed.html:18 >> +shadow.appendChild(document.createElement("span")); > > Is it needed? (Would you explain where this test can crash without your change?) This triggers ShadowRoot::childrenChanged() where owner() is used.

Build Bot

Comment 5 2012-11-21 01:56:14 PST

Comment on attachment 175367 [details] Patch Attachment 175367 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14909937

Build Bot

Comment 6 2012-11-21 02:05:39 PST

Comment on attachment 175375 [details] Patch Attachment 175375 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14910952

Hajime Morrita

Comment 7 2012-11-21 02:17:51 PST

Created attachment 175385 [details] Patch

Kentaro Hara

Comment 8 2012-11-21 16:28:45 PST

Comment on attachment 175385 [details] Patch Looks ok.