Teach V8GCController how to traverse TemplateContentDocumentFragment::host

Teach V8GCController how to traverse TemplateContentDocumentFragment::host

This allows us to remove the [KeepAttributeAliveForGC] extended IDL attribute,
at the cost of a ridiculously-named new method on Node.

R=haraken@chromium.org
BUG=312518
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161282

[haraken, 2013-10-30 00:09:52]

https://codereview.chromium.org/48633006/diff/1/LayoutTests/fast/dom/HTMLTemp...
File LayoutTests/fast/dom/HTMLTemplateElement/custom-element-wrapper-gc.html
(right):

https://codereview.chromium.org/48633006/diff/1/LayoutTests/fast/dom/HTMLTemp...
LayoutTests/fast/dom/HTMLTemplateElement/custom-element-wrapper-gc.html:17:
gc();

You might also want to add a test for minorGC().

https://codereview.chromium.org/48633006/diff/1/Source/bindings/v8/V8GCContro...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/48633006/diff/1/Source/bindings/v8/V8GCContro...
Source/bindings/v8/V8GCController.cpp:193: node =
node->parentOrShadowHostNode();

Don't you need to change this to parentOrShadowHostOrTemplateHostNode() as well?
Otherwise, minor gc will probably mis-collect the template host node.

[adamk, 2013-10-30 17:02:44]

https://codereview.chromium.org/48633006/diff/1/Source/bindings/v8/V8GCContro...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/48633006/diff/1/Source/bindings/v8/V8GCContro...
Source/bindings/v8/V8GCController.cpp:193: node =
node->parentOrShadowHostNode();
On 2013/10/30 00:09:53, haraken wrote:
> 
> Don't you need to change this to parentOrShadowHostOrTemplateHostNode() as
well?
> Otherwise, minor gc will probably mis-collect the template host node.

Very good point, forgot about minor GC (I think there was no way to test it when
<template> first landed). Will add a test and fix the code.

[adamk, 2013-10-30 18:28:43]

