Bug 59571 – [JSC] Nodes in shadow DOM should not be GCed while their hosts are alive and they have JS properties

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dominic Cooney]

The related bug for the same problem in V8 is bug 59284.

[Dominic Cooney]

Created attachment 91208 [details]
Patch

[Geoffrey Garen]

Comment on attachment 91208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91208&action=review

It looks like your patch doesn't include any new test results. If we're going to make this change, I think we need a test with results that vary depending on whether this change is in the tree.

> LayoutTests/fast/dom/shadow/gc-shadow.html:19
> +    // Some ports (unfortunately) take GCController.collect as advisory.
> +    for (var i = 0; i < 10000; i++)
> +        var s = new String(i);

Which ports take GCController.collect as advisory?

If there is a port like that, it's broken. I don't think we should make this test 2-3X slower on all ports just because some ports are broken. If window.GCController is defined, we should call collect() and return early.

[WebKit Review Bot]

http://trac.webkit.org/changeset/85082 might have broken Leopard Intel Debug (Build)

[Sam Weinig]

Why is this right behavior. Why is keeping the shadow DOMs wrappers alive important or necessary? Is it detectable from a web page?

[Dominic Cooney]

re: Geoffrey’s r-

My choice of words—“advisory”—was not right. I’ve done some debugging and the test was erroneously passing on Mac because as written, the shadow root wrapper was still alive in the machine thread roots.

Peturbing the test causes it drop out of the root set, for example replacing this line:

GCController.collect();

with this:

if (window.GCController)
    GCController.collect();

is enough to make the shadow root wrapper disappear from machineThreadRoots.

What’s your advice here? Don’t allocate the objects (a given), use this if test, and check in failures for Mac? What do you recommend as a robust way to make the thread roots reflect the state of variables in the script?

re: Sam’s comment—why are we doing this

Shadow roots aren’t exposed to script. But at some point down the road they should be, as part of the component model. This change is some incremental progress towards that goal. Making the change now makes sense because it means we can keep pushing on component model plumbing in layout tests, without exposing any component model API prematurely.

[Sam Weinig]

(In reply to comment #5)
> re: Sam’s comment—why are we doing this
> 
> Shadow roots aren’t exposed to script. But at some point down the road they should be, as part of the component model. This change is some incremental progress towards that goal. Making the change now makes sense because it means we can keep pushing on component model plumbing in layout tests, without exposing any component model API prematurely.

Can you give me an example (e.g. imagine we support XBL2) of what this would look like?  In general, we don't want to keep wrappers alive longer than necessary.

[Dominic Cooney]

Ah! This doesn’t prevent the wrapper being collected if it has no properties/event listeners (isn’t "observable.") I have updated the bug title to reflect this. The patch just extends the notion of "connected" from parentNode to parentNode and shadowHost.

As for what this looks like in the API—we don’t have a specific proposal yet. But we expect scenarios such as: the script "inside" the component boundary adds event listeners to nodes in shadow DOM defined by the component; etc. The XBL2 analogy is adding event listeners or properties to elements in shadowTree.

[Geoffrey Garen]

> What do you recommend as a robust way to make the thread roots reflect the state of variables in the script?

Our typical solution is to test a few different elements in a loop. So, put 10 'p' elements in an array, ensureShadowRoot and tatoo each of them in a loop, gc, and then test all 10 shadow roots for the tatoo in a loop. Since the same code tatoos each element, it's extremely likely -- pretty much guaranteed -- to overwrite old references with new references, leaving at least some of the tested elements completely unreferenced.

So, to clarify what I think is needed here:

1. Early return after calling GCController.collect if GCController exists.

2. A looping test that would fail on Mac without your change, but passes with your change.

[Dominic Cooney]

Created attachment 91403 [details]
Patch

[Dominic Cooney]

Created attachment 91405 [details]
Patch

[Dominic Cooney]

Neat suggestion—the patch on bug 59647 implements the loop.

I have updated this patch to base if off the patch on bug 59647.

[Geoffrey Garen]

Comment on attachment 91405 [details]
Patch

Nice. r=me

[Dimitri Glazkov (Google)]

Committed r85216: <http://trac.webkit.org/changeset/85216>

[WebKit Review Bot]

http://trac.webkit.org/changeset/85216 might have broken GTK Linux 64-bit Debug

[Adam Roben (:aroben)]

*** Bug 59708 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

There are still crashes happening on the bots: <http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r85290%20(29645)/results.html>.

I skipped all of fast/dom/shadow in r85298.

[Dimitri Glazkov (Google)]

Thank you Adam! We should've been watching the bots.

[Dominic Cooney]

Thank you for skipping; I will debug. I have not been able to repro these crashes on Snow Leopard debug but I will keep trying.

[Dominic Cooney]

I can reliably reproduce this crash on Leopard Debug (but *only* on Leopard). The culprit is
fast/dom/shadow/layout-tests-can-access-shadow.html running this and another test (another of the same or fast/dom/shadow/no-renderers-for-light-children.html) crashes with this callstack.

/me continues to investigate.

[Dominic Cooney]

I am closing this now since the fast/dom/shadow tests are running on Leopard without crashes now.