Bug 42115 - [GTK] Improper webkit_dom_document_evaluate implementation.
: [GTK] Improper webkit_dom_document_evaluate implementation.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-12 15:16 PDT by Oliver Mader
Modified: 2010-08-28 13:56 PDT (History)
8 users (show)

See Also:


Attachments
Patch that solves bug #42115 (2.55 KB, patch)
2010-08-28 06:53 PDT, mlq
xan.lopez: review-
Details | Formatted Diff | Diff
Updated patch (Added Changelog entry) (3.34 KB, patch)
2010-08-28 07:24 PDT, mlq
no flags Details | Formatted Diff | Diff
Updated patch (3.17 KB, text/plain)
2010-08-28 07:33 PDT, mlq
no flags Details
Updated (Changed Changelog, ..) (3.17 KB, patch)
2010-08-28 08:38 PDT, mlq
no flags Details | Formatted Diff | Diff
Updated patch (Genereated with git format-patch) (3.54 KB, patch)
2010-08-28 09:37 PDT, mlq
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 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