Minor GC support added, PTAL. In particular, though my new test passes, I'm
wondering if I need to add support in traverseTree() for walking down into
template contents (I'm not clear on exactly how this works).

[haraken, 2013-10-31 00:35:08]

On 2013/10/30 18:28:43, adamk wrote:
> Minor GC support added, PTAL. In particular, though my new test passes, I'm
> wondering if I need to add support in traverseTree() for walking down into
> template contents (I'm not clear on exactly how this works).

Good point. I think we should. The point is that when GC traverses a parent
node, GC has to use parentOrShadowHostOrTemplateHostNode.

Another idea is that we can simply give up collecting the DOM tree if we find
any node that has a template host during a minor GC. (I'm not sure if this is a
good idea though.)

[adamk, 2013-10-31 15:50:56]

On 2013/10/31 00:35:08, haraken wrote:
> On 2013/10/30 18:28:43, adamk wrote:
> > Minor GC support added, PTAL. In particular, though my new test passes, I'm
> > wondering if I need to add support in traverseTree() for walking down into
> > template contents (I'm not clear on exactly how this works).
> 
> Good point. I think we should. The point is that when GC traverses a parent
> node, GC has to use parentOrShadowHostOrTemplateHostNode.
> 
> Another idea is that we can simply give up collecting the DOM tree if we find
> any node that has a template host during a minor GC. (I'm not sure if this is
a
> good idea though.)

I'm happy to add the code for walking down, but I'd like to understand how to
test it. It seems what I need to do is get a non-newspace node into the
<template> content...any hints for doing that would be appreciated.

[haraken, 2013-10-31 16:00:09]

On 2013/10/31 15:50:56, adamk wrote:
> On 2013/10/31 00:35:08, haraken wrote:
> > On 2013/10/30 18:28:43, adamk wrote:
> > > Minor GC support added, PTAL. In particular, though my new test passes,
I'm
> > > wondering if I need to add support in traverseTree() for walking down into
> > > template contents (I'm not clear on exactly how this works).
> > 
> > Good point. I think we should. The point is that when GC traverses a parent
> > node, GC has to use parentOrShadowHostOrTemplateHostNode.
> > 
> > Another idea is that we can simply give up collecting the DOM tree if we
find
> > any node that has a template host during a minor GC. (I'm not sure if this
is
> a
> > good idea though.)
> 
> I'm happy to add the code for walking down, but I'd like to understand how to
> test it. It seems what I need to do is get a non-newspace node into the
> <template> content...any hints for doing that would be appreciated.

It would be hard to test it deterministically, because the order in which V8
iterates DOM wrappers is not deterministic.

I think you can create several DOM trees that need some traversal to find a
template host node, and check if all DOM trees are kept alive correctly. It's
not a perfect way to test what we want to test, but it would be approximately
testing what we want to test.

[adamk, 2013-10-31 16:04:26]

On 2013/10/31 16:00:09, haraken wrote:
> On 2013/10/31 15:50:56, adamk wrote:
> > On 2013/10/31 00:35:08, haraken wrote:
> > > On 2013/10/30 18:28:43, adamk wrote:
> > > > Minor GC support added, PTAL. In particular, though my new test passes,
> I'm
> > > > wondering if I need to add support in traverseTree() for walking down
into
> > > > template contents (I'm not clear on exactly how this works).
> > > 
> > > Good point. I think we should. The point is that when GC traverses a
parent
> > > node, GC has to use parentOrShadowHostOrTemplateHostNode.
> > > 
> > > Another idea is that we can simply give up collecting the DOM tree if we
> find
> > > any node that has a template host during a minor GC. (I'm not sure if this
> is
> > a
> > > good idea though.)
> > 
> > I'm happy to add the code for walking down, but I'd like to understand how
to
> > test it. It seems what I need to do is get a non-newspace node into the
> > <template> content...any hints for doing that would be appreciated.
> 
> It would be hard to test it deterministically, because the order in which V8
> iterates DOM wrappers is not deterministic.
> 
> I think you can create several DOM trees that need some traversal to find a
> template host node, and check if all DOM trees are kept alive correctly. It's
> not a perfect way to test what we want to test, but it would be approximately
> testing what we want to test.

Here's my plan: fix the code and add a FIXME to unit-test the code in some way.
Writing a nondeterministic test doesn't seem particularly useful.

[haraken, 2013-10-31 16:05:57]

> > It would be hard to test it deterministically, because the order in which V8
> > iterates DOM wrappers is not deterministic.
> > 
> > I think you can create several DOM trees that need some traversal to find a
> > template host node, and check if all DOM trees are kept alive correctly.
It's
> > not a perfect way to test what we want to test, but it would be
approximately
> > testing what we want to test.
> 
> Here's my plan: fix the code and add a FIXME to unit-test the code in some
way.
> Writing a nondeterministic test doesn't seem particularly useful.

Sounds plausible. Fixing the bug should be the first thing to do.

[adamk, 2013-10-31 17:36:48]

Okay, added proper traversal to traverseTree().

[haraken, 2013-10-31 22:32:52]

LGTM. Thanks!

https://codereview.chromium.org/48633006/diff/200001/Source/bindings/v8/V8GCC...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/48633006/diff/200001/Source/bindings/v8/V8GCC...
Source/bindings/v8/V8GCController.cpp:184: if
(node->hasTagName(HTMLNames::templateTag)) {

Let's add a comment about this.

[commit-bot: I haz the power, 2013-11-04 19:14:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/48633006/250001

[adamk, 2013-11-04 19:15:40]

https://codereview.chromium.org/48633006/diff/200001/Source/bindings/v8/V8GCC...
File Source/bindings/v8/V8GCController.cpp (right):

https://codereview.chromium.org/48633006/diff/200001/Source/bindings/v8/V8GCC...
Source/bindings/v8/V8GCController.cpp:184: if
(node->hasTagName(HTMLNames::templateTag)) {
On 2013/10/31 22:32:53, haraken wrote:
> 
> Let's add a comment about this.

Done.

[commit-bot: I haz the power, 2013-11-04 21:10:26]

Change committed as 161282