|Summary:||[JSC] Nodes in shadow DOM should not be GCed while their hosts are alive and they have JS properties|
|Product:||WebKit||Reporter:||Dominic Cooney <dominicc>|
|Severity:||Normal||CC:||abarth, aroben, dglazkov, eric, ggaren, sam, webkit.review.bot|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
|Bug Depends on:||59647, 61245|
|Bug Blocks:||59726, 59803, 59823|
Description Dominic Cooney 2011-04-26 18:42:23 PDT
The related bug for the same problem in V8 is bug 59284.
Comment 2 Geoffrey Garen 2011-04-27 11:58:36 PDT
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.
Comment 3 WebKit Review Bot 2011-04-27 14:18:57 PDT
http://trac.webkit.org/changeset/85082 might have broken Leopard Intel Debug (Build)
Comment 4 Sam Weinig 2011-04-27 15:25:59 PDT
Why is this right behavior. Why is keeping the shadow DOMs wrappers alive important or necessary? Is it detectable from a web page?
Comment 5 Dominic Cooney 2011-04-27 15:49:36 PDT
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.
Comment 6 Sam Weinig 2011-04-27 16:21:14 PDT
(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.
Comment 7 Dominic Cooney 2011-04-27 16:39:24 PDT
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.
Comment 8 Geoffrey Garen 2011-04-27 17:10:51 PDT
> 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.
Comment 11 Dominic Cooney 2011-04-27 19:01:54 PDT
Comment 12 Geoffrey Garen 2011-04-28 11:41:30 PDT
Comment on attachment 91405 [details] Patch Nice. r=me
Comment 13 Dimitri Glazkov (Google) 2011-04-28 11:44:03 PDT
Committed r85216: <http://trac.webkit.org/changeset/85216>
Comment 14 WebKit Review Bot 2011-04-28 12:45:38 PDT
http://trac.webkit.org/changeset/85216 might have broken GTK Linux 64-bit Debug
Comment 15 Adam Roben (:aroben) 2011-04-28 21:02:20 PDT
*** Bug 59708 has been marked as a duplicate of this bug. ***
Comment 16 Adam Roben (:aroben) 2011-04-28 21:05:08 PDT
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.
Comment 17 Dimitri Glazkov (Google) 2011-04-28 21:34:13 PDT
Thank you Adam! We should've been watching the bots.
Comment 18 Dominic Cooney 2011-04-28 21:34:33 PDT
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.
Comment 19 Dominic Cooney 2011-05-21 04:33:11 PDT
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.
Comment 20 Dominic Cooney 2011-05-22 16:57:57 PDT
I am closing this now since the fast/dom/shadow tests are running on Leopard without crashes now.