Bug 34231 - XPathResult should keep its node set's JS wrappers alive
: XPathResult should keep its node set's JS wrappers alive
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-01-27 16:10 PST by
Modified: 2012-08-23 17:18 PST (History)


Attachments
test case (617 bytes, text/html)
2010-01-27 16:10 PST, Alexey Proskuryakov
no flags Details
test case 2 (1.46 KB, text/html)
2010-02-04 15:10 PST, Geoffrey Garen
no flags Details
Proposed fix: Mark the node in the XPathResult nodeset for snapshot types (19.17 KB, patch)
2011-03-29 09:07 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
Rebased proposed fix: Mark the node in the XPathResult nodeset for snapshot types (17.88 KB, patch)
2011-03-30 23:56 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
Re-Rebased proposed fix: Mark the node in the XPathResult nodeset for snapshot types (18.04 KB, patch)
2011-04-01 22:03 PST, Julien Chaffraix
ap: review-
ap: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Mark XPathResult nodeset JS wrapper - taken Alexey's comments into account (22.14 KB, patch)
2011-04-08 08:17 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
Same as previously with testing for the iterator type (24.67 KB, patch)
2011-04-09 17:11 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
Patch (25.53 KB, patch)
2011-04-30 16:33 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-01-27 16:10:27 PST
Created an attachment (id=47571) [details]
test case

Test attached.
------- Comment #1 From 2010-02-04 15:10:12 PST -------
Created an attachment (id=48173) [details]
test case 2

Here's another test case with event listeners.
------- Comment #2 From 2011-03-29 09:07:41 PST -------
Created an attachment (id=87341) [details]
Proposed fix: Mark the node in the XPathResult nodeset for snapshot types
------- Comment #3 From 2011-03-29 10:02:08 PST -------
Attachment 87341 [details] did not build on win:
Build output: http://queues.webkit.org/results/8283360
------- Comment #4 From 2011-03-30 23:56:59 PST -------
Created an attachment (id=87673) [details]
Rebased proposed fix: Mark the node in the XPathResult nodeset for snapshot types 

The previous patch did not apply properly on most EWS. Rebased the patch to get proper coverage on all our build system.
------- Comment #5 From 2011-03-31 00:15:07 PST -------
Attachment 87673 [details] did not build on win:
Build output: http://queues.webkit.org/results/8314151
------- Comment #6 From 2011-04-01 22:03:59 PST -------
Created an attachment (id=87964) [details]
Re-Rebased proposed fix: Mark the node in the XPathResult nodeset for snapshot types

Reworked the windows part, let's hope it works this time.
------- Comment #7 From 2011-04-01 23:02:54 PST -------
Attachment 87964 [details] did not build on win:
Build output: http://queues.webkit.org/results/8319376
------- Comment #8 From 2011-04-02 21:50:52 PST -------
(From update of attachment 87964 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=87964&action=review

R- because this breaks the build, and also I don't think that it's worth making the fix for snapshot results only. But looks mostly good!

> LayoutTests/ChangeLog:12
> +        Test that an event listener registered on an XPATH result snapshots' node does
> +        not crash. This case was already fixed prior to this change. However it is better
> +        to keep it as a regression test.

Geoff's test crashes with an assertion in debug builds of ToT. I think that this hasn't changed since it was attached.

> Source/WebCore/ChangeLog:15
> +        * Android.jscbindings.mk:
> +        Added the new file to our build systems.

ChangeLog doesn't mention most changed project files.

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:63100
> +						ExcludedFromBuild="true"

Since this file is excluded from build, it should be added to JSBindingsAllInOne.cpp. This is why the build failed.

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:2
> + * Copyright (C) 2010 Julien Chaffraix <jchaffraix@webkit.org>

2011

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:3
> + * All right reserved.

Why did you split the copyright line in two?

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:43
> +    // Only mark the snapshots' node.

I don't understand what this comment is saying.

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:44
> +    if (impl()->resultType() == XPathResult::UNORDERED_NODE_SNAPSHOT_TYPE || impl()->resultType() == XPathResult::ORDERED_NODE_SNAPSHOT_TYPE) {

What about other types based on NodeSet? There are two iterator types, and two single-node types, and it seems that they are affected, too.

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:47
> +        ExceptionCode ignoredException = 0;
> +        unsigned long snapshotLength = impl()->snapshotLength(ignoredException);
> +        ASSERT_UNUSED(ignoredException, !ignoredException);

I would suggest adding necessary functions to XPathResult to avoid the runtime cost of doing the checks repeatedly, and also to avoid the ugly code that ignores exceptions.

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:54
> +    // FIXME: We need to mark the result of ANY_UNORDERED_NODE_TYPE and FIRST_ORDERED_NODE_TYPE.

Ah yes. Why not do that right now?
------- Comment #9 From 2011-04-08 07:50:29 PST -------
> Geoff's test crashes with an assertion in debug builds of ToT. I think that this hasn't changed since it was attached.

I haven't seen that on ToT. I am using a Debug build of Qt.

> > Source/WebCore/WebCore.vcproj/WebCore.vcproj:63100
> > +						ExcludedFromBuild="true"
> 
> Since this file is excluded from build, it should be added to JSBindingsAllInOne.cpp. This is why the build failed.

Ah, thanks! That explains the build failure.

> > Source/WebCore/bindings/js/JSXPathResultCustom.cpp:3
> > + * All right reserved.
> 
> Why did you split the copyright line in two?

Because it inelegantly extended on the line past the rest of the license. I double checked and the "All right reserved." is optional so I just removed it.

> > Source/WebCore/bindings/js/JSXPathResultCustom.cpp:44
> > +    if (impl()->resultType() == XPathResult::UNORDERED_NODE_SNAPSHOT_TYPE || impl()->resultType() == XPathResult::ORDERED_NODE_SNAPSHOT_TYPE) {
> 
> What about other types based on NodeSet? There are two iterator types, and two single-node types, and it seems that they are affected, too.

This is mostly true. The iterators are not affected as any change in the DOM would invalidate them. Thus there is no way for us to reach a node removed from the DOM using an iterator.

> I would suggest adding necessary functions to XPathResult to avoid the runtime cost of doing the checks repeatedly, and also to avoid the ugly code that ignores exceptions.

Fine enough. I did not want to expose more of XPathResult but I am not too set on that.

> > Source/WebCore/bindings/js/JSXPathResultCustom.cpp:54
> > +    // FIXME: We need to mark the result of ANY_UNORDERED_NODE_TYPE and FIRST_ORDERED_NODE_TYPE.
> 
> Ah yes. Why not do that right now?

I wanted to keep the change small but it is not too hard to add that too.

The other comments will be taken of.
------- Comment #10 From 2011-04-08 08:17:19 PST -------
Created an attachment (id=88825) [details]
Mark XPathResult nodeset JS wrapper - taken Alexey's comments into account
------- Comment #11 From 2011-04-08 09:01:14 PST -------
> The iterators are not affected as any change in the DOM would invalidate them. Thus there is no way for us to reach a node removed from the DOM using an iterator.

I'm not quite convinced yet. What if XPath results are in a frameless document that becomes unreferenced? E.g.

var doc = (new DOMParser).parse(...);
doc.getElementById(...).foo = "bar";
var result = doc.evaluate(...);
doc = 0;
gc();
------- Comment #12 From 2011-04-09 12:53:11 PST -------
(In reply to comment #11)
> > The iterators are not affected as any change in the DOM would invalidate them. Thus there is no way for us to reach a node removed from the DOM using an iterator.
> 
> I'm not quite convinced yet. What if XPath results are in a frameless document that becomes unreferenced? E.g.

Indeed, I overlooked frameless document. I will update the patch with a test for that to cover that iterators are fixed too.
------- Comment #13 From 2011-04-09 17:11:46 PST -------
Created an attachment (id=88936) [details]
Same as previously with testing for the iterator type
------- Comment #14 From 2011-04-10 10:16:33 PST -------
(From update of attachment 88936 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=88936&action=review

r=me. You may need to land this manually, since commit queue usually fails on changes to vcproj files. But we'll see.

> LayoutTests/ChangeLog:12
> +        Test that an event listener registered on an XPATH result snapshots' node does
> +        not crash. This case was already fixed prior to this change. However it is better
> +        to keep it as a regression test.

Since I'm still seeing this test crashing with an assertion test, I'm not a fan of this comment.

> LayoutTests/ChangeLog:16
> +        Test that an XPATH snapshot result keeps its node wrapper alive.

XPath.

> LayoutTests/ChangeLog:20
> +        Test that an XPATH iterator result keeps its node wrapper alive.

XPath.

> LayoutTests/ChangeLog:24
> +        Test that other XPATH result that holds nodes keeps their wrapper alive.

XPath.
------- Comment #15 From 2011-04-10 10:18:38 PST -------
(From update of attachment 88936 [details])
Rejecting attachment 88936 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
ct.pbxproj
Hunk #2 succeeded at 9701 (offset 1 line).
Hunk #3 succeeded at 18169 (offset 3 lines).
Hunk #4 succeeded at 24767 (offset 3 lines).
patching file Source/WebCore/bindings/js/JSBindingsAllInOne.cpp
patching file Source/WebCore/bindings/js/JSXPathResultCustom.cpp
patching file Source/WebCore/xml/XPathResult.h
patching file Source/WebCore/xml/XPathResult.idl

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Alexey Proskuryakov', ..." exit_code: 1

Full output: http://queues.webkit.org/results/8377665
------- Comment #16 From 2011-04-30 16:33:07 PST -------
Created an attachment (id=91808) [details]
Patch
------- Comment #17 From 2011-04-30 16:36:12 PST -------
The patch is updated after JSC's GC changes, Geoffrey could you check that this is the right way to mark the nodes now? Thanks!
------- Comment #18 From 2011-05-01 09:48:09 PST -------
Oliver understands the ways of the new marking as well.
------- Comment #19 From 2011-05-04 15:45:13 PST -------
(From update of attachment 91808 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=91808&action=review

The tests here are great, and I think this patch would work as-is, but I think it's worth it to implement the optimization I suggested above.

> Source/WebCore/bindings/js/JSXPathResultCustom.cpp:49
> +    const XPath::Value& xpathValue = impl()->value();
> +    if (xpathValue.isNodeSet()) {
> +        const XPath::NodeSet& nodesToMark = xpathValue.toNodeSet();
> +        for (size_t i = 0; i < nodesToMark.size(); ++i) {
> +            Node* node = nodesToMark[i];
> +            visitor.addOpaqueRoot(root(node));
> +        }

I think you can make this more efficient.

All nodes in an XPathResult are in the same document, which is XPathResult::m_document, and iteration is only allowed so long as the document has not changed. Therefore, you can just do this:

visitor.addOpaqueRoot(impl()->document());

and this:

class XPathResult {
...
    Document* document() { return m_document.get(); }
};
------- Comment #20 From 2011-05-06 08:03:54 PST -------
> The tests here are great, and I think this patch would work as-is, but I think it's worth it to implement the optimization I suggested above.

Thanks for the review Geoffrey.

> All nodes in an XPathResult are in the same document, which is XPathResult::m_document, and iteration is only allowed so long as the document has not changed.

You are right about the iteration. However there are other types of XPathResult - namely snapshot and node type - that allow to access the underlying Nodes even if the DOM changed.

> Therefore, you can just do this:

I tried your solution and unfortunately it will not do. We can modify the DOM and still access the results, thus we need to mark each Nodes individually as they may have been removed from the DOM but still need to be kept alive. Let me know what you think?
------- Comment #21 From 2011-05-06 17:10:23 PST -------
> > Therefore, you can just do this:
> 
> I tried your solution and unfortunately it will not do. We can modify the DOM and still access the results, thus we need to mark each Nodes individually as they may have been removed from the DOM but still need to be kept alive. Let me know what you think?

Ah, I see my ignorance of XPath is showing through :(. Thanks for looking into this.
------- Comment #22 From 2011-05-06 17:11:01 PST -------
(From update of attachment 91808 [details])
Changing my r- to r+ because the optimization I suggested wouldn't work for all XPath result types.
------- Comment #23 From 2011-05-11 08:24:44 PST -------
(From update of attachment 91808 [details])
Clearing flags on attachment: 91808

Committed r86234: <http://trac.webkit.org/changeset/86234>
------- Comment #24 From 2011-05-11 08:24:51 PST -------
All reviewed patches have been landed.  Closing bug.