Bug 42115

Summary: [GTK] Improper webkit_dom_document_evaluate implementation.
Product: WebKit Reporter: Oliver Mader <dotb52>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dbates, eric, mlq, mrobinson, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch that solves bug #42115
xan.lopez: review-
Updated patch (Added Changelog entry)
none
Updated patch
none
Updated (Changed Changelog, ..)
none
Updated patch (Genereated with git format-patch) none

Description Oliver Mader 2010-07-12 15:16:11 PDT
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
Comment 1 Oliver Mader 2010-07-12 15:28:17 PDT
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
Comment 2 Xan Lopez 2010-07-14 04:39:30 PDT
(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.
Comment 3 Oliver Mader 2010-07-23 03:57:00 PDT
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.
Comment 4 mlq 2010-08-28 06:53:41 PDT
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.
Comment 5 Xan Lopez 2010-08-28 06:59:12 PDT
Comment on attachment 65818 [details]
Patch that solves bug #42115

Looks good, but needs a ChangeLog :)
Comment 6 mlq 2010-08-28 07:24:47 PDT
Created attachment 65822 [details]
Updated patch (Added Changelog entry)
Comment 7 mlq 2010-08-28 07:33:26 PDT
Created attachment 65823 [details]
Updated patch
Comment 8 mlq 2010-08-28 08:38:25 PDT
Created attachment 65825 [details]
Updated (Changed Changelog, ..)
Comment 9 Xan Lopez 2010-08-28 08:43:54 PDT
Comment on attachment 65825 [details]
Updated (Changed Changelog, ..)

LGTM.
Comment 10 WebKit Commit Bot 2010-08-28 08:59:29 PDT
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
Comment 11 Eric Seidel (no email) 2010-08-28 09:19:21 PDT
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. :)
Comment 12 mlq 2010-08-28 09:37:14 PDT
Created attachment 65826 [details]
Updated patch (Genereated with git format-patch)
Comment 13 Xan Lopez 2010-08-28 09:39:24 PDT
Comment on attachment 65826 [details]
Updated patch (Genereated with git format-patch)

Strike 2.
Comment 14 WebKit Commit Bot 2010-08-28 09:57:04 PDT
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>
Comment 15 WebKit Commit Bot 2010-08-28 09:57:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Daniel Bates 2010-08-28 10:23:33 PDT
(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.
Comment 17 WebKit Review Bot 2010-08-28 10:27:22 PDT
http://trac.webkit.org/changeset/66308 might have broken Qt Linux Release
Comment 18 Daniel Bates 2010-08-28 10:44:21 PDT
(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.
Comment 19 mlq 2010-08-28 13:56:36 PDT
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