Bug 153493 – Do not show context menu when right clicking on a scrollbar

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Scrollbars don't currently handle right clicks, but we are showing the context menu when they are right clicked. This is not desired at least in GTK+ and I've checked that other applications in Mac don't show the context menu when right clicking on a scrollbar.

[Carlos Garcia Campos]

Created attachment 269890 [details]
Patch

[Michael Catanzaro]

Comment on attachment 269890 [details]
Patch

Isn't this testable?

[Carlos Garcia Campos]

Created attachment 270202 [details]
Add a test

[Build Bot]

Comment on attachment 270202 [details]
Add a test

Attachment 270202 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/755258

New failing tests:
fast/events/contextmenu-on-scrollbars.html

[Build Bot]

Created attachment 270204 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Carlos Garcia Campos]

I'm surprised it only crashed in mac wk2, could someone with a mac check if the tests fails even without the patch?

[Darin Adler]

Comment on attachment 270202 [details]
Add a test

This looks like it introduced crashes in WebKit2 tests on Mac.

[Carlos Garcia Campos]

(In reply to comment #7)
> Comment on attachment 270202 [details]
> Add a test
> 
> This looks like it introduced crashes in WebKit2 tests on Mac.

Could your confirm that the test doesn't crash with current trunk (even if it fails, of course). I'm surprised it only fails in wk2, and the changes introduced are cross-platform. The only thing the patch does differently is calling view->scrollbarAtPoint(event.position()). And view is null checked already. The other thing is that I remove the !mouseEvent.scrollbar() condition from the if below, because we are now returning early when having a scrollbar.

[Darin Adler]

(In reply to comment #8)
> Could your confirm that the test doesn't crash with current trunk (even if
> it fails, of course).

I have no idea. I was just looking at EWS output.

[Michael Catanzaro]

My guess is the crash is unrelated to your code change, and you just happened to find some test that crashes JSC.


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

[Carlos Garcia Campos]

Ok, I know what the problem is. The crash is in WTR, when handling contextClick. GTK and EFL use WKBundlePageCopyContextMenuItems that always returns a valid array, even if it's empty when there's not contexct menu, but other ports use WKBundlePageCopyContextMenuAtPointInWindow() that can return nullptr. The returned value is used below without any null check. I don't know why we need different code for EFL and GTK here, and I don't know why WebPage::contextMenuAtPointInWindow() can return nullptr, but WebPage::contextMenu() doesn't. But this is an exiting issue in any case so I'll open a new bug.

[Michael Catanzaro]

Comment on attachment 270202 [details]
Add a test

In that case, r=me, good job as usual.

[Carlos Garcia Campos]

Created attachment 270637 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r196112: <http://trac.webkit.org/changeset/196112>