Type conversion of exponential part failed

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Hojong Han]

Testcase ecma/TypeConversion/9.3.1-3.js failed 
Failure messages were:
-"1e-" = -1 FAILED! expected: NaN

It's been occurred because "e-" is regarded just as trailing junks while parsing exponential part.
It should not be consider as trailing junks without decimal digits.

[Hojong Han]

Created attachment 130965 [details]
Patch

[Mark Hahnenberg]

You're correct that this is a regression as per the spec, but the way in which you've fixed this regression is probably not the way we want to go. The code you've modified was pulled in from an upstream open source repository (http://code.google.com/p/double-conversion/), and we probably want to leave it alone as much as possible. Also, the fact that we can ignore trailing junk strings at the end of otherwise valid numbers if we so choose is a feature, not a bug.

As you've already figured out, the issue is that the place that calls strtod expects parsing trailing junk strings to return NaN, but we're ignoring these trailing junk strings and just returning the valid prefix. Instead of removing the ability to ignore junk strings, as your current patch does, we need to pass the correct AllowJunkStringTag value to strtod when calling jsToNumber.

[Geoffrey Garen]

Comment on attachment 130965 [details]
Patch

r- based on Mark's comments.

[Hojong Han]

(In reply to comment #2)
> You're correct that this is a regression as per the spec, but the way in which you've fixed this regression is probably not the way we want to go. The code you've modified was pulled in from an upstream open source repository (http://code.google.com/p/double-conversion/), and we probably want to leave it alone as much as possible. Also, the fact that we can ignore trailing junk strings at the end of otherwise valid numbers if we so choose is a feature, not a bug.
> 
> As you've already figured out, the issue is that the place that calls strtod expects parsing trailing junk strings to return NaN, but we're ignoring these trailing junk strings and just returning the valid prefix. Instead of removing the ability to ignore junk strings, as your current patch does, we need to pass the correct AllowJunkStringTag value to strtod when calling jsToNumber.

I totally agree with your explanation on AllowJunkStringTag, but I was deeply wondering if 'e' or 'E' without signed decimal digits should be considered as trailing junk or not. I decided at that time it's not trailing junk but obvious parsing error.
Isn't it correct that additional things, only after signed decimal digits, are regarded as junk in case of parsing exponential part?? I want you to make it sure this one more time.

And I cannot find what you want me to check at (http://code.google.com/p/double-conversion/). Could you let me know more specific URL or something?

[Mark Hahnenberg]

> I totally agree with your explanation on AllowJunkStringTag, but I was deeply wondering if 'e' or 'E' without signed decimal digits should be considered as trailing junk or not. I decided at that time it's not trailing junk but obvious parsing error.
> Isn't it correct that additional things, only after signed decimal digits, are regarded as junk in case of parsing exponential part?? I want you to make it sure this one more time.

According to the ECMA 262 spec section 9.3.1, if you have an exponential, you must have either 'e' or 'E' which must be always followed by a decimal string. You can compare our behavior with Chrome or Firefox.

> And I cannot find what you want me to check at (http://code.google.com/p/double-conversion/). Could you let me know more specific URL or something?

Nothing to look at there, I was just showing you the upstream project I was referencing.

I actually have a patch ready to go for this which fixes a couple other things that were wrong too, so don't worry about submitting a new patch. Thanks for reporting this bug!

[Mark Hahnenberg]

Created attachment 131146 [details]
Patch

[Early Warning System Bot]

Comment on attachment 131146 [details]
Patch

Attachment 131146 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11906927

[Early Warning System Bot]

Comment on attachment 131146 [details]
Patch

Attachment 131146 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11903978

[Gustavo Noronha (kov)]

Comment on attachment 131146 [details]
Patch

Attachment 131146 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11915880

[Mark Hahnenberg]

Created attachment 131148 [details]
Patch

[WebKit Review Bot]

Comment on attachment 131148 [details]
Patch

Attachment 131148 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11892943

New failing tests:
fast/forms/number/ValidityState-typeMismatch-number.html
fast/forms/range/input-valueasnumber-range.html
fast/forms/number/input-valueasnumber-number.html

[Mark Hahnenberg]

Created attachment 131194 [details]
Patch

[Geoffrey Garen]

Comment on attachment 131194 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131194&action=review

r=me

> Source/JavaScriptCore/wtf/dtoa/double-conversion.cc:437
>      // Returns true if a nonspace found and false if the end has reached.

Please update this comment before committing -- and possibly the function name. "Whitespace" would be a better term than "space".

[Mark Hahnenberg]

Fixed in http://trac.webkit.org/changeset/110657 with build fixes in http://trac.webkit.org/changeset/110659 and http://trac.webkit.org/changeset/110660