XPath double minus should coerce to number.

RESOLVED FIXED

Status

()

Core
DOM
--
minor
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Yonathan Randolph, Unassigned)

Tracking

({helpwanted, student-project})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments)

(Reporter)

Description

7 years ago
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
Jonas, are you our current xpath expert?
Component: DOM: Core & HTML → DOM: Other
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: helpwanted, student-project

Comment 3

6 years ago
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 on attachment 570208 [details] [diff] [review]
Patch

Thanks for the patch!  You want to request review on patches so they get looked at....
Attachment #570208 - Flags: review?(jonas)
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.
Attachment #570208 - Flags: review?(jonas) → review-

Comment 6

6 years ago
How do I wrap a txNumberExpr around the inner expression? I thought txNumberExpr wants two expressions and an operation...
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>)).
Also, please write a mochitest for this as part of the patch.

Comment 9

6 years ago
Created attachment 570463 [details] [diff] [review]
Patch
Attachment #570463 - Flags: review?(jonas)
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.
Attachment #570463 - Flags: review?(jonas) → review+
Oh, and if you want to remove all other nullchecks-after-new in the parser, that'd be awesome ;-)

Comment 12

6 years ago
Created attachment 572337 [details] [diff] [review]
Coerce exprs that were negated at some point to numbers
Attachment #572337 - Flags: review?(jonas)

Comment 13

6 years ago
Created attachment 572338 [details] [diff] [review]
Remove nullcheck after new's
Attachment #572338 - Flags: review?(jonas)
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.
Attachment #572337 - Flags: review?(jonas) → review+
Comment on attachment 572338 [details] [diff] [review]
Remove nullcheck after new's

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

Yay!
Attachment #572338 - Flags: review?(jonas) → review+

Comment 16

6 years ago
Created attachment 572341 [details] [diff] [review]
Don't break single negation
Attachment #572341 - Flags: review?(jonas)
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.
Attachment #572341 - Flags: review?(jonas) → review+

Comment 18

6 years ago
Er, yes, that's supposed to say -8.
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

6 years ago
If you could put it down as "Timothy Zhu" that'd be great.
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Component: DOM: Other → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.