Bug 68044 - Replace old strtod with new strtod
Summary: Replace old strtod with new strtod
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on: 78774
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-13 17:12 PDT by Mark Hahnenberg
Modified: 2012-02-22 13:22 PST (History)
11 users (show)

See Also:


Attachments
Patch (17.28 KB, patch)
2011-09-14 12:40 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Benchmark results (4.42 KB, text/plain)
2011-09-14 16:50 PDT, Mark Hahnenberg
no flags Details
Patch (21.97 KB, patch)
2012-02-12 20:29 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (22.18 KB, patch)
2012-02-12 20:49 PST, Mark Hahnenberg
ggaren: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Bencher results (7.25 KB, text/plain)
2012-02-13 15:09 PST, Mark Hahnenberg
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2011-09-13 17:12:25 PDT
As part of the transition from the old dtoa library to the new dtoa library, we need to replace strtod.  I don't expect this to be much of a performance change, but it's a necessary step to deprecate the old code.
Comment 1 Mark Hahnenberg 2011-09-14 12:40:30 PDT
Created attachment 107376 [details]
Patch
Comment 2 Geoffrey Garen 2011-09-14 15:28:47 PDT
I don't expect a perf. change either, but probably worth posting perf test numbers just in case.
Comment 3 Mark Hahnenberg 2011-09-14 16:50:51 PDT
Created attachment 107422 [details]
Benchmark results
Comment 4 WebKit Review Bot 2011-09-15 10:14:24 PDT
Comment on attachment 107376 [details]
Patch

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

New failing tests:
fast/forms/range/input-valueasnumber-range.html
fast/forms/ValidityState-typeMismatch-number.html
fast/forms/input-valueasnumber-number.html
Comment 5 Geoffrey Garen 2011-09-15 11:41:58 PDT
> New failing tests:
> fast/forms/range/input-valueasnumber-range.html
> fast/forms/ValidityState-typeMismatch-number.html
> fast/forms/input-valueasnumber-number.html

Are these real failures?
Comment 6 Geoffrey Garen 2011-09-15 11:45:23 PDT
> Are these real failures?

To be clearer (in the spirit of webkit-dev discussion about ambiguous comments), I believe that determining whether these are real failures is a blocking issue for landing this patch.
Comment 7 Mark Hahnenberg 2011-09-15 11:50:09 PDT
(In reply to comment #6)
> > Are these real failures?
> 
> To be clearer (in the spirit of webkit-dev discussion about ambiguous comments), I believe that determining whether these are real failures is a blocking issue for landing this patch.

Yes, they're real failures.  Currently working on it.
Comment 8 Geoffrey Garen 2011-09-15 11:52:30 PDT
Comment on attachment 107376 [details]
Patch

r- due to test failures.

Why did this fail on Chromium only? Is there something unique about the Chromium build here? If so, perhaps a Chromium maintainer can help with this.
Comment 9 Mark Hahnenberg 2011-09-15 11:58:27 PDT
(In reply to comment #8)
> (From update of attachment 107376 [details])
> r- due to test failures.
> 
> Why did this fail on Chromium only? Is there something unique about the Chromium build here? If so, perhaps a Chromium maintainer can help with this.

I believe Chromium EWS is the only bot that runs regression tests.
Comment 10 Mark Hahnenberg 2012-02-12 20:29:15 PST
Created attachment 126705 [details]
Patch
Comment 11 WebKit Review Bot 2012-02-12 20:31:37 PST
Attachment 126705 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/text/WTFString.cpp:1057:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Early Warning System Bot 2012-02-12 20:43:41 PST
Comment on attachment 126705 [details]
Patch

Attachment 126705 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11509598
Comment 13 Gyuyoung Kim 2012-02-12 20:45:49 PST
Comment on attachment 126705 [details]
Patch

Attachment 126705 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11511335
Comment 14 Philippe Normand 2012-02-12 20:47:28 PST
Comment on attachment 126705 [details]
Patch

Attachment 126705 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11509599
Comment 15 Mark Hahnenberg 2012-02-12 20:49:56 PST
Created attachment 126708 [details]
Patch
Comment 16 WebKit Review Bot 2012-02-12 20:51:49 PST
Attachment 126708 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/text/WTFString.cpp:1058:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 WebKit Review Bot 2012-02-12 21:21:05 PST
Comment on attachment 126708 [details]
Patch

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

New failing tests:
media/media-fragments/TC0070-TC0079.html
Comment 18 Geoffrey Garen 2012-02-13 11:26:01 PST
Comment on attachment 126708 [details]
Patch

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

Can you post performance results?

> Source/JavaScriptCore/ChangeLog:16
> +        It's takes a template argument to allow clients to determine statically whether it should allow 

Typo: "It's" should be "It".

> Source/JavaScriptCore/parser/Lexer.cpp:1331
> +                tokenData->doubleValue = WTF::strtod<true>(reinterpret_cast<const char*>(m_buffer8.data()), 0);

The WebKit style is to use bool in cases where the value is usually in a named variable, but to use a named enumeration in other cases, for readability. You should use a named enumeration in your template parameter, rather than bool.
Comment 19 Mark Hahnenberg 2012-02-13 15:09:53 PST
Created attachment 126846 [details]
Bencher results

Mostly a wash, maybe a tiny bit faster overall.
Comment 20 Mark Hahnenberg 2012-02-13 15:38:59 PST
Committed r107625: <http://trac.webkit.org/changeset/107625>
Comment 21 Ojan Vafai 2012-02-13 18:40:57 PST
Committed r107659: <http://trac.webkit.org/changeset/107659>
Comment 22 Csaba Osztrogonác 2012-02-14 01:48:40 PST
(In reply to comment #20)
> Committed r107625: <http://trac.webkit.org/changeset/107625>

It broke 3 tests on Qt:

--- /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/viewport/viewport-67-expected.txt 
+++ /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/viewport/viewport-67-actual.txt 
@@ -1,3 +1,3 @@
-CONSOLE MESSAGE: line 3: Viewport argument value "100;" for key "width" was truncated to its numeric prefix.
+CONSOLE MESSAGE: line 3: Viewport argument value "100;" for key "width" not recognized. Content ignored.
 viewport size 320x352 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000
 

--- /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/viewport/viewport-68-expected.txt 
+++ /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/viewport/viewport-68-actual.txt 
@@ -1,3 +1,3 @@
-CONSOLE MESSAGE: line 3: Viewport argument value "100x" for key "width" was truncated to its numeric prefix.
+CONSOLE MESSAGE: line 3: Viewport argument value "100x" for key "width" not recognized. Content ignored.
 viewport size 320x352 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000
 

--- /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/viewport/viewport-warnings-5-expected.txt 
+++ /home/webkitbuildbot/slaves/release32bit-NRWT/buildslave/qt-linux-32-release-NRWT/build/layout-test-results/fast/viewport/viewport-warnings-5-actual.txt 
@@ -1,6 +1,6 @@
 CONSOLE MESSAGE: line 3: Viewport argument value "device-width;" for key "width" not recognized. Content ignored.
-CONSOLE MESSAGE: line 3: Viewport argument value "1.0;" for key "initial-scale" was truncated to its numeric prefix.
-CONSOLE MESSAGE: line 3: Viewport argument value "1.0;" for key "maximum-scale" was truncated to its numeric prefix.
-CONSOLE MESSAGE: line 3: Viewport argument value "0;" for key "user-scalable" was truncated to its numeric prefix.
-viewport size 320x352 scale 1.000000 with limits [1.000000, 1.000000] and userScalable 0.000000
+CONSOLE MESSAGE: line 3: Viewport argument value "1.0;" for key "initial-scale" not recognized. Content ignored.
+CONSOLE MESSAGE: line 3: Viewport argument value "1.0;" for key "maximum-scale" not recognized. Content ignored.
+CONSOLE MESSAGE: line 3: Viewport argument value "0;" for key "user-scalable" not recognized. Content ignored.
+viewport size 1280x1408 scale 0.250000 with limits [0.250000, 0.250000] and userScalable 0.000000
Comment 23 Csaba Osztrogonác 2012-02-14 02:07:21 PST
I skipped them to make our bots happier - http://trac.webkit.org/changeset/107692

Please unskip them with the proper fix. Thanks.
Comment 24 Philippe Normand 2012-02-14 08:11:21 PST
(In reply to comment #22)
> (In reply to comment #20)
> > Committed r107625: <http://trac.webkit.org/changeset/107625>
> 
> It broke 3 tests on Qt:
> 

And GTK :(
Comment 25 Christian Sejersen 2012-02-15 09:10:43 PST
When using the viewport meta tag the standard [1] specifies the use of commas for keyword values. Android originally allowed the use of semicolon - making the use widespread - and iPhone still supports it (due to allowing trailing junk) so there are still many websites using semicolon (plus the fact that Windows Phone only supports semicolon).

The bug comes from ViewportArguments::numericPrefix that uses toFloat that calls strtod. The patch below fixes the viewport issue, but I don't know if it breaks other things, as the code explicitly set it to not allowing trailing junk.

[1] http://dev.w3.org/html5/spec/Overview.html#the-meta-element

diff --git a/Source/JavaScriptCore/wtf/text/WTFString.cpp b/Source/JavaScriptCore/wtf/text/WTFString.cpp
index 8e0ea32..a8f90fc 100644
--- a/Source/JavaScriptCore/wtf/text/WTFString.cpp
+++ b/Source/JavaScriptCore/wtf/text/WTFString.cpp
@@ -1053,7 +1053,7 @@ static inline double toDoubleType(const CharType* data, size_t length, bool* ok,
     bytes[length] = '\0';
     char* start = bytes.data();
     char* end;
-    double val = WTF::strtod<WTF::DisallowTrailingJunk>(start, &end);
+    double val = WTF::strtod<WTF::AllowTrailingJunk>(start, &end);
     if (ok)
         *ok = (end == 0 || *end == '\0') && !isnan(val);
     if (didReadNumber)
Comment 26 Kenneth Rohde Christiansen 2012-02-15 09:19:14 PST
(In reply to comment #25)
> When using the viewport meta tag the standard [1] specifies the use of commas for keyword values. Android originally allowed the use of semicolon - making the use widespread - and iPhone still supports it (due to allowing trailing junk) so there are still many websites using semicolon (plus the fact that Windows Phone only supports semicolon).

So the spec mandates to separate the values on comma. WebKit additionally supports splitting on spaces.

Android shipped with support for splitting using ; as well, making it somewhat widespread in viewport meta tag examples, especially as "; " worked on iOS due to the fact that iOS splits on spaces (due to WebKit) and ignored the ; as trailing junk.

Now that Windows Phone supports viewport meta tag using the ";" and advertising that in their documentation/blog posts, it seems that the ; unfortunately is becoming even more popular (it is even used on www.html5test.com).

The above patch breaks numericPrefix using by the viewport handling, as it no longer supports the trailing junk, which is going to break numerous pages on mobile browsers.

The main difference in supporting ";" as a separator (already shot down in other bug reports) and not supporting it, is that we would parse things like "width=320;initial-scale=1" like intended, where the trailing junk support would only parse the width = 320 and ignore the rest as garbage due to no space following the ;. This is a very minor problem as most sites using ; follows it by a space.
Comment 27 Mark Hahnenberg 2012-02-17 10:41:51 PST
I *believe* this should be fixed in ToT. Could somebody please verify?
Comment 28 Mark Hahnenberg 2012-02-22 13:22:24 PST
Due to radio silence, I'll assume everything is good.