Bug 150192 – Null dereference loading Blink layout test fast/forms/color/input-color-onchange-event.html

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jon Honeycutt]

Created attachment 263199 [details]
crashing test

Null dereference loading Blink layout test fast/forms/color/input-color-onchange-event.html.


Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGABRT)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000014

VM Regions Near 0x14:
--> 
    __TEXT                 0000000107e85000-0000000107f1f000 [  616K] r-x/rwx SM=COW  /Users/USER/*

Application Specific Information:
CRASHING TEST: blink-tests-that-are-different/fast/forms/color/input-color-onchange-event.html
================================================================
==25562==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000014 (pc 0x00010801aec2 bp 0x7fff57d73650 sp 0x7fff57d73650 T0)
    #0 0x10801aec1 in WebCore::Node::getFlag(WebCore::Node::NodeFlags) const Node.h:641
    #1 0x108020c31 in WebCore::Node::hasTagName(WebCore::HTMLQualifiedName const&) const HTMLElement.h:161
    #2 0x108007d06 in WebCore::Internals::selectColorInColorChooser(WebCore::Element*, WTF::String const&) Internals.cpp:892
    #3 0x10804fc6a in WebCore::jsInternalsPrototypeFunctionSelectColorInColorChooser(JSC::ExecState*) JSInternals.cpp:1624
    #4 0x5948e0601027  (<unknown module>)
    #5 0x108cb864f in llint_entry (/Users/jhoneycutt/src/OpenSource/WebKitBuild2/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore+0xab464f)
    #6 0x108cb2a0a in vmEntryToJavaScript (/Users/jhoneycutt/src/OpenSource/WebKitBuild2/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore+0xaaea0a)
    #7 0x108a1407d in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) JITCode.cpp:80
    #8 0x1089ca17f in JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*) Interpreter.cpp:1269
    #9 0x1089c9542 in JSC::eval(JSC::ExecState*) Interpreter.cpp:182
    #10 0x108cab0f6 in llint_slow_path_call_eval LLIntSlowPaths.cpp:1312
    #11 0x108cb8cd9 in llint_entry (/Users/jhoneycutt/src/OpenSource/WebKitBuild2/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore+0xab4cd9)
    #12 0x108cb864f in llint_entry (/Users/jhoneycutt/src/OpenSource/WebKitBuild2/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore+0xab464f)
    #13 0x108cb2a0a in vmEntryToJavaScript (/Users/jhoneycutt/src/OpenSource/WebKitBuild2/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore+0xaaea0a)
    #14 0x108a1407d in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) JITCode.cpp:80
    #15 0x1089d0cc6 in JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) Interpreter.cpp:961
    #16 0x108393689 in JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) Completion.cpp:104
    #17 0x10e97e3ad in WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) JSMainThreadExecState.h:62
    #18 0x10f5ba410 in WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) ScriptController.cpp:164
    #19 0x10f5ba618 in WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) ScriptController.cpp:180
    #20 0x10f5cc586 in WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) ScriptElement.cpp:309
    #21 0x10f5c9e6a in WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) ScriptElement.cpp:242
    #22 0x10e2bf9cb in WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) HTMLScriptRunner.cpp:308
    #23 0x10e2bf705 in WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) HTMLScriptRunner.cpp:177
    #24 0x10e1eaa6f in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() HTMLDocumentParser.cpp:195
    #25 0x10e1eace3 in WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) HTMLDocumentParser.cpp:213
    #26 0x10e1ea2a8 in WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) HTMLDocumentParser.cpp:259
    #27 0x10e1ebc9d in WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution() HTMLDocumentParser.cpp:496
    #28 0x10e1ebf61 in WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) HTMLDocumentParser.cpp:536
    #29 0x10d80dca7 in WebCore::CachedResource::checkNotify() CachedResource.cpp:297
    #30 0x10f896588 in WebCore::SubresourceLoader::didFinishLoading(double) SubresourceLoader.cpp:372
    #31 0x7fff8c4a3850 in __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke (/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork+0x2e850)
    #32 0x7fff8c4a3765 in -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] (/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork+0x2e765)
    #33 0x7fff8c4a366a in -[NSURLConnectionInternal _withActiveConnectionAndDelegate:] (/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork+0x2e66a)
    #34 0x7fff8c4a8491 in ___ZN27URLConnectionClient_Classic26_delegate_didFinishLoadingEU13block_pointerFvvE_block_invoke (/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork+0x33491)
    #35 0x7fff8c63c976 in ___ZN27URLConnectionClient_Classic18_withDelegateAsyncEPKcU13block_pointerFvP16_CFURLConnectionPK33CFURLConnectionClientCurrent_VMaxE_block_invoke_2 (/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork+0x1c7976)
    #36 0x7fff9a99c3c2 in _dispatch_client_callout (/usr/lib/system/libdispatch.dylib+0x23c2)
    #37 0x7fff9a9aa0bd in _dispatch_block_invoke (/usr/lib/system/libdispatch.dylib+0x100bd)
    #38 0x7fff8c4a3527 in RunloopBlockContext::_invoke_block(void const*, void*) (/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork+0x2e527)
    #39 0x7fff96f5ce63 in CFArrayApplyFunction (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x4ce63)
    #40 0x7fff8c4a3420 in RunloopBlockContext::perform() (/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork+0x2e420)
    #41 0x7fff8c4a32c1 in MultiplexerSource::perform() (/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork+0x2e2c1)
    #42 0x7fff8c4a30e3 in MultiplexerSource::_perform(void*) (/System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork+0x2e0e3)
    #43 0x7fff96fba8b0 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0xaa8b0)
    #44 0x7fff96f9a0ab in __CFRunLoopDoSources0 (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x8a0ab)
    #45 0x7fff96f995ce in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x895ce)
    #46 0x7fff96f98fc7 in CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x88fc7)
    #47 0x107ea798d in runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) DumpRenderTree.mm:2030
    #48 0x107ea6f39 in runTestingServerLoop() DumpRenderTree.mm:1180
    #49 0x107ea6267 in dumpRenderTree(int, char const**) DumpRenderTree.mm:1288
    #50 0x107ea82b1 in DumpRenderTreeMain(int, char const**) DumpRenderTree.mm:1418
    #51 0x7fff931e95ac in start (/usr/lib/system/libdyld.dylib+0x35ac)
    #52 0x1  (<unknown module>)

[Radar WebKit Bug Importer]

<rdar://problem/23135050>

[Chris Dumez]

Created attachment 263378 [details]
Patch

[Darin Adler]

Comment on attachment 263378 [details]
Patch

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

I am saying review+ but I do have doubts.

> Source/WebCore/ChangeLog:18
> +        - Tightens the IDL parameter type to HTMLInputElement instead of Element
> +          as this is the expected type.
> +        - Adds [StrictTypeChecking] IDL extended attribute so that the JS bindings
> +          will take care of validating the input type and throw an exception for us.

Why not just do the null check instead of making all these other changes? I agree they make the function in Internals slightly more elegant, but is it worth the cost?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:975
> +            push(@headerContent, "    WEBCORE_EXPORT static RefPtr<NodeFilter> toWrapped(JSC::VM&, JSC::JSValue);\n");

Wow, all these new WEBCORE_EXPORT on all bindings mean we are going to be exporting a lot more functions, I think! Will that have a cost that we would like to avoid in production code? Are these exports needed only for testing?

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 263378 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263378&action=review
> 
> I am saying review+ but I do have doubts.
> 
> > Source/WebCore/ChangeLog:18
> > +        - Tightens the IDL parameter type to HTMLInputElement instead of Element
> > +          as this is the expected type.
> > +        - Adds [StrictTypeChecking] IDL extended attribute so that the JS bindings
> > +          will take care of validating the input type and throw an exception for us.
> 
> Why not just do the null check instead of making all these other changes? I
> agree they make the function in Internals slightly more elegant, but is it
> worth the cost?
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:975
> > +            push(@headerContent, "    WEBCORE_EXPORT static RefPtr<NodeFilter> toWrapped(JSC::VM&, JSC::JSValue);\n");
> 
> Wow, all these new WEBCORE_EXPORT on all bindings mean we are going to be
> exporting a lot more functions, I think! Will that have a cost that we would
> like to avoid in production code? Are these exports needed only for testing?

Ok. This is indeed a lot of overhead (extra exports) just for an internals function. I'll do a simple null-check.
I don't think we even need to throw an exception. Throwing is the expected behavior from an IDL standpoint and this is what the blink test expects as well.
However, given that this is an internals function, I don't think it is worth the extra code.

[Chris Dumez]

Created attachment 263395 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 263395 [details]
Patch

Clearing flags on attachment: 263395

Committed r191294: <http://trac.webkit.org/changeset/191294>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.