Bug 82857 – Streamline strtod and fix some related problems

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

Streamline dtoa and fix some related problems

[Darin Adler]

Not dtoa, strtod.

[Darin Adler]

Created attachment 135031 [details]
Patch

[WebKit Review Bot]

Attachment 135031 [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/WTF/wtf/dtoa.h:53:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WTF/wtf/dtoa/double-conversion.h:367:  processed_characters_count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

I got started on this because I was using Instruments to profile web page loading and the strlen inside strtod showed up on a profile.

[Darin Adler]

Created attachment 135033 [details]
Patch

[WebKit Review Bot]

Attachment 135033 [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/WTF/wtf/dtoa.h:53:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WTF/wtf/dtoa/double-conversion.h:367:  processed_characters_count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 135033 [details]
Patch

Attachment 135033 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12307505

[Darin Adler]

I’ll need some help making the .def file changes for Windows.

[Mark Hahnenberg]

(In reply to comment #8)
> I’ll need some help making the .def file changes for Windows.

The symbols listed in the build failure just need to be removed from JavaScriptCore.def I believe:

??$strtod@$00$00@WTF@@YANPBDPAPAD@Z
??$strtod@$00$0A@@WTF@@YANPBDPAPAD@Z
??$strtod@$0A@$00@WTF@@YANPBDPAPAD@Z
??$strtod@$0A@$0A@@WTF@@YANPBDPAPAD@Z

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #8)
> > I’ll need some help making the .def file changes for Windows.
> 
> The symbols listed in the build failure just need to be removed from JavaScriptCore.def I believe:
> 
> ??$strtod@$00$00@WTF@@YANPBDPAPAD@Z
> ??$strtod@$00$0A@@WTF@@YANPBDPAPAD@Z
> ??$strtod@$0A@$00@WTF@@YANPBDPAPAD@Z
> ??$strtod@$0A@$0A@@WTF@@YANPBDPAPAD@Z

Thanks. You’re right, that’s step one. Then I will need to add new symbols to that .def file.

[Darin Adler]

Created attachment 135746 [details]
Patch

[WebKit Review Bot]

Attachment 135746 [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/WTF/wtf/dtoa.h:53:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WTF/wtf/dtoa/double-conversion.h:367:  processed_characters_count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 135746 [details]
Patch

Attachment 135746 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12339089

[Darin Adler]

Created attachment 135823 [details]
Patch

[WebKit Review Bot]

Attachment 135823 [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/WTF/wtf/dtoa.h:53:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WTF/wtf/dtoa/double-conversion.h:367:  processed_characters_count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 135823 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 135823 [details]
Patch

Clearing flags on attachment: 135823

Committed r113454: <http://trac.webkit.org/changeset/113454>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

(In reply to comment #18)
> All reviewed patches have been landed.  Closing bug.

We're now hitting the following assertion on the GTK+ debug build. It seems related.


#0  0x00007f73893258d0 in WebCore::parseToDoubleForNumberType (string="1.2E65535", result=0x0) at ../../Source/WebCore/html/parser/HTMLParserIdioms.cpp:82
82	    ASSERT(isfinite(value));

#0  0x00007f73893258d0 in WebCore::parseToDoubleForNumberType (string="1.2E65535", result=0x0) at ../../Source/WebCore/html/parser/HTMLParserIdioms.cpp:82
#1  0x00007f738931391e in WebCore::NumberInputType::sanitizeValue (this=0x3e2b060, proposedValue="1.2E65535") at ../../Source/WebCore/html/NumberInputType.cpp:327
#2  0x00007f73892b45a0 in WebCore::HTMLInputElement::sanitizeValue (this=0x3a6e700, proposedValue="1.2E65535") at ../../Source/WebCore/html/HTMLInputElement.cpp:1384
#3  0x00007f73892b2e6e in WebCore::HTMLInputElement::setValue (this=0x3a6e700, value="1.2E65535", eventBehavior=WebCore::DispatchNoEvent) at ../../Source/WebCore/html/HTMLInputElement.cpp:1037
#4  0x00007f7389d17a44 in WebCore::setJSHTMLInputElementValue (exec=0x7f733c109090, thisObject=0x7f733c0da4a0, value=...) at DerivedSources/WebCore/JSHTMLInputElement.cpp:964
#5  0x00007f7389d196e1 in JSC::lookupPut<WebCore::JSHTMLInputElement> (exec=0x7f733c109090, propertyName=, value=..., table=0x7f738c6804b0, thisObj=0x7f733c0da4a0, shouldThrow=false) at ../../Source/JavaScriptCore/runtime/Lookup.h:362
#6  0x00007f7389d1940d in JSC::lookupPut<WebCore::JSHTMLInputElement, WebCore::JSHTMLElement> (exec=0x7f733c109090, propertyName=, value=..., table=0x7f738c6804b0, thisObj=0x7f733c0da4a0, slot=...) at ../../Source/JavaScriptCore/runtime/Lookup.h:378
#7  0x00007f7389d16bfc in WebCore::JSHTMLInputElement::put (cell=0x7f733c0da4a0, exec=0x7f733c109090, propertyName=, value=..., slot=...) at DerivedSources/WebCore/JSHTMLInputElement.cpp:731
#8  0x00007f738d148cfc in JSC::JSValue::put (this=0x7ffffcd57510, exec=0x7f733c109090, propertyName=, value=..., slot=...) at ../../Source/JavaScriptCore/runtime/JSObject.h:840
#9  0x00007f738d1f14a6 in JSC::cti_op_put_by_id_generic (args=0x7ffffcd57550) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:1391
#10 0x00007f738d1f0b66 in JSC::JITThunks::tryCacheGetByID (callFrame=0x7ffffcd57500, codeBlock=0x7f733c0d9ea0, returnAddress=..., baseValue=..., propertyName=, slot=..., stubInfo=0x17104f0) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:970
#11 0x00007f738d1c897f in JSC::JITCode::execute (this=0x7f733c05f3f8, registerFile=0x1cf08a8, callFrame=0x7f733c109038, globalData=0x17104f0) at ../../Source/JavaScriptCore/jit/JITCode.h:127
#12 0x00007f738d1c53f3 in JSC::Interpreter::execute (this=0x1cf0890, program=0x7f733c05f3e0, callFrame=0x7f733c0bf7e8, scopeChain=0x7f733c0df3a0, thisObj=0x7f733c0dffa0) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:1198
#13 0x00007f738d27f538 in JSC::evaluate (exec=0x7f733c0bf7e8, scopeChain=0x7f733c0df3a0, source=..., thisValue=..., returnedException=0x7ffffcd58ba0) at ../../Source/JavaScriptCore/runtime/Completion.cpp:73
#14 0x00007f7388e684b0 in WebCore::JSMainThreadExecState::evaluate (exec=0x7f733c0bf7e8, chain=0x7f733c0df3a0, source=..., thisValue=..., exception=0x7ffffcd58ba0) at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:76
#15 0x00007f7388eac66d in WebCore::ScriptController::evaluateInWorld (this=0x16ec558, sourceCode=..., world=0x1cf4bc0) at ../../Source/WebCore/bindings/js/ScriptController.cpp:145
#16 0x00007f7388eac77a in WebCore::ScriptController::evaluate (this=0x16ec558, sourceCode=...) at ../../Source/WebCore/bindings/js/ScriptController.cpp:162
#17 0x00007f738915fd68 in WebCore::ScriptElement::executeScript (this=0x39b51a8, sourceCode=...) at ../../Source/WebCore/dom/ScriptElement.cpp:290
#18 0x00007f738915f83f in WebCore::ScriptElement::prepareScript (this=0x39b51a8, scriptStartPosition=..., supportLegacyTypes=WebCore::ScriptElement::DisallowLegacyTypeInTypeAttribute) at ../../Source/WebCore/dom/ScriptElement.cpp:235
#19 0x00007f7389328b46 in WebCore::HTMLScriptRunner::runScript (this=0x39bfa00, script=0x39b5130, scriptStartPosition=...) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:296
#20 0x00007f7389328165 in WebCore::HTMLScriptRunner::execute (this=0x39bfa00, scriptElement=..., scriptStartPosition=...) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:170
#21 0x00007f738931a486 in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder (this=0x3d79920) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:207
#22 0x00007f738931a543 in WebCore::HTMLDocumentParser::canTakeNextToken (this=0x3d79920, mode=WebCore::HTMLDocumentParser::AllowYield, session=...) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:225
#23 0x00007f738931a97d in WebCore::HTMLDocumentParser::pumpTokenizer (this=0x3d79920, mode=WebCore::HTMLDocumentParser::AllowYield) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:263
#24 0x00007f738931a2d5 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible (this=0x3d79920, mode=WebCore::HTMLDocumentParser::AllowYield) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:177
#25 0x00007f738931af26 in WebCore::HTMLDocumentParser::append (this=0x3d79920, source=...) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:370
#26 0x00007f73890942cc in WebCore::DecodedDataDocumentParser::appendBytes (this=0x3d79920, writer=0x3c3cdc0, data=0x3ef9620 "<!DOCTYPE html>\n<html>\n<head>\n<script src=\"../../../fast/js/resources/js-test-pre.js\"></script>\n</head>\n<body>\n<script>\ndescription('This test aims to check for typeMismatch flag with type=number inpu"..., length=2069) at ../../Source/WebCore/dom/DecodedDataDocumentParser.cpp:50
#27 0x00007f73894b26e6 in WebCore::DocumentWriter::addData (this=0x3c3cdc0, bytes=0x3ef9620 "<!DOCTYPE html>\n<html>\n<head>\n<script src=\"../../../fast/js/resources/js-test-pre.js\"></script>\n</head>\n<body>\n<script>\ndescription('This test aims to check for typeMismatch flag with type=number inpu"..., length=2069) at ../../Source/WebCore/loader/DocumentWriter.cpp:218
#28 0x00007f73894a666e in WebCore::DocumentLoader::commitData (this=0x3c3cca0, bytes=0x3ef9620 "<!DOCTYPE html>\n<html>\n<head>\n<script src=\"../../../fast/js/resources/js-test-pre.js\"></script>\n</head>\n<body>\n<script>\ndescription('This test aims to check for typeMismatch flag with type=number inpu"..., length=2069) at ../../Source/WebCore/loader/DocumentLoader.cpp:324
#29 0x00007f7388c492da in WebKit::FrameLoaderClient::committedLoad (this=0x16cbc90, loader=0x3c3cca0, data=0x3ef9620 "<!DOCTYPE html>\n<html>\n<head>\n<script src=\"../../../fast/js/resources/js-test-pre.js\"></script>\n</head>\n<body>\n<script>\ndescription('This test aims to check for typeMismatch flag with type=number inpu"..., length=2069) at ../../Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:170
#30 0x00007f73894a6541 in WebCore::DocumentLoader::commitLoad (this=0x3c3cca0, data=0x3ef9620 "<!DOCTYPE html>\n<html>\n<head>\n<script src=\"../../../fast/js/resources/js-test-pre.js\"></script>\n</head>\n<body>\n<script>\ndescription('This test aims to check for typeMismatch flag with type=number inpu"..., length=2069) at ../../Source/WebCore/loader/DocumentLoader.cpp:310
#31 0x00007f73894a6728 in WebCore::DocumentLoader::receivedData (this=0x3c3cca0, data=0x3ef9620 "<!DOCTYPE html>\n<html>\n<head>\n<script src=\"../../../fast/js/resources/js-test-pre.js\"></script>\n</head>\n<body>\n<script>\ndescription('This test aims to check for typeMismatch flag with type=number inpu"..., length=2069) at ../../Source/WebCore/loader/DocumentLoader.cpp:336
#32 0x00007f73894f2cc7 in WebCore::MainResourceLoader::addData (this=0x3c84740, data=0x3ef9620 "<!DOCTYPE html>\n<html>\n<head>\n<script src=\"../../../fast/js/resources/js-test-pre.js\"></script>\n</head>\n<body>\n<script>\ndescription('This test aims to check for typeMismatch flag with type=number inpu"..., length=2069, allAtOnce=false) at ../../Source/WebCore/loader/MainResourceLoader.cpp:175
#33 0x00007f738950043f in WebCore::ResourceLoader::didReceiveData (this=0x3c84740, data=0x3ef9620 "<!DOCTYPE html>\n<html>\n<head>\n<script src=\"../../../fast/js/resources/js-test-pre.js\"></script>\n</head>\n<body>\n<script>\ndescription('This test aims to check for typeMismatch flag with type=number inpu"..., length=2069, encodedDataLength=2069, allAtOnce=false) at ../../Source/WebCore/loader/ResourceLoader.cpp:288
#34 0x00007f73894f411a in WebCore::MainResourceLoader::didReceiveData (this=0x3c84740, data=0x3ef9620 "<!DOCTYPE html>\n<html>\n<head>\n<script src=\"../../../fast/js/resources/js-test-pre.js\"></script>\n</head>\n<body>\n<script>\ndescription('This test aims to check for typeMismatch flag with type=number inpu"..., length=2069, encodedDataLength=2069, allAtOnce=false) at ../../Source/WebCore/loader/MainResourceLoader.cpp:460
#35 0x00007f7389500d52 in WebCore::ResourceLoader::didReceiveData (this=0x3c84740, data=0x3ef9620 "<!DOCTYPE html>\n<html>\n<head>\n<script src=\"../../../fast/js/resources/js-test-pre.js\"></script>\n</head>\n<body>\n<script>\ndescription('This test aims to check for typeMismatch flag with type=number inpu"..., length=2069, encodedDataLength=2069) at ../../Source/WebCore/loader/ResourceLoader.cpp:442
#36 0x00007f73896a47d2 in WebCore::readCallback (source=0x1ebaf60, asyncResult=0x1f022a0, data=0x3cfc0b0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:679
#37 0x00007f73868deb6f in async_ready_callback_wrapper () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libgio-2.0.so.0
#38 0x00007f73868f5a83 in g_simple_async_result_complete () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libgio-2.0.so.0
#39 0x00007f73868f5c4f in complete_in_idle_cb_for_thread () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libgio-2.0.so.0
#40 0x00007f73867813f3 in g_idle_dispatch () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libglib-2.0.so.0
#41 0x00007f738677ec8a in g_main_dispatch () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libglib-2.0.so.0
#42 0x00007f738677f950 in g_main_context_dispatch () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libglib-2.0.so.0
#43 0x00007f738677fb3a in g_main_context_iterate () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libglib-2.0.so.0
#44 0x00007f738677ff70 in g_main_loop_run () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libglib-2.0.so.0
#45 0x00007f7387656e99 in gtk_main () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Dependencies/Root/lib64/libgtk-3.so.0
#46 0x000000000045b2cc in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:705
#47 0x000000000045a944 in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:492
#48 0x000000000045d960 in main (argc=2, argv=0x7ffffcd5a268) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1387

It seems like it might be related.

[Darin Adler]

Not sure why this is happening on GTK but not Mac; I ran the regression tests on my Mac with a debug build.

I am not at a computer with a source tree so I can’t fix this right now, but the fix is to roll back the change to HTMLParserIdioms.cpp, so please feel free to do that.

[James Simonsen]

(In reply to comment #20)
> Not sure why this is happening on GTK but not Mac; I ran the regression tests on my Mac with a debug build.
> 
> I am not at a computer with a source tree so I can’t fix this right now, but the fix is to roll back the change to HTMLParserIdioms.cpp, so please feel free to do that.

Chrome is hitting the same DCHECK on all 3 platforms. It's while running fast/forms/number/ValidityState-typeMismatch-number.html

I'll look into rolling out the HTMLParserIdioms.cpp change.

[James Simonsen]

(In reply to comment #21)
> I'll look into rolling out the HTMLParserIdioms.cpp change.

Rolled out here: https://bugs.webkit.org/show_bug.cgi?id=83402