872853 - +"0x" erroneously yields zero

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Richard Gibson]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.22 (KHTML, like Gecko) Ubuntu Chromium/25.0.1364.160 Chrome/25.0.1364.160 Safari/537.22

Steps to reproduce:

cast string "0x" to number with unary `+` operator (similar to but distinct from #582643)


Actual results:

a result of 0 (zero)


Expected results:

a result of NaN (http://www.ecma-international.org/ecma-262/5.1/#sec-9.3.1)

Comment 1

[Jeff Walden [:Waldo]]

Huh, we get this wrong?  That's kind of amazing after all this time.  :-)

This should be pretty easy to fix, excepting that the fix is to a centralized method, and making sure all the callers have the semantics desired here might take a little work.

Comment 2

[Richard Gibson]

Well, it *did* take a weird path to hit this:
* jQuery bug with filtering by a selector containing escaped whitespace: http://bugs.jquery.com/ticket/13894
* Fix in Sizzle CSS unescaping logic: https://github.com/jquery/sizzle/commit/00a7e0d8d66fb7915a24a2270033b8efc9e9c774#diff-3

Comment 3

[Jeff Walden [:Waldo]]

Attachment: 1 - Eliminate jsnuminlines.h

Turns out with methodjit gone, StringToNumberType is only used one place, only with T = double.  Simplify!

Comment 4

[Jeff Walden [:Waldo]]

Attachment: 2 - Detect no-hex-digits as NaN

Comment 5

[Tom S [:evilpie]]

Comment on attachment 750786 [details] [diff] [review]
2 - Detect no-hex-digits as NaN

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

::: js/src/jsnum.cpp
@@ +1361,5 @@
>          const jschar *endptr;
>          double d;
> +        if (!GetPrefixInteger(cx, bp + 2, end, 16, &endptr, &d) ||
> +            endptr == bp + 2 ||
> +            SkipSpace(endptr, end) != end)

This is not super obvious. A comment describing the last two parts could be good.

::: js/src/tests/ecma_5/Number/tonumber-string-hex.js
@@ +12,5 @@
> + * BEGIN TEST *
> + **************/
> +
> +assertEq(+"0x", NaN);
> +assertEq(+"\n0x", NaN);

I feel like we should probably test some other space.

Comment 6

[Tom S [:evilpie]]

Comment on attachment 750778 [details] [diff] [review]
1 - Eliminate jsnuminlines.h

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

nice

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23741f85220a
https://hg.mozilla.org/mozilla-central/rev/8ed13280c811