Bug 102893 - [Shadow DOM][V8] Assertion failure when shadow host is reclaimed before ShadowRoot
Summary: [Shadow DOM][V8] Assertion failure when shadow host is reclaimed before Shado...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-21 00:38 PST by Hajime Morrita
Modified: 2012-11-21 16:45 PST (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 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.
Comment 1 Hajime Morrita 2012-11-21 01:10:25 PST
Created attachment 175367 [details]
Patch
Comment 2 Kentaro Hara 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?)
Comment 3 Hajime Morrita 2012-11-21 01:34:36 PST
Created attachment 175375 [details]
Patch
Comment 4 Hajime Morrita 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.
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Hajime Morrita 2012-11-21 02:17:51 PST
Created attachment 175385 [details]
Patch
Comment 8 Kentaro Hara 2012-11-21 16:28:45 PST
Comment on attachment 175385 [details]
Patch

Looks ok.
Comment 9 WebKit Review Bot 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>
Comment 10 WebKit Review Bot 2012-11-21 16:45:43 PST
All reviewed patches have been landed.  Closing bug.