Fix Document leak from NodeFilter.

Fix Document leak from NodeFilter.

* Problem Description
When NodeIterator/TreeWalker with filter JS callback is created, the following reference chain was created:
NodeIterator -(RefPtr)-> NodeFilter -(RefPtr)-> V8NodeFilterCondition -(ScopedPersistent)-> JS callback object -> window

This caused the whole document to be leaked when NodeIterator was referenced from window. 
For example, the following script created a circular reference which could not be collected.
<script> window.foobar = document.createNodeIterator(document, NodeFilter.SHOW_ELEMENT, function(node) { return NodeFilter.FILTER_ACCEPT; }); </script>

* Proposal
This patch modifies the reference chain to avoid leak. The basic idea is to move the callback's whole reference chain to the V8 side.

We change the strong reference to the JS callback object held by V8NodeFilterCondition to a weak reference.
The JS callback is instead kept alive by a wrapper of NodeFilter, referenced from NodeIterator wrapper.

The new reference chain is illustrated as follows:
Blink world:    NodeIterator      -(RefPtr)->        NodeFilter       -(RefPtr)->     V8NodeFilterCondition
                     ^^^                                 ^^^                             vvv(weakref)vvv
   V8 world: NodeIterator wrap -(HiddenProperty)-> NodeFilter wrap -(HiddenProperty)->   JS callback obj. -> window

The new reference chain can be collected correctly, as the whole circular reference chain is visible from V8 GC.

BUG=None

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155425

[kouhei (in TOK), 2013-07-31 05:57:56]

The second leak detected from auto GC leak finder.

Reproducing LayoutTest testcase to be added.

[abarth-chromium, 2013-07-31 06:04:14]

Oh, I'm so glad you're fixing this leak.  It's been on my list for a long time!

You should add a test using the new internals.observeGC function.

https://codereview.chromium.org/21274004/diff/1/Source/bindings/v8/V8Binding.cpp
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/21274004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.cpp:151: v8::Handle<v8::Object> filterWrapper =
toV8(PassRefPtr<NodeFilter>(filter), v8::Handle<v8::Object>(),
isolate).As<v8::Object>();
PassRefPtr<NodeFilter>(filter)  ->  filter.get()
^^ That should work.

https://codereview.chromium.org/21274004/diff/1/Source/core/dom/NodeIterator.idl
File Source/core/dom/NodeIterator.idl (right):

https://codereview.chromium.org/21274004/diff/1/Source/core/dom/NodeIterator....
Source/core/dom/NodeIterator.idl:25: [KeepAttributeAliveForGC] readonly
attribute NodeFilter filter;
Why is this attribute needed?  Storing the filter on the wrapper should be
sufficient...

[kouhei (in TOK), 2013-07-31 06:21:34]

Thank you for your comments!

> Oh, I'm so glad you're fixing this leak.  It's been on my list for a long
time!
I'm currently trying to create tools for automatically detecting and help fixing
GC leaks. If you keep a list of known GC leaks, I would love to see them.

> You should add a test using the new internals.observeGC function.
Yes. I'm writing one using internals.observeGC. It is a fantastic tool!

https://codereview.chromium.org/21274004/diff/1/Source/bindings/v8/V8Binding.cpp
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/21274004/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.cpp:151: v8::Handle<v8::Object> filterWrapper =
toV8(PassRefPtr<NodeFilter>(filter), v8::Handle<v8::Object>(),
isolate).As<v8::Object>();
On 2013/07/31 06:04:14, abarth wrote:
> PassRefPtr<NodeFilter>(filter)  ->  filter.get()
> ^^ That should work.

Done.

https://codereview.chromium.org/21274004/diff/1/Source/core/dom/NodeIterator.idl
File Source/core/dom/NodeIterator.idl (right):

https://codereview.chromium.org/21274004/diff/1/Source/core/dom/NodeIterator....
Source/core/dom/NodeIterator.idl:25: [KeepAttributeAliveForGC] readonly
attribute NodeFilter filter;
On 2013/07/31 06:04:14, abarth wrote:
> Why is this attribute needed?  Storing the filter on the wrapper should be
> sufficient...

I'm new to the bindings world so it's very likely I'm just getting this wrong.

The JS callback is used from NodeIterator whenever nextNode()/previousNode() is
called, whether or not attribute filter was referenced from JS.

My understanding is that a new NodeFilter wrapper is created/disposed all the
time when "iterator.filter" is called on JS side. As the NodeFilter wrapper is
where the reference to the JS callback is held, I thought we needed to keep the
NodeFilter's wrapper in NodeIterator's wrapper so it won't be disposed.

[abarth-chromium, 2013-07-31 06:28:45]

I just look for ScriptValue/ScriptObject/ScopedPersistent being stored in DOM
objects.  Any time we do that (and we don't make it weak), we've got a leak.  I
don't have a good list.

You're probably right about the KeepAttributeAliveForGC attribute.  I haven't
studied the details of that issue.

[haraken, 2013-07-31 06:46:12]

Thanks for working on this. I'm very happy you're detecting and fixing *actual*
leaks one after another!

https://codereview.chromium.org/21274004/diff/1/Source/core/dom/NodeIterator.idl
File Source/core/dom/NodeIterator.idl (right):

https://codereview.chromium.org/21274004/diff/1/Source/core/dom/NodeIterator....
Source/core/dom/NodeIterator.idl:25: [KeepAttributeAliveForGC] readonly
attribute NodeFilter filter;

I think it's a right thing to keep 'filter' alive as long as NodeIterator's
wrapper is alive.

However, maybe you don't need [KeepAttributeAliveForGC], because by default, a
hidden reference is added from NodeIterator's wrapper to readonly attributes
whose type is not Node. (Sorry, the condition under which a hidden reference is
added is extremely complicated. See ShouldKeepAttributeAlive() in the code
generator.)

Would you check that the generated code doesn't change if you remove
[KeepAttributeAliveForGC] ? If it doesn't change, you don't need
[KeepAttributeAliveForGC].

[dominicc (has gone to gerrit), 2013-07-31 14:09:55]

DBC inline

I look forward to seeing the test. I don't understand what is going on with
fast/dom/constants.html in this patch.

https://codereview.chromium.org/21274004/diff/1/Source/bindings/v8/V8NodeFilt...
File Source/bindings/v8/V8NodeFilterCondition.cpp (right):

https://codereview.chromium.org/21274004/diff/1/Source/bindings/v8/V8NodeFilt...
Source/bindings/v8/V8NodeFilterCondition.cpp:62: if (!filter->IsObject())
Is it worth asserting that this is non-empty?

[kouhei (in TOK), 2013-08-01 01:12:24]

Thank you for comments!
I refactored the patch. It should be ready for review now.

haraken: r?

https://codereview.chromium.org/21274004/diff/1/Source/bindings/v8/V8NodeFilt...
File Source/bindings/v8/V8NodeFilterCondition.cpp (right):

https://codereview.chromium.org/21274004/diff/1/Source/bindings/v8/V8NodeFilt...
Source/bindings/v8/V8NodeFilterCondition.cpp:62: if (!filter->IsObject())
On 2013/07/31 14:09:55, dominicc wrote:
> Is it worth asserting that this is non-empty?

Done.

[haraken, 2013-08-01 03:44:14]

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
File LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html
(right):

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html:30:
shouldBeTrue("callbackObservation.wasCollected", "The callback should be
collected when nodeIterator is collected");

I don't fully understand how internals.observeGC works. Why can 'callback' be
collected here, even though we're still in the scope where the 'callback' is
defined (i.e. 'var callback = ...' is still alive here)?

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
File LayoutTests/fast/dom/NodeIterator/NodeIterator-leak-document.html (right):

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-leak-document.html:45:
frame.src='data:text/html;charset=utf-8,<script>keep =
document.createNodeIterator(document, NodeFilter.SHOW_ELEMENT, function(node) {
return NodeFilter.SHOW_ELEMENT; }, false);</script'+'>';

Instead of adding one test file for one leak you detected, you can prepare
fast/dom/leak-document.html and put all tests into the file.

https://codereview.chromium.org/21274004/diff/21001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8DocumentCustom.cpp (right):

https://codereview.chromium.org/21274004/diff/21001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8DocumentCustom.cpp:126: PassRefPtr<NodeFilter>
toNodeFilterWithWrapper(v8::Handle<v8::Object>* filterWrapper,
v8::Handle<v8::Value> callback, v8::Handle<v8::Object> creationContext,
v8::Isolate* isolate)

I'd really want to avoid writing custom bindings here. Instead, we might want to
seek a way to auto-generate the code we need.

In my understanding, the point of the custom binding is that we add a hidden
reference from NodeIterator's wrapper to NodeFilter's wrapper. Maybe can we use
[GenerateIsReachable=owner] to add the reference (and thus avoid custom
bindings)? By using [GenerateIsReachable=owner] on interface X, you can
explicitly say that there is a reference from X::owner()'s wrapper to X's
wrapper. (See DOMImplementation.idl and HTMLCollection.idl for more detailss)

[dominicc (has gone to gerrit), 2013-08-01 04:51:12]

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
File LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html
(right):

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html:18:
callback = null;
For style, I would put this line right after creating the callback observation.

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html:20:
shouldBeTrue("!nodeFilterObservation.wasCollected", "The NodeFilter wrapper
should not be collected while nodeIterator is alive");
shouldBeTrue("! -> shouldBeFalse("

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html:20:
shouldBeTrue("!nodeFilterObservation.wasCollected", "The NodeFilter wrapper
should not be collected while nodeIterator is alive");
Use single quotes for JavaScript string literals. It all works out much nicer
when you have markup (with double quoted attributes) in JavaScript, or
JavaScript in attributes.

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html:28:
shouldBeTrue("nodeIteratorObservation.wasCollected", "The nodeIterator should be
collected.");
I think shouldBeTrue just takes one arg. If you were cribbing from a test of
mine which passes two--sorry--I need to go back and fix that.

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html:30:
shouldBeTrue("callbackObservation.wasCollected", "The callback should be
collected when nodeIterator is collected");
On 2013/08/01 03:44:15, haraken wrote:
> 
> I don't fully understand how internals.observeGC works. Why can 'callback' be
> collected here, even though we're still in the scope where the 'callback' is
> defined (i.e. 'var callback = ...' is still alive here)?

Because line 18 sets it to null.

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
File LayoutTests/fast/dom/NodeIterator/NodeIterator-leak-document.html (right):

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-leak-document.html:45:
frame.src='data:text/html;charset=utf-8,<script>keep =
document.createNodeIterator(document, NodeFilter.SHOW_ELEMENT, function(node) {
return NodeFilter.SHOW_ELEMENT; }, false);</script'+'>';
Duplicating this is a shame, can you just put a document in resources and src=
to that?

[kouhei (in TOK), 2013-08-01 05:09:53]

Thank you for your comments! Pushed some styling fixes...

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
File LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html
(right):

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html:18:
callback = null;
On 2013/08/01 04:51:12, dominicc wrote:
> For style, I would put this line right after creating the callback
observation.

Done.

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html:18:
callback = null;
On 2013/08/01 04:51:12, dominicc wrote:
> For style, I would put this line right after creating the callback
observation.

Done.

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html:20:
shouldBeTrue("!nodeFilterObservation.wasCollected", "The NodeFilter wrapper
should not be collected while nodeIterator is alive");
On 2013/08/01 04:51:12, dominicc wrote:
> shouldBeTrue("! -> shouldBeFalse("

Done.

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-dont-overcollect.html:28:
shouldBeTrue("nodeIteratorObservation.wasCollected", "The nodeIterator should be
collected.");
On 2013/08/01 04:51:12, dominicc wrote:
> I think shouldBeTrue just takes one arg. If you were cribbing from a test of
> mine which passes two--sorry--I need to go back and fix that.

Done.

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
File LayoutTests/fast/dom/NodeIterator/NodeIterator-leak-document.html (right):

https://codereview.chromium.org/21274004/diff/21001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeIterator/NodeIterator-leak-document.html:45:
frame.src='data:text/html;charset=utf-8,<script>keep =
document.createNodeIterator(document, NodeFilter.SHOW_ELEMENT, function(node) {
return NodeFilter.SHOW_ELEMENT; }, false);</script'+'>';
On 2013/08/01 03:44:15, haraken wrote:
> 
> Instead of adding one test file for one leak you detected, you can prepare
> fast/dom/leak-document.html and put all tests into the file.

Done. Unified to fast/dom/leak-document.html

https://codereview.chromium.org/21274004/diff/21001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8DocumentCustom.cpp (right):

https://codereview.chromium.org/21274004/diff/21001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8DocumentCustom.cpp:126: PassRefPtr<NodeFilter>
toNodeFilterWithWrapper(v8::Handle<v8::Object>* filterWrapper,
v8::Handle<v8::Value> callback, v8::Handle<v8::Object> creationContext,
v8::Isolate* isolate)
On 2013/08/01 03:44:15, haraken wrote:
> 
> I'd really want to avoid writing custom bindings here. Instead, we might want
to
> seek a way to auto-generate the code we need.
> 
> In my understanding, the point of the custom binding is that we add a hidden
> reference from NodeIterator's wrapper to NodeFilter's wrapper. Maybe can we
use
> [GenerateIsReachable=owner] to add the reference (and thus avoid custom
> bindings)? By using [GenerateIsReachable=owner] on interface X, you can
> explicitly say that there is a reference from X::owner()'s wrapper to X's
> wrapper. (See DOMImplementation.idl and HTMLCollection.idl for more detailss)

I had offline discussion with haraken. The conclusion is to modify the binding
generator so that equivalent code can be generated.

Neither [GenerateIsReachable=owner] nor [CustomOpaqueRoot] are likely to work
here, as they rely on Node* to work. Modifying V8GCController.cpp was also
considered, but it doesn't feel right to modify them for every case like this.
The most general solution to this problem is probably to modify the binding
generator.

I will come back on this CL after the binding generator is modified to support
this kind of wrapper reference.

[kouhei (in TOK), 2013-08-01 08:33:33]

> I had offline discussion with haraken. The conclusion is to modify the binding
> generator so that equivalent code can be generated.
> 
> Neither [GenerateIsReachable=owner] nor [CustomOpaqueRoot] are likely to work
> here, as they rely on Node* to work. Modifying V8GCController.cpp was also
> considered, but it doesn't feel right to modify them for every case like this.
> The most general solution to this problem is probably to modify the binding
> generator.
> 
> I will come back on this CL after the binding generator is modified to support
> this kind of wrapper reference.
Tried CustomToV8. I think now it is simple enough to merge this version first,
and work on auto-generating it later.

[haraken, 2013-08-01 13:32:19]

The approach looks good!

https://codereview.chromium.org/21274004/diff/40001/LayoutTests/fast/dom/Node...
File LayoutTests/fast/dom/NodeFilterCondition-leak-document.html (right):

https://codereview.chromium.org/21274004/diff/40001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeFilterCondition-leak-document.html:49: frame.src = src;

I'm sorry I was misreading the tests in your previous patch. You're testing
multiple things in this file, which are not good. In particular, this is a test
for GC bugs, so we want to minimize the test case.

Shall we split the tests into 3 files?

- The test file for setPosition().
- The test file for createNodeIterator().
- The test file for createTreeWalker().

Needless to say, you don't need to run setPosition() in the latter two tests.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.cpp:151: v8::Handle<v8::Object> filterWrapper =
toV8(PassRefPtr<NodeFilter>(filter), v8::Handle<v8::Object>(),
isolate).As<v8::Object>();

Do you need PassRefPtr<NodeFilter> ?

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/V8Node...
File Source/bindings/v8/V8NodeFilterCondition.h (right):

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/V8Node...
Source/bindings/v8/V8NodeFilterCondition.h:44: class V8NodeFilterCondition :
public NodeFilterCondition {

Would you add a comment about the relationship between NodeFilter, NodeIterator
and NodeFilterCondition?

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/V8Node...
Source/bindings/v8/V8NodeFilterCondition.h:56: explicit
V8NodeFilterCondition(v8::Handle<v8::Value> filter, v8::Handle<v8::Object>
owner);

Nit: explicit isn't needed.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8DocumentCustom.cpp (right):

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8DocumentCustom.cpp:52: #include
"bindings/v8/V8NodeFilterCondition.h"

These #includes will be no longer necessary.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8NodeIteratorCustom.cpp (right):

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:1: #include "config.h"

Please add a copyright.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:4: #include "V8NodeFilter.h"

This is not needed.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:7: #include
"bindings/v8/V8Utilities.h"

This wouldn't be needed. Please be careful not to include unnecessary files. The
same comment for V8TreeWalkerCustom.h.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:8: #include "core/dom/Node.h"

Ditto.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:9: #include
"core/dom/NodeIterator.h"

Ditto.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:11: #include "wtf/RefPtr.h"

Ditto.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8TreeWalkerCustom.cpp (right):

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8TreeWalkerCustom.cpp:1: #include "config.h"

Please add a copyright.

[kouhei (in TOK), 2013-08-02 03:26:57]

Thank you for comments!

Updated patch. I introduced fast/dom/resources/leak-check.js to avoid code
duplication.

haraken: r?

https://codereview.chromium.org/21274004/diff/40001/LayoutTests/fast/dom/Node...
File LayoutTests/fast/dom/NodeFilterCondition-leak-document.html (right):

https://codereview.chromium.org/21274004/diff/40001/LayoutTests/fast/dom/Node...
LayoutTests/fast/dom/NodeFilterCondition-leak-document.html:49: frame.src = src;
On 2013/08/01 13:32:20, haraken wrote:
> 
> I'm sorry I was misreading the tests in your previous patch. You're testing
> multiple things in this file, which are not good. In particular, this is a
test
> for GC bugs, so we want to minimize the test case.
> 
> Shall we split the tests into 3 files?
> 
> - The test file for setPosition().
> - The test file for createNodeIterator().
> - The test file for createTreeWalker().
> 
> Needless to say, you don't need to run setPosition() in the latter two tests.

Done.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.cpp:151: v8::Handle<v8::Object> filterWrapper =
toV8(PassRefPtr<NodeFilter>(filter), v8::Handle<v8::Object>(),
isolate).As<v8::Object>();
On 2013/08/01 13:32:20, haraken wrote:
> 
> Do you need PassRefPtr<NodeFilter> ?

Done. Changed to .get().

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/V8Node...
File Source/bindings/v8/V8NodeFilterCondition.h (right):

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/V8Node...
Source/bindings/v8/V8NodeFilterCondition.h:44: class V8NodeFilterCondition :
public NodeFilterCondition {
On 2013/08/01 13:32:20, haraken wrote:
> 
> Would you add a comment about the relationship between NodeFilter,
NodeIterator
> and NodeFilterCondition?

Done.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/V8Node...
Source/bindings/v8/V8NodeFilterCondition.h:56: explicit
V8NodeFilterCondition(v8::Handle<v8::Value> filter, v8::Handle<v8::Object>
owner);
On 2013/08/01 13:32:20, haraken wrote:
> 
> Nit: explicit isn't needed.

Done.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8DocumentCustom.cpp (right):

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8DocumentCustom.cpp:52: #include
"bindings/v8/V8NodeFilterCondition.h"
On 2013/08/01 13:32:20, haraken wrote:
> 
> These #includes will be no longer necessary.

Done.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8NodeIteratorCustom.cpp (right):

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:1: #include "config.h"
On 2013/08/01 13:32:20, haraken wrote:
> 
> Please add a copyright.

Done.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:4: #include "V8NodeFilter.h"
On 2013/08/01 13:32:20, haraken wrote:
> 
> This is not needed.
Won't compile without this file.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:7: #include
"bindings/v8/V8Utilities.h"
On 2013/08/01 13:32:20, haraken wrote:
> 
> This wouldn't be needed. Please be careful not to include unnecessary files.
The
> same comment for V8TreeWalkerCustom.h.

Done.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:8: #include "core/dom/Node.h"
On 2013/08/01 13:32:20, haraken wrote:
> 
> Ditto.

Won't compile without this file.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:9: #include
"core/dom/NodeIterator.h"
On 2013/08/01 13:32:20, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:11: #include "wtf/RefPtr.h"
On 2013/08/01 13:32:20, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8NodeIteratorCustom.cpp:11: #include "wtf/RefPtr.h"
On 2013/08/01 13:32:20, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8TreeWalkerCustom.cpp (right):

https://codereview.chromium.org/21274004/diff/40001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8TreeWalkerCustom.cpp:1: #include "config.h"
On 2013/08/01 13:32:20, haraken wrote:
> 
> Please add a copyright.

Done.

[haraken, 2013-08-02 03:47:59]

LGTM. Great patch.

https://codereview.chromium.org/21274004/diff/50001/LayoutTests/fast/dom/reso...
File LayoutTests/fast/dom/resources/leak-check.js (right):

https://codereview.chromium.org/21274004/diff/50001/LayoutTests/fast/dom/reso...
LayoutTests/fast/dom/resources/leak-check.js:1: // include
../../js/resources/js-test-pre.js before this file.

../../js/resources/js-test-pre.js => fast/js/resources/js-test-pre.js

https://codereview.chromium.org/21274004/diff/50001/LayoutTests/fast/dom/reso...
LayoutTests/fast/dom/resources/leak-check.js:39: console.log("This test only
runs on DumpRenderTree, as it requires existence of window.internals and
cross-domain resource access check disabled.");

Nit: We prefer debug("...") instead of console.log().

https://codereview.chromium.org/21274004/diff/50001/LayoutTests/fast/dom/reso...
LayoutTests/fast/dom/resources/leak-check.js:62: return
'data:text/html;charset=utf-8,'+html;

Nit: Spaces are needed around '+'.

https://codereview.chromium.org/21274004/diff/50001/LayoutTests/fast/dom/reso...
LayoutTests/fast/dom/resources/leak-check.js:69: // include
../../js/resources/js-test-post.js after this file.

../../js/resources/js-test-post.js => fast/js/resources/js-test-post.js

https://codereview.chromium.org/21274004/diff/50001/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/21274004/diff/50001/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.cpp:151: v8::Handle<v8::Object> filterWrapper =
toV8(filter.get(), v8::Handle<v8::Object>(), isolate).As<v8::Object>();

filter.get() => filter

We want to use toV8(PassRefPtr<X>) instead of toV8(X*) as much as possible.
toV8(X*) is just a fast path that is used in limited places.

https://codereview.chromium.org/21274004/diff/50001/Source/bindings/v8/V8Node...
File Source/bindings/v8/V8NodeFilterCondition.h (right):

https://codereview.chromium.org/21274004/diff/50001/Source/bindings/v8/V8Node...
Source/bindings/v8/V8NodeFilterCondition.h:52: // let V8 GC handle collection of
|m_filter|.

Instead of words, it might be better to illustrate it in a diagram:

(DOM)
NodeIterator  ----RefPtr----> NodeFilter ----RefPtr----> NodeFilterCondition
  |   ^                        |   ^                        |
 weak |                       weak |                ScopedPersistent(weak)
  | RefPtr                     | RefPtr                     |
  v   |                        v   |                        v
NodeIterator  --HiddenValue--> NodeFilter --HiddenValue--> JS Callback
(JS)

[kouhei (in TOK), 2013-08-02 04:04:25]

Thanks for your comments! Updated patch.

https://codereview.chromium.org/21274004/diff/50001/LayoutTests/fast/dom/reso...
File LayoutTests/fast/dom/resources/leak-check.js (right):

https://codereview.chromium.org/21274004/diff/50001/LayoutTests/fast/dom/reso...
LayoutTests/fast/dom/resources/leak-check.js:1: // include
../../js/resources/js-test-pre.js before this file.
On 2013/08/02 03:47:59, haraken wrote:
> 
> ../../js/resources/js-test-pre.js => fast/js/resources/js-test-pre.js

Done.

https://codereview.chromium.org/21274004/diff/50001/LayoutTests/fast/dom/reso...
LayoutTests/fast/dom/resources/leak-check.js:39: console.log("This test only
runs on DumpRenderTree, as it requires existence of window.internals and
cross-domain resource access check disabled.");
On 2013/08/02 03:47:59, haraken wrote:
> 
> Nit: We prefer debug("...") instead of console.log().

Done.

https://codereview.chromium.org/21274004/diff/50001/LayoutTests/fast/dom/reso...
LayoutTests/fast/dom/resources/leak-check.js:62: return
'data:text/html;charset=utf-8,'+html;
On 2013/08/02 03:47:59, haraken wrote:
> 
> Nit: Spaces are needed around '+'.

Done.

https://codereview.chromium.org/21274004/diff/50001/LayoutTests/fast/dom/reso...
LayoutTests/fast/dom/resources/leak-check.js:69: // include
../../js/resources/js-test-post.js after this file.
On 2013/08/02 03:47:59, haraken wrote:
> 
> ../../js/resources/js-test-post.js => fast/js/resources/js-test-post.js

Done.

https://codereview.chromium.org/21274004/diff/50001/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/21274004/diff/50001/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.cpp:151: v8::Handle<v8::Object> filterWrapper =
toV8(filter.get(), v8::Handle<v8::Object>(), isolate).As<v8::Object>();
On 2013/08/02 03:47:59, haraken wrote:
> 
> filter.get() => filter
> 
> We want to use toV8(PassRefPtr<X>) instead of toV8(X*) as much as possible.
> toV8(X*) is just a fast path that is used in limited places.

WontFix.
template<typename U> PassRefPtr(const RefPtr<U>& ptr) is defined in PassRefPtr.h
so compiler can't decide which toV8 to use unless the ".get()".

I will post a separate CL which tries to fix this issue.

[commit-bot: I haz the power, 2013-08-02 04:05:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/21274004/58001

[commit-bot: I haz the power, 2013-08-02 06:06:51]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[kouhei (in TOK), 2013-08-02 07:09:18]

On 2013/08/02 06:06:51, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

Suppressed printing numberOfLiveDocuments when test passed. It seems
numberOfLiveDocuments at the start of test may vary. My guess is that the test
run previously on the same batch are leaking.

[commit-bot: I haz the power, 2013-08-02 07:10:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/21274004/74001

[commit-bot: I haz the power, 2013-08-02 09:44:41]

Change committed as 155425