Bug 142974 – Assertion firing in JavaScriptCore/parser/parser.h for statesman.com site

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Brent Fulgham]

After updating sources this morning and rebuilding, a test case for a bug I am working on now asserts in JavaScriptCore:

Test site:  <http://www.statesman.com/s/sports/>

Assertion:     ASSERT(m_source->startColumn() > 0); (Line 921 in JavaScriptCore/parser/Parser.h)

Call stack:
Thread 1Queue : com.apple.main-thread (serial)
#0	0x000000010d4aa6da in WTFCrash at /Volumes/Data/Projects/WebKit/OpenSource/Source/WTF/wtf/Assertions.cpp:321
#1	0x000000010cb320b9 in std::__1::unique_ptr<JSC::ProgramNode, std::__1::default_delete<JSC::ProgramNode> > JSC::Parser<JSC::Lexer<unsigned char> >::parse<JSC::ProgramNode>(JSC::ParserError&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/JavaScriptCore/parser/Parser.h:921
#2	0x000000010cb308fb in std::__1::unique_ptr<JSC::ProgramNode, std::__1::default_delete<JSC::ProgramNode> > JSC::parse<JSC::ProgramNode>(JSC::VM*, JSC::SourceCode const&, JSC::FunctionParameters*, JSC::Identifier const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::JSParserCodeType, JSC::ParserError&, JSC::JSTextPosition*, JSC::ConstructorKind) at /Volumes/Data/Projects/WebKit/OpenSource/Source/JavaScriptCore/parser/Parser.h:998
#3	0x000000010cbdde2f in JSC::UnlinkedProgramCodeBlock* JSC::CodeCache::getGlobalCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/JavaScriptCore/runtime/CodeCache.cpp:95
#4	0x000000010cbdd0df in JSC::CodeCache::getProgramCodeBlock(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/JavaScriptCore/runtime/CodeCache.cpp:125
#5	0x000000010d1477cf in JSC::JSGlobalObject::createProgramCodeBlock(JSC::ExecState*, JSC::ProgramExecutable*, JSC::JSObject**) at /Volumes/Data/Projects/WebKit/OpenSource/Source/JavaScriptCore/runtime/JSGlobalObject.cpp:789
#6	0x000000010cf13c1d in JSC::ProgramExecutable::initializeGlobalProperties(JSC::VM&, JSC::ExecState*, JSC::JSScope*) at /Volumes/Data/Projects/WebKit/OpenSource/Source/JavaScriptCore/runtime/Executable.cpp:496
#7	0x000000010d0a9f73 in JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) at /Volumes/Data/Projects/WebKit/OpenSource/Source/JavaScriptCore/interpreter/Interpreter.cpp:830
#8	0x000000010cc07f30 in JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) at /Volumes/Data/Projects/WebKit/OpenSource/Source/JavaScriptCore/runtime/Completion.cpp:83
#9	0x000000010fbd99f5 in WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/bindings/js/JSMainThreadExecState.h:62
#10	0x000000011061510d in WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/bindings/js/ScriptController.cpp:164
#11	0x0000000110615254 in WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/bindings/js/ScriptController.cpp:180
#12	0x00000001106243d7 in WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/dom/ScriptElement.cpp:301
#13	0x0000000110623360 in WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/dom/ScriptElement.cpp:237
#14	0x000000010f634c06 in WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/html/parser/HTMLScriptRunner.cpp:309
#15	0x000000010f634a09 in WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/html/parser/HTMLScriptRunner.cpp:178
#16	0x000000010f56f5d0 in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/html/parser/HTMLDocumentParser.cpp:195
#17	0x000000010f56f6d1 in WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/html/parser/HTMLDocumentParser.cpp:213
#18	0x000000010f56e9e8 in WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/html/parser/HTMLDocumentParser.cpp:259
#19	0x000000010f56e5c9 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/html/parser/HTMLDocumentParser.cpp:166
#20	0x000000010f570719 in WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/html/parser/HTMLDocumentParser.cpp:496
#21	0x000000010f570b18 in WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/html/parser/HTMLDocumentParser.cpp:536
#22	0x000000010f570b7f in non-virtual thunk to WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/html/parser/HTMLDocumentParser.cpp:537
#23	0x000000010ed1dd52 in WebCore::CachedResource::checkNotify() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:291
#24	0x000000010ed1de64 in WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:307
#25	0x000000010ed3cee1 in WebCore::CachedScript::finishLoading(WebCore::SharedBuffer*) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/loader/cache/CachedScript.cpp:86
#26	0x00000001108e0495 in WebCore::SubresourceLoader::didFinishLoading(double) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/loader/SubresourceLoader.cpp:364
#27	0x000000010a25bdac in WebKit::WebResourceLoader::didFinishResourceLoad(double) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:162
#28	0x000000010a261143 in void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double), std::__1::tuple<double>, 0ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double), std::__1::tuple<double>&&, std::index_sequence<0ul>) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit2/Platform/IPC/HandleMessage.h:16
#29	0x000000010a261098 in void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double), std::__1::tuple<double>, std::make_index_sequence<1ul> >(std::__1::tuple<double>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double)) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit2/Platform/IPC/HandleMessage.h:22
#30	0x000000010a26054d in void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double)>(IPC::MessageDecoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double)) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit2/Platform/IPC/HandleMessage.h:92
#31	0x000000010a25fc82 in WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) at /Volumes/Data/Projects/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit2/WebResourceLoaderMessageReceiver.cpp:71
#32	0x0000000109b75680 in WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp:60
#33	0x0000000109986b33 in IPC::Connection::dispatchMessage(IPC::MessageDecoder&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit2/Platform/IPC/Connection.cpp:847
#34	0x000000010997ef40 in IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit2/Platform/IPC/Connection.cpp:870
#35	0x000000010998712f in IPC::Connection::dispatchOneMessage() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit2/Platform/IPC/Connection.cpp:898
#36	0x000000010998881d in IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_9::operator()() const at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit2/Platform/IPC/Connection.cpp:841
#37	0x00000001099887ec in decltype(std::__1::forward<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_9&>(fp)(std::__1::forward<>(fp0))) std::__1::__invoke<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_9&>(IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_9&&&) [inlined] at /Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.10.xctoolchain/usr/bin/../include/c++/v1/__functional_base:413
#38	0x00000001099887db in std::__1::__function::__func<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_9, std::__1::allocator<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_9>, void ()>::operator()() at /Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.10.xctoolchain/usr/bin/../include/c++/v1/functional:1370
#39	0x000000010cff345a in std::__1::function<void ()>::operator()() const at /Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.10.xctoolchain/usr/bin/../include/c++/v1/functional:1755
#40	0x000000010d4e5551 in WTF::RunLoop::performWork() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WTF/wtf/RunLoop.cpp:119
#41	0x000000010d4e6724 in WTF::RunLoop::performWork(void*) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WTF/wtf/cf/RunLoopCF.cpp:38
#42	0x00007fff8d0daa01 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ ()
#43	0x00007fff8d0ccc5c in __CFRunLoopDoSources0 ()
#44	0x00007fff8d0cc1bf in __CFRunLoopRun ()
#45	0x00007fff8d0cbbd8 in CFRunLoopRunSpecific ()
#46	0x00007fff83b6a56f in RunCurrentEventLoopInMode ()
#47	0x00007fff83b6a2ea in ReceiveNextEventCommon ()
#48	0x00007fff83b6a12b in _BlockUntilNextEventMatchingListInModeWithFilter ()
#49	0x00007fff86677a7b in _DPSNextEvent ()
#50	0x00007fff86677028 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#51	0x00007fff8666ccb3 in -[NSApplication run] ()
#52	0x00007fff865e9424 in NSApplicationMain ()
#53	0x00007fff89ed7958 in _xpc_objc_main ()
#54	0x00007fff89ed9060 in xpc_main ()
#55	0x00000001056d8185 in main at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.Development.mm:162
#56	0x00007fff8c0345c9 in start ()

[Brent Fulgham]

*** Bug 142973 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

I think I caused this in <http://trac.webkit.org/changeset/181810>.

[Radar WebKit Bug Importer]

<rdar://problem/20263890>

[Geoffrey Garen]

It looks like the HTML parser sometimes -- including on this site -- produces crazy negative column numbers, and the sanitization that I relaxed in <http://trac.webkit.org/changeset/181810> used to protect us against them.

[Geoffrey Garen]

I don't really like the solution I came up with for attribute event listeners before, so I'll take this opportunity to fix them up a bit.

[Alexey Proskuryakov]

Also at http://www.theblaze.com/stories/2015/03/25/man-thought-he-had-to-worry-about-people-stealing-his-packages-but-was-surprised-by-what-he-caught-a-mail-carrier-doing-on-camera-instead/ (from bug 143054).

[Geoffrey Garen]

Created attachment 249520 [details]
Patch

[WebKit Commit Bot]

Attachment 249520 [details] did not pass style-queue:


ERROR: 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: 1 in 19 files


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

[Michael Saboff]

Comment on attachment 249520 [details]
Patch

Will the line number rebasing require changes to LayoutTests/inspector/debugger/{breakpoint-columns,breakpoint-scope,search-scripts} tests?  These use line number in the tests and expected output.

[Geoffrey Garen]

> Will the line number rebasing require changes to
> LayoutTests/inspector/debugger/{breakpoint-columns,breakpoint-scope,search-
> scripts} tests?  These use line number in the tests and expected output.

I didn't see any changes in those tests when I ran them.

For reasons that are subtle, the old way and the new way are equivalent for most line numbers -- it's just that the new way is more robust and catches some corner cases.

[Geoffrey Garen]

Created attachment 249527 [details]
Patch

[Geoffrey Garen]

Added a test specifically covering the behavior change I described in Comment 10.

[Geoffrey Garen]

Created attachment 249528 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 249528 [details]
Patch

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

r=me

> Source/JavaScriptCore/interpreter/StackVisitor.cpp:337
> +    if (codeBlock->ownerExecutable()->hasOverrideLineNo())
> +        line = codeBlock->ownerExecutable()->overrideLineNo();

Are the divot details important? If you have a multiline inline attribute, will this provide the expected line for messages/errors for each line? Test:

    <div onclick="console.log(1);
        console.log(2);
        console.log(3);">Click Me</div>

> Source/JavaScriptCore/runtime/Executable.h:363
> +    void setOverrideLineNo(int overrideLineNo) { m_overrideLineNo = overrideLineNo; }
> +    bool hasOverrideLineNo() { return m_overrideLineNo != -1; }
> +    int overrideLineNo() { return m_overrideLineNo;; }

Normally we avoid abbreviations. Especially weird ones like "No" instead of "Number", despite the pre-existing case, can we make the new versions "overrideLineNumber"?

Nit: The bottom two look like they can be made const.
Nit: Double semicolon on line 363.

> Source/WebCore/bindings/js/JSLazyEventListener.cpp:109
> +    // We want all errors to refer back to the line on which our attribute was
> +    // declared, regardless of any newlines in our JavaScript source text.
> +    int overrideLineNo = m_position.m_line.oneBasedInt();

Ahh, it seems your intent is to always be the line of the attribute and not just with an original offset starting from the the line of the attribute. That seems fine.

[Geoffrey Garen]

> Are the divot details important? If you have a multiline inline attribute,
> will this provide the expected line for messages/errors for each line? Test:
> 
>     <div onclick="console.log(1);
>         console.log(2);
>         console.log(3);">Click Me</div>

> > Source/WebCore/bindings/js/JSLazyEventListener.cpp:109
> > +    // We want all errors to refer back to the line on which our attribute was
> > +    // declared, regardless of any newlines in our JavaScript source text.
> > +    int overrideLineNo = m_position.m_line.oneBasedInt();
> 
> Ahh, it seems your intent is to always be the line of the attribute and not
> just with an original offset starting from the the line of the attribute.
> That seems fine.

Right. It would be nicer to get the exact right line in this multiline case, but that's very tricky because: (1) Newlines in JavaScript source text do not always break a line in the .html file; (2) JavaScript anonymous function rules add their own newlines. So, I think the best we can do for these attributes (and it's pretty good) is just to guarantee that we'll point you back at the location of the attribute.

[Geoffrey Garen]

> > Source/JavaScriptCore/runtime/Executable.h:363
> > +    void setOverrideLineNo(int overrideLineNo) { m_overrideLineNo = overrideLineNo; }
> > +    bool hasOverrideLineNo() { return m_overrideLineNo != -1; }
> > +    int overrideLineNo() { return m_overrideLineNo;; }
> 
> Normally we avoid abbreviations. Especially weird ones like "No" instead of
> "Number", despite the pre-existing case, can we make the new versions
> "overrideLineNumber"?

Yes. I'll take care of this in a follow-up style patch.

> Nit: The bottom two look like they can be made const.
> Nit: Double semicolon on line 363.

Fixed.

[Michael Saboff]

Comment on attachment 249528 [details]
Patch

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

r=me

> Source/WebCore/bindings/js/ScriptController.cpp:281
> +    // FIXME: If we are not currently parsing, we should use our current location
> +    // in JavaScript, to cover cases like "element.setAttribute('click', ...)".
> +
> +    // FIXME: This location maps to the end of the HTML tag, and not to the
> +    // exact column number belonging to the event handler attribute.

Add defects for these FIXME's.

[Geoffrey Garen]

Committed r182034: <http://trac.webkit.org/changeset/182034>