616774 - XPath double minus should coerce to number.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Yonathan Randolph]

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 [:bzbarsky]]

Jonas, are you our current xpath expert?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[nattofriends]

Attachment: Patch

I'm a student, and this is my first? second? time contributing to Firefox. Hope this goes well...

Comment 4

[Boris Zbarsky [:bzbarsky]]

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) No longer reading bugmail consistently]

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

[nattofriends]

How do I wrap a txNumberExpr around the inner expression? I thought txNumberExpr wants two expressions and an operation...

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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) No longer reading bugmail consistently]

Also, please write a mochitest for this as part of the patch.

Comment 9

[nattofriends]

Attachment: Patch

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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) No longer reading bugmail consistently]

Oh, and if you want to remove all other nullchecks-after-new in the parser, that'd be awesome ;-)

Comment 12

[nattofriends]

Attachment: Coerce exprs that were negated at some point to numbers

Comment 13

[nattofriends]

Attachment: Remove nullcheck after new's

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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) No longer reading bugmail consistently]

Comment on attachment 572338 [details] [diff] [review]
Remove nullcheck after new's

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

Yay!

Comment 16

[nattofriends]

Attachment: Don't break single negation

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[nattofriends]

Er, yes, that's supposed to say -8.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

[e-mail]: 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

[nattofriends]

If you could put it down as "Timothy Zhu" that'd be great.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.