Last Comment Bug 616774 - XPath double minus should coerce to number.
: XPath double minus should coerce to number.
Status: RESOLVED FIXED
: helpwanted, student-project
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- minor (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
http://trac.webkit.org/export/head/tr...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-04 23:24 PST by Yonathan Randolph
Modified: 2013-04-04 13:52 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (534 bytes, patch)
2011-10-28 01:52 PDT, Timothy Zhu
jonas: review-
Details | Diff | Review
Patch (3.05 KB, patch)
2011-10-29 00:02 PDT, Timothy Zhu
jonas: review+
Details | Diff | Review
Coerce exprs that were negated at some point to numbers (2.89 KB, patch)
2011-11-06 14:21 PST, Timothy Zhu
jonas: review+
Details | Diff | Review
Remove nullcheck after new's (4.29 KB, patch)
2011-11-06 14:22 PST, Timothy Zhu
jonas: review+
Details | Diff | Review
Don't break single negation (3.02 KB, patch)
2011-11-06 14:40 PST, Timothy Zhu
jonas: review+
Details | Diff | Review

Description Yonathan Randolph 2010-12-04 23:24:10 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/534.7 (KHTML, like Gecko) Chrome/7.0.517.44 Safari/534.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.12) Gecko/20101027 Ubuntu/10.04 (lucid) Firefox/3.6.12

The following expression fails with XPathException 52 because the return type is a string rather than a number.
document.evaluate('- - "1"', document, null, XPathResult.ANY_TYPE, null).numberValue

It should return a number, as indicated by XPath 1.0 spec: "The numeric operators convert their operands to numbers as if by calling the number function." (http://www.w3.org/TR/xpath/#numbers).

The easy workaround is to ask for the type coercion:
document.evaluate('- - "1"', document, null, XPathResult.NUMBER_TYPE, null).numberValue
document.evaluate('number(- - "1")', document, null, XPathResult.ANY_TYPE, null).numberValue

Reproducible: Always

Steps to Reproduce:
1. Type this into firebug:
document.evaluate('- - "1"', document, null, XPathResult.ANY_TYPE, null).numberValue

Actual Results:  
XPathException (code 52)

Expected Results:  
1

I found this minor bug while writing a test case for WebKit:
https://bugs.webkit.org/show_bug.cgi?id=50366
Comment 1 Boris Zbarsky [:bz] 2010-12-04 23:32:54 PST
Jonas, are you our current xpath expert?
Comment 2 Jonas Sicking (:sicking) 2010-12-06 11:08:55 PST
Yeah, I've known about this for a long time. The bug is here:

http://mxr.mozilla.org/mozilla-central/source/content/xslt/src/xpath/txExprParser.cpp#316

Patches welcome.
Comment 3 Timothy Zhu 2011-10-28 01:52:48 PDT
Created attachment 570208 [details] [diff] [review]
Patch

I'm a student, and this is my first? second? time contributing to Firefox. Hope this goes well...
Comment 4 Boris Zbarsky [:bz] 2011-10-28 08:15:37 PDT
Comment on attachment 570208 [details] [diff] [review]
Patch

Thanks for the patch!  You want to request review on patches so they get looked at....
Comment 5 Jonas Sicking (:sicking) 2011-10-28 09:17:41 PDT
Comment on attachment 570208 [details] [diff] [review]
Patch

This won't work. For example evaluating

document.evaluate('- - id("foo")', document, null, XPathResult.ANY_TYPE, null).numberValue

should return the contents of element with id="foo" as a number value. I don't think your patch will do that.

You need to either wrap a new UnaryExpr for each '-' token that is found. Or detect when an even number of '-' are used and then wrap a txNumberExpr around the inner expression.
Comment 6 Timothy Zhu 2011-10-28 22:28:00 PDT
How do I wrap a txNumberExpr around the inner expression? I thought txNumberExpr wants two expressions and an operation...
Comment 7 Jonas Sicking (:sicking) 2011-10-28 22:36:03 PDT
You are correct, my bad. What we want here is to wrap it in a txCoreFunctionCall with mType set to NUMBER and taking one argument which is the wrapped expression.

Or you can wrap with one UnaryExpr for each '-' sign. So for "--<some expr>" we'd get UnaryExpr(UnaryExpr(<some expr>)).
Comment 8 Jonas Sicking (:sicking) 2011-10-28 22:36:34 PDT
Also, please write a mochitest for this as part of the patch.
Comment 9 Timothy Zhu 2011-10-29 00:02:47 PDT
Created attachment 570463 [details] [diff] [review]
Patch
Comment 10 Jonas Sicking (:sicking) 2011-10-31 16:39:59 PDT
Comment on attachment 570463 [details] [diff] [review]
Patch

Review of attachment 570463 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with those changes. Though please attach a new patch for easier landing.

::: content/xslt/src/xpath/txExprParser.cpp
@@ +331,5 @@
> +                
> +                if (!unaryExpr) {
> +                    rv = NS_ERROR_OUT_OF_MEMORY;
> +                    break;
> +                }

Actually, nowadays |new| can't fail. So no need to nullcheck unaryExpr.

So just remove the unaryExpr temporary and instead create a temporary of type FunctionCall here. Also remove the nullcheck.

@@ +335,5 @@
> +                }
> +                
> +                rv = ((FunctionCall*) unaryExpr)->addParam(expr);
> +                if (NS_FAILED(rv))
> +                    return rv;

And then after here do |expr = <the new functioncall temporary>;|

@@ +343,5 @@
> +                
> +                if (!unaryExpr) {
> +                    rv = NS_ERROR_OUT_OF_MEMORY;
> +                    break;
> +                }

And here just do:

expr = new UnaryExpr(expr);

and remove the nullcheck.
Comment 11 Jonas Sicking (:sicking) 2011-10-31 16:40:32 PDT
Oh, and if you want to remove all other nullchecks-after-new in the parser, that'd be awesome ;-)
Comment 12 Timothy Zhu 2011-11-06 14:21:46 PST
Created attachment 572337 [details] [diff] [review]
Coerce exprs that were negated at some point to numbers
Comment 13 Timothy Zhu 2011-11-06 14:22:25 PST
Created attachment 572338 [details] [diff] [review]
Remove nullcheck after new's
Comment 14 Jonas Sicking (:sicking) 2011-11-06 14:31:08 PST
Comment on attachment 572337 [details] [diff] [review]
Coerce exprs that were negated at some point to numbers

Review of attachment 572337 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: content/xslt/src/xpath/txExprParser.cpp
@@ +336,4 @@
>              }
> +            else {
> +                expr.forget();
> +                expr = new UnaryExpr(expr);

This won't work. expr.forget() sets expr to null. I'm surprised this doesn't crash?

expr = new UnaryExpr(expr.forget());

::: content/xslt/tests/mochitest/test_bug616774.html
@@ +18,5 @@
> +<script class="testbody" type="text/javascript">
> +
> +/** Test for Bug 616774 **/
> +is(document.evaluate('- - "999"', document, null, XPathResult.ANY_TYPE, null).numberValue, 999, "String literal should evaluate to itself");
> +is(document.evaluate('- - id("content")', document, null, XPathResult.ANY_TYPE, null).numberValue, 42, "DOM element should evaluate to itself coerced to a number");

Also add a test for just a single negative sign. This is likely why you never crashed.
Comment 15 Jonas Sicking (:sicking) 2011-11-06 14:32:09 PST
Comment on attachment 572338 [details] [diff] [review]
Remove nullcheck after new's

Review of attachment 572338 [details] [diff] [review]:
-----------------------------------------------------------------

Yay!
Comment 16 Timothy Zhu 2011-11-06 14:40:29 PST
Created attachment 572341 [details] [diff] [review]
Don't break single negation
Comment 17 Jonas Sicking (:sicking) 2011-11-06 15:27:52 PST
Comment on attachment 572341 [details] [diff] [review]
Don't break single negation

Review of attachment 572341 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming this passes tests after fixing the below issue.

No need to attach a new patch, I can fix the issue before landing.

::: content/xslt/tests/mochitest/test_bug616774.html
@@ +17,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +/** Test for Bug 616774 **/
> +is(document.evaluate('- "8"', document, null, XPathResult.ANY_TYPE, null).numberValue, 999, "Negated string literal should evaluate to itself negated");

This should say -8 and not 999, right? I can fix that while landing though.
Comment 18 Timothy Zhu 2011-11-06 15:29:02 PST
Er, yes, that's supposed to say -8.
Comment 19 Jonas Sicking (:sicking) 2011-11-07 20:53:30 PST
nattofriends@gmail.com: If you get this in time, let me know what name you want me to use when pushing this patch (all patches have the patch-writers name and email attached to them)
Comment 20 Timothy Zhu 2011-11-07 20:55:11 PST
If you could put it down as "Timothy Zhu" that'd be great.
Comment 21 Jonas Sicking (:sicking) 2011-11-07 22:37:02 PST
Checked in

https://hg.mozilla.org/mozilla-central/rev/048ee4bf28f2
https://hg.mozilla.org/mozilla-central/rev/81dedcc49ac0

Thanks for the patches!

For what it's worth, there's lots more cleanup that we could use help with. And if you want to attack something bigger, in the XSLT/XPath code or elsewhere, do let me know.

For example bug 213996 and bug 94270 are some of the few outstanding compliance bugs that we have.

Note You need to log in before you can comment on or make changes to this bug.