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
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Julien Chaffraix
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-27 16:10 PST by Alexey Proskuryakov
Modified: 2012-08-23 17:18 PDT (History)
7 users (show)

See Also:


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 PDT, Julien Chaffraix
no flags 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 PDT, Julien Chaffraix
no flags 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 PDT, Julien Chaffraix
ap: review-
ap: commit‑queue-
Details | Formatted Diff | Diff
Mark XPathResult nodeset JS wrapper - taken Alexey's comments into account (22.14 KB, patch)
2011-04-08 08:17 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Same as previously with testing for the iterator type (24.67 KB, patch)
2011-04-09 17:11 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch (25.53 KB, patch)
2011-04-30 16:33 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2010-01-27 16:10:27 PST
Created attachment 47571 [details]
test case

Test attached.
Comment 1 Geoffrey Garen 2010-02-04 15:10:12 PST
Created attachment 48173 [details]
test case 2

Here's another test case with event listeners.
Comment 2 Julien Chaffraix 2011-03-29 09:07:41 PDT
Created attachment 87341 [details]
Proposed fix: Mark the node in the XPathResult nodeset for snapshot types
Comment 3 Build Bot 2011-03-29 10:02:08 PDT
Attachment 87341 [details] did not build on win:
Build output: http://queues.webkit.org/results/8283360
Comment 4 Julien Chaffraix 2011-03-30 23:56:59 PDT
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.
Comment 5 Build Bot 2011-03-31 00:15:07 PDT
Attachment 87673 [details] did not build on win:
Build output: http://queues.webkit.org/results/8314151
Comment 6 Julien Chaffraix 2011-04-01 22:03:59 PDT
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.
Comment 7 Build Bot 2011-04-01 23:02:54 PDT
Attachment 87964 [details] did not build on win:
Build output: http://queues.webkit.org/results/8319376
Comment 8 Alexey Proskuryakov 2011-04-02 21:50:52 PDT
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 <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 Julien Chaffraix 2011-04-08 07:50:29 PDT
> 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 Julien Chaffraix 2011-04-08 08:17:19 PDT
Created attachment 88825 [details]
Mark XPathResult nodeset JS wrapper - taken Alexey's comments into account
Comment 11 Alexey Proskuryakov 2011-04-08 09:01:14 PDT
> 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 Julien Chaffraix 2011-04-09 12:53:11 PDT
(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 Julien Chaffraix 2011-04-09 17:11:46 PDT
Created attachment 88936 [details]
Same as previously with testing for the iterator type
Comment 14 Alexey Proskuryakov 2011-04-10 10:16:33 PDT
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.
Comment 15 WebKit Commit Bot 2011-04-10 10:18:38 PDT
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
Comment 16 Julien Chaffraix 2011-04-30 16:33:07 PDT
Created attachment 91808 [details]
Patch
Comment 17 Julien Chaffraix 2011-04-30 16:36:12 PDT
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 Eric Seidel 2011-05-01 09:48:09 PDT
Oliver understands the ways of the new marking as well.
Comment 19 Geoffrey Garen 2011-05-04 15:45:13 PDT
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(); }
};
Comment 20 Julien Chaffraix 2011-05-06 08:03:54 PDT
> 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 Geoffrey Garen 2011-05-06 17:10:23 PDT
> > 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 Geoffrey Garen 2011-05-06 17:11:01 PDT
Comment on attachment 91808 [details]
Patch

Changing my r- to r+ because the optimization I suggested wouldn't work for all XPath result types.
Comment 23 WebKit Commit Bot 2011-05-11 08:24:44 PDT
Comment on attachment 91808 [details]
Patch

Clearing flags on attachment: 91808

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