Bug 80673 - Type conversion of exponential part failed
: Type conversion of exponential part failed
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: Other Linux
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-03-08 20:20 PST by
Modified: 2012-03-14 11:38 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2012-03-08 20:51:14 PST -------
Created an attachment (id=130965) [details]
Patch
------- Comment #2 From 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 From 2012-03-09 15:40:24 PST -------
(From update of attachment 130965 [details])
r- based on Mark's comments.
------- Comment #4 From 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 From 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 From 2012-03-09 18:35:54 PST -------
Created an attachment (id=131146) [details]
Patch
------- Comment #7 From 2012-03-09 18:40:31 PST -------
(From update of attachment 131146 [details])
Attachment 131146 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11906927
------- Comment #8 From 2012-03-09 18:42:00 PST -------
(From update of attachment 131146 [details])
Attachment 131146 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11903978
------- Comment #9 From 2012-03-09 18:49:07 PST -------
(From update of attachment 131146 [details])
Attachment 131146 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11915880
------- Comment #10 From 2012-03-09 18:53:51 PST -------
Created an attachment (id=131148) [details]
Patch
------- Comment #11 From 2012-03-09 20:04:42 PST -------
(From update of attachment 131148 [details])
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 From 2012-03-10 17:11:27 PST -------
Created an attachment (id=131194) [details]
Patch
------- Comment #13 From 2012-03-12 12:22:19 PST -------
(From update of attachment 131194 [details])
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".
------- Comment #14 From 2012-03-14 11:38:55 PST -------
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