Bug 153835

Summary: [WTR] Crash in EventSendingController::contextClick() when context menu event is not handled
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, lforschler
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 153493    
Attachments:
Description Flags
Patch mrobinson: review+

Description Carlos Garcia Campos 2016-02-03 10:08:05 PST 
WKBundlePageCopyContextMenuAtPointInWindow() returns nullptr when the context menu event is not hanlded, but we are using the returned value without null checking it. This happened in EWS with a new test that will be introduced in bug #153493

CRASHING TEST: fast/events/contextmenu-on-scrollbars.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x000000010bd528a9 WKArrayGetSize + 9
1   WebKitTestRunnerInjectedBundle	0x00000001172386f7 0x117228000 + 67319
2   com.apple.JavaScriptCore      	0x000000010cf49bab long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*) + 571 (APICallbackFunction.h:61)
3   com.apple.JavaScriptCore      	0x000000010d071343 JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 595 (LLIntSlowPaths.cpp:1110)
4   com.apple.JavaScriptCore      	0x000000010d078157 llint_entry + 23679
5   com.apple.JavaScriptCore      	0x000000010d0722f5 vmEntryToJavaScript + 299
6   com.apple.JavaScriptCore      	0x000000010cefc9ae JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 158 (JITCode.cpp:81)
7   com.apple.JavaScriptCore      	0x000000010ce7b15e JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) + 10558 (Interpreter.cpp:972)
8   com.apple.JavaScriptCore      	0x000000010cb27d91 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 593 (Completion.cpp:105)
9   com.apple.WebCore             	0x000000010e56bef5 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) + 309 (JSMainThreadExecState.h:80)
10  com.apple.WebCore             	0x000000010e56c140 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ExceptionDetails*) + 48 (ScriptController.cpp:180)
11  com.apple.WebCore             	0x000000010e5721d4 WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) + 260 (ScriptElement.cpp:310)
12  com.apple.WebCore             	0x000000010e570ce5 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) + 1061 (StdLibExtras.h:350)
13  com.apple.WebCore             	0x000000010dd591d8 WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) + 344 (ScriptElement.h:59)
14  com.apple.WebCore             	0x000000010dd59030 WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) + 48 (HTMLScriptRunner.cpp:189)
15  com.apple.WebCore             	0x000000010dcf5f86 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() + 86 (StdLibExtras.h:350)
16  com.apple.WebCore             	0x000000010dcf604d WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) + 93 (HTMLDocumentParser.cpp:214)
17  com.apple.WebCore             	0x000000010dcf5c40 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 544 (HTMLDocumentParser.cpp:252)
18  com.apple.WebCore             	0x000000010dcf6990 WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl>&&) + 736 (DocumentParser.h:71)
19  com.apple.WebCore             	0x000000010da88725 WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter&, char const*, unsigned long) + 117 (StdLibExtras.h:350)
20  com.apple.WebCore             	0x000000010dadd731 WebCore::DocumentLoader::commitData(char const*, unsigned long) + 657 (DocumentLoader.cpp:890)
21  com.apple.WebKit              	0x000000010bc606c6 WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) + 50
22  com.apple.WebCore             	0x000000010dadf6d1 WebCore::DocumentLoader::commitLoad(char const*, int) + 145 (DocumentLoader.h:229)
23  com.apple.WebCore             	0x000000010d8eaa70 WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int) + 160 (CachedResourceClientWalker.h:51)
24  com.apple.WebCore             	0x000000010d8ea941 WebCore::CachedRawResource::addDataBuffer(WebCore::SharedBuffer&) + 145 (CachedRawResource.cpp:70)
25  com.apple.WebCore             	0x000000010e6b155a WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::PassRefPtr<WebCore::SharedBuffer>, long long, WebCore::DataPayloadType) + 218 (SubresourceLoader.cpp:300)
26  com.apple.WebCore             	0x000000010e6b1443 WebCore::SubresourceLoader::didReceiveData(char const*, unsigned int, long long, WebCore::DataPayloadType) + 35 (StdLibExtras.h:350)
27  com.apple.WebKit              	0x000000010bd287ff WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) + 291
28  com.apple.WebKit              	0x000000010bafc5bd IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) + 127
29  com.apple.WebKit              	0x000000010bafecc4 IPC::Connection::dispatchOneMessage() + 126
30  com.apple.JavaScriptCore      	0x000000010d2c9fd5 WTF::RunLoop::performWork() + 437 (functional:1742)
31  com.apple.JavaScriptCore      	0x000000010d2ca382 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
32  com.apple.CoreFoundation      	0x00007fff97fa6a01 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
33  com.apple.CoreFoundation      	0x00007fff97f98b8d __CFRunLoopDoSources0 + 269
34  com.apple.CoreFoundation      	0x00007fff97f981bf __CFRunLoopRun + 927
35  com.apple.CoreFoundation      	0x00007fff97f97bd8 CFRunLoopRunSpecific + 296
36  com.apple.HIToolbox           	0x00007fff9842856f RunCurrentEventLoopInMode + 235
37  com.apple.HIToolbox           	0x00007fff984282ea ReceiveNextEventCommon + 431
38  com.apple.HIToolbox           	0x00007fff9842812b _BlockUntilNextEventMatchingListInModeWithFilter + 71
39  com.apple.AppKit              	0x00007fff8c4718ab _DPSNextEvent + 978
40  com.apple.AppKit              	0x00007fff8c470e58 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 346
41  com.apple.AppKit              	0x00007fff8c466af3 -[NSApplication run] + 594
42  com.apple.AppKit              	0x00007fff8c3e3244 NSApplicationMain + 1832
43  libxpc.dylib                  	0x00007fff90bb8928 _xpc_objc_main + 793
44  libxpc.dylib                  	0x00007fff90bba030 xpc_main + 490
45  com.apple.WebKit.WebContent.Development	0x000000010bab4e78 main + 422 (XPCServiceMain.mm:114)
46  libdyld.dylib                 	0x00007fff90c0a5c9 start + 1
[reply] [-] Comment 11
Comment 1 Carlos Garcia Campos 2016-02-03 10:10:38 PST 
Created attachment 270583 [details]
Patch
Comment 2 Martin Robinson 2016-02-03 10:30:28 PST 
Comment on attachment 270583 [details]
Patch

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

> Tools/ChangeLog:9
> +        the context menu event is not hanlded, but we are using the

Nit: hanlded -> handled

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:558
> -    size_t entriesSize = WKArrayGetSize(menuEntries.get());
> +    size_t entriesSize = menuEntries ? WKArrayGetSize(menuEntries.get()) : 0;

Perhaps better to simply return early in this case?
Comment 3 Carlos Garcia Campos 2016-02-03 23:04:21 PST 
Comment on attachment 270583 [details]
Patch

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

Thanks for the review!

>> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:558
>> +    size_t entriesSize = menuEntries ? WKArrayGetSize(menuEntries.get()) : 0;
> 
> Perhaps better to simply return early in this case?

Yes, indeed.
Comment 4 Carlos Garcia Campos 2016-02-03 23:35:08 PST 
Committed r196110: <http://trac.webkit.org/changeset/196110>