Bug 34231 – XPathResult should keep its node set's JS wrappers alive

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Alexey Proskuryakov]

Created attachment 47571 [details]
test case

Test attached.

[Geoffrey Garen]

Created attachment 48173 [details]
test case 2

Here's another test case with event listeners.

[Julien Chaffraix]

Created attachment 87341 [details]
Proposed fix: Mark the node in the XPathResult nodeset for snapshot types

[Build Bot]

Attachment 87341 [details] did not build on win:
Build output: http://queues.webkit.org/results/8283360

[Julien Chaffraix]

Created attachment 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.

[Build Bot]

Attachment 87673 [details] did not build on win:
Build output: http://queues.webkit.org/results/8314151

[Julien Chaffraix]

Created attachment 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.

[Build Bot]

Attachment 87964 [details] did not build on win:
Build output: http://queues.webkit.org/results/8319376

[Alexey Proskuryakov]

Comment on attachment 87964 [details]
Re-Rebased proposed fix: Mark the node in the XPathResult nodeset for snapshot types

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 <[e-mail]>

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?

[Julien Chaffraix]

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

[Julien Chaffraix]

Created attachment 88825 [details]
Mark XPathResult nodeset JS wrapper - taken Alexey's comments into account

[Alexey Proskuryakov]

> 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();

[Julien Chaffraix]

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

[Julien Chaffraix]

Created attachment 88936 [details]
Same as previously with testing for the iterator type

[Alexey Proskuryakov]

Comment on attachment 88936 [details]
Same as previously with testing for the iterator type

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.

[WebKit Commit Bot]

Comment on attachment 88936 [details]
Same as previously with testing for the iterator type

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

[Julien Chaffraix]

Created attachment 91808 [details]
Patch

[Julien Chaffraix]

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!

[Eric Seidel (no email)]

Oliver understands the ways of the new marking as well.

[Geoffrey Garen]

Comment on attachment 91808 [details]
Patch

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(); }
};

[Julien Chaffraix]

> 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?

[Geoffrey Garen]

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

[Geoffrey Garen]

Comment on attachment 91808 [details]
Patch

Changing my r- to r+ because the optimization I suggested wouldn't work for all XPath result types.

[WebKit Commit Bot]

Comment on attachment 91808 [details]
Patch

Clearing flags on attachment: 91808

Committed r86234: <http://trac.webkit.org/changeset/86234>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.