Bug 80673 - Type conversion of exponential part failed
: Type conversion of exponential part failed
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: 528+ (Nightly build)
: Other Linux
: P2 Normal
Assigned To: Mark Hahnenberg
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-08 20:20 PST by Hojong Han
Modified: 2012-03-14 11:38 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.13 KB, patch)
2012-03-08 20:51 PST, Hojong Han
no flags Details | Formatted Diff | Diff
Patch (48.08 KB, patch)
2012-03-09 18:35 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (48.90 KB, patch)
2012-03-09 18:53 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (54.14 KB, patch)
2012-03-10 17:11 PST, Mark Hahnenberg
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hojong Han 2012-03-08 20:20:01 PST
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.
Comment 1 Hojong Han 2012-03-08 20:51:14 PST
Created attachment 130965 [details]
Patch
Comment 2 Mark Hahnenberg 2012-03-09 15:13:08 PST
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.
Comment 3 Geoffrey Garen 2012-03-09 15:40:24 PST
Comment on attachment 130965 [details]
Patch

r- based on Mark's comments.
Comment 4 Hojong Han 2012-03-09 17:42:17 PST
(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?
Comment 5 Mark Hahnenberg 2012-03-09 17:48:50 PST
> 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!
Comment 6 Mark Hahnenberg 2012-03-09 18:35:54 PST
Created attachment 131146 [details]
Patch
Comment 7 Early Warning System Bot 2012-03-09 18:40:31 PST
Comment on attachment 131146 [details]
Patch

Attachment 131146 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11906927
Comment 8 Early Warning System Bot 2012-03-09 18:42:00 PST
Comment on attachment 131146 [details]
Patch

Attachment 131146 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11903978
Comment 9 Gustavo Noronha (kov) 2012-03-09 18:49:07 PST
Comment on attachment 131146 [details]
Patch

Attachment 131146 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11915880
Comment 10 Mark Hahnenberg 2012-03-09 18:53:51 PST
Created attachment 131148 [details]
Patch
Comment 11 WebKit Review Bot 2012-03-09 20:04:42 PST
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
Comment 12 Mark Hahnenberg 2012-03-10 17:11:27 PST
Created attachment 131194 [details]
Patch
Comment 13 Geoffrey Garen 2012-03-12 12:22:19 PDT
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".