Bug 119718 – Add leak tests for NodeIterator and TreeWalker

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Fix the problem https://chromium.googlesource.com/chromium/blink/+/65db79784ecd4a7e744d10a26befe5159638a56c fixed in WebKit if applicable.

* 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.

[Ahmad Saleem]

rniwa - This seems to reference v8 and related bindings, I am not sure, it would be simple merge of this patch. Plus do we require this now or Webkit code is now diverged and this is already fixed? Thanks!

[Alexey Proskuryakov]

The action here is to either analyze our code to see if it has a similar problem, or to try the test case to see if it reproduces the problem.

The latter won't be trivial, as the test cannot be run in Safari, it relies on window.internals. If we are lucky, it will just work with run-webkit-tests, but it may need adjustment.

[Ryosuke Niwa]

I've read our code, and as far as I can tell we don't have the same bug.

[Ryosuke Niwa]

Modified tests pass in WebKit as well. Let's check in these tests.

[Ryosuke Niwa]

Pull request: https://github.com/WebKit/WebKit/pull/4473

[EWS]

Committed 254711@main (b17ccd4f3712): <https://commits.webkit.org/254711@main>

Reviewed commits have been landed. Closing PR #4473 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/100206367>