Bug 42115 – [GTK] Improper webkit_dom_document_evaluate implementation.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Oliver Mader]

Hey,

I just tried to evaluate a simple XPathExpression while I came across the
webkit_dom_document_evaluate() function which seems not to be implemented
correctly.

Like
http://www.w3.org/TR/DOM-Level-3-XPath/xpath.html#XPathEvaluator-evaluate
says the in_result parameter maybe NULL but the assert prohibits a NULL
value.
Even if I try to pass an WebKitDOMXPathResult Object the function segfaults
randomly.

I hope I made myself clear.

Best regards,
Oliver

[Oliver Mader]

A little snippet how I pass an XPathResult object:


WebKitDOMXPathNSResolver *ns = webkit_dom_document_create_ns_resolver(dom, body);
WebKitDOMXPathResult *in = g_object_new(WEBKIT_TYPE_DOM_XPATH_RESULT, NULL);
WebKitDOMXPathResult *result = webkit_dom_document_evaluate(dom, "//a | //button | //textarea | //select | //input[not(@type='hidden')]", WEBKIT_DOM_NODE(dom), ns, 7, in, NULL);

and then it goes down


** (jumanji-debug:20791): CRITICAL **: WebCore::XPathResult* WebKit::core(WebKitDOMXPathResult*): assertion `coreObject' failed

** (jumanji-debug:20791): CRITICAL **: WebKitDOMXPathResult* webkit_dom_document_evaluate(WebKitDOMDocument*, gchar*, WebKitDOMNode*, WebKitDOMXPathNSResolver*, gushort, WebKitDOMXPathResult*, GError**): assertion `converted_in_result' failed

[Xan Lopez]

(In reply to comment #1)
> A little snippet how I pass an XPathResult object:
> 
> 
> WebKitDOMXPathNSResolver *ns = webkit_dom_document_create_ns_resolver(dom, body);
> WebKitDOMXPathResult *in = g_object_new(WEBKIT_TYPE_DOM_XPATH_RESULT, NULL);
> WebKitDOMXPathResult *result = webkit_dom_document_evaluate(dom, "//a | //button | //textarea | //select | //input[not(@type='hidden')]", WEBKIT_DOM_NODE(dom), ns, 7, in, NULL);
> 
> and then it goes down
> 
> 
> ** (jumanji-debug:20791): CRITICAL **: WebCore::XPathResult* WebKit::core(WebKitDOMXPathResult*): assertion `coreObject' failed
> 
> ** (jumanji-debug:20791): CRITICAL **: WebKitDOMXPathResult* webkit_dom_document_evaluate(WebKitDOMDocument*, gchar*, WebKitDOMNode*, WebKitDOMXPathNSResolver*, gushort, WebKitDOMXPathResult*, GError**): assertion `converted_in_result' failed

The crash happens because you are just passing an empty GObject container without a core XPathResult inside. We need a way to construct proper empty XPathResults and/or fix the method to accept NULL objects.

[Oliver Mader]

Seems this is a little bigger, there are more functions which should be capable of parameters which accept NULL as values, e.g. like the pseudoElement of webkit_dom_dom_window_get_computed_style.

mrobinson pointed out that this is a problem in the perl script which generates the C code out of the idl files.

Created attachment 65818 [details]
Patch that solves bug #42115

This patch (hack) solves this bug report by changing the CodeGeneratorGObject.pm script that it generates a proper implementation of the webkit_dom_document_evaluate function. 
It allows the in_result parameter to be NULL and checks further usage of it and the other converted values.

[Xan Lopez]

Comment on attachment 65818 [details]
Patch that solves bug #42115

Looks good, but needs a ChangeLog :)

Created attachment 65822 [details]
Updated patch (Added Changelog entry)

Created attachment 65823 [details]
Updated patch

Created attachment 65825 [details]
Updated (Changed Changelog, ..)

[Xan Lopez]

Comment on attachment 65825 [details]
Updated (Changed Changelog, ..)

LGTM.

[WebKit Commit Bot]

Comment on attachment 65825 [details]
Updated (Changed Changelog, ..)

Rejecting patch 65825 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Xan Lopez', u'--force']" exit_code: 2
Parsed 2 diffs from patch file(s).
patching file WebCore/ChangeLog
patch: **** malformed patch at line 24:  

patching file WebCore/bindings/scripts/CodeGeneratorGObject.pm

Full output: http://queues.webkit.org/results/3841070

[Eric Seidel (no email)]

Did someone hand-edit the patch?  That's the most common cause of this kind of failure.

webkit-patch apply-from-bug 42115

will repro the failure for you. :)

Created attachment 65826 [details]
Updated patch (Genereated with git format-patch)

[Xan Lopez]

Comment on attachment 65826 [details]
Updated patch (Genereated with git format-patch)

Strike 2.

[WebKit Commit Bot]

Comment on attachment 65826 [details]
Updated patch (Genereated with git format-patch)

Clearing flags on attachment: 65826

Committed r66308: <http://trac.webkit.org/changeset/66308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(In reply to comment #11)
> Did someone hand-edit the patch?  That's the most common cause of this kind of failure.
> 
> webkit-patch apply-from-bug 42115
> 
> will repro the failure for you. :)

The header of the WebCore/ChangeLog diff has an invalid range line "@@ -1,3 +1,21 @@". It should read "@@ -1,3 +1,19 @@". I'm not sure why someone would hand-edit the range.

Correcting the range line in the original patch I was able to apply the patch. I then ran svn-create-patch, and the output, including the range line in question, is correctly generated. I'm curious as to how attachment 65825 [details] <https://bugs.webkit.org/attachment.cgi?id=65825> was generated.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66308 might have broken Qt Linux Release

[Daniel Bates]

(In reply to comment #16)
> (In reply to comment #11)
> > Did someone hand-edit the patch?  That's the most common cause of this kind of failure.
> > 
> > webkit-patch apply-from-bug 42115
> > 
> > will repro the failure for you. :)
> 
> The header of the WebCore/ChangeLog diff has an invalid range line "@@ -1,3 +1,21 @@". It should read "@@ -1,3 +1,19 @@". I'm not sure why someone would hand-edit the range.
>

err, I doubt that the range line was hand-edited. Most likely lines were removed by hand from the ChangeLog diff. That is, the ChangeLog diff was originally 21 lines long and was reduced to 19 lines by hand.

For clarification: I did not bear in mind that I changed a line in the Changelog right in the patch file, so in the end there is nothing wrong with the script. Sorry for the trouble