RESOLVED FIXED 102893
[Shadow DOM][V8] Assertion failure when shadow host is reclaimed before ShadowRoot 
https://bugs.webkit.org/show_bug.cgi?id=102893
Summary [Shadow DOM][V8] Assertion failure when shadow host is reclaimed before Shado...
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
DetailsFormatted DiffDiff
Patch (5.32 KB, patch)
2012-11-21 01:34 PST, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Patch (5.25 KB, patch)
2012-11-21 02:17 PST, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
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.
WebKit Review Bot
Comment 9 2012-11-21 16:45:39 PST
Comment on attachment 175385 [details] Patch Clearing flags on attachment: 175385 Committed r135456: <http://trac.webkit.org/changeset/135456>
WebKit Review Bot
Comment 10 2012-11-21 16:45:43 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.