Bug 68044 – Replace old strtod with new strtod

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 107376 [details]
Patch

[Geoffrey Garen]

I don't expect a perf. change either, but probably worth posting perf test numbers just in case.

[Mark Hahnenberg]

Created attachment 107422 [details]
Benchmark results

[WebKit Review Bot]

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

[Geoffrey Garen]

> 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?

[Geoffrey Garen]

> 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.

[Mark Hahnenberg]

(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.

[Geoffrey Garen]

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.

[Mark Hahnenberg]

(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.

[Mark Hahnenberg]

Created attachment 126705 [details]
Patch

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 126705 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 126705 [details]
Patch

Attachment 126705 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11511335

[Philippe Normand]

Comment on attachment 126705 [details]
Patch

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

[Mark Hahnenberg]

Created attachment 126708 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Geoffrey Garen]

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.

[Mark Hahnenberg]

Created attachment 126846 [details]
Bencher results

Mostly a wash, maybe a tiny bit faster overall.

[Mark Hahnenberg]

Committed r107625: <http://trac.webkit.org/changeset/107625>

[Ojan Vafai]

Committed r107659: <http://trac.webkit.org/changeset/107659>

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

I skipped them to make our bots happier - http://trac.webkit.org/changeset/107692

Please unskip them with the proper fix. Thanks.

[Philippe Normand]

(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 :(

[Christian Sejersen]

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)

[Kenneth Rohde Christiansen]

(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.

[Mark Hahnenberg]

I *believe* this should be fixed in ToT. Could somebody please verify?

[Mark Hahnenberg]

Due to radio silence, I'll assume everything is good.