Bug 34231

Created attachment 48173 [details] test case 2 Here's another test case with event listeners.

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

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

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.

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

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.

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

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?

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

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

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

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

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

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

Created attachment 91808 [details] Patch

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!

Oliver understands the ways of the new marking as well.

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

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

> > 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 on attachment 91808 [details] Patch Changing my r- to r+ because the optimization I suggested wouldn't work for all XPath result types.

Comment on attachment 91808 [details] Patch Clearing flags on attachment: 91808 Committed r86234: <http://trac.webkit.org/changeset/86234>

All reviewed patches have been landed. Closing bug.