To reproduce: 1. Go to http://cuteoverload.com/2010/12/12/home-for-the-holidays/ 2. Right-click on the video No context menu appears. In WebKit1, a context menu appears.
Chromium patches the ::TrackPopupMenu API [1]; maybe we need to do this, too. 1. http://www.google.com/codesearch/p?hl=en#hfE6470xZHk/webkit/glue/plugins/webplugin_delegate_impl_win.cc&q=trackpopupmenupatch&sa=N&cd=1&ct=rc
<rdar://problem/8769281>
My guess is that ::TrackPopupMenu is failing due to it being passed a window that's in another process. I think Chrome and Firefox both have code to patch ::TrackPopupMenu so a different window will get used.
Heh, I should read my own comments before writing new ones. :-)
Interestingly, some windowless Flash plugins do show context menus (e.g., <http://www.communitymx.com/content/source/E5141/wmodetrans.htm>).
http://msdn.microsoft.com/en-us/library/ms809762.aspx seems very relevant if we are to take the Import Address Table-hooking approach.
Other links explaining the PE format: http://msdn.microsoft.com/en-us/magazine/cc301805.aspx http://msdn.microsoft.com/en-us/windows/hardware/gg463119.aspx
(In reply to comment #7) > http://msdn.microsoft.com/en-us/magazine/cc301805.aspx Part 2: http://msdn.microsoft.com/en-us/magazine/cc301808.aspx
(In reply to comment #5) > Interestingly, some windowless Flash plugins do show context menus (e.g., <http://www.communitymx.com/content/source/E5141/wmodetrans.htm>). Ah, on that page Flash is passing its own hidden top-level HWND to ::TrackPopupMenu. But on other pages it passes the WKView's HWND.
I now have code that can hook an imported function for a given module. Now I just need to wire it up for windowless plugins!
Created attachment 90581 [details] Add code to enumerate all the functions imported by a particular Windows binary
Created attachment 90582 [details] Give windowless plugins' context menus an owner window in the same thread as the plugin
Comment on attachment 90581 [details] Add code to enumerate all the functions imported by a particular Windows binary View in context: https://bugs.webkit.org/attachment.cgi?id=90581&action=review > Source/WebCore/platform/win/DelayLoadedModulesEnumerator.cpp:80 > + // relative. Seems like this comment could be all on one line. > Source/WebCore/platform/win/ImportedModulesEnumeratorBase.h:42 > + ~ImportedModulesEnumeratorBase() { } ~ImportedModulesEnumeratorBase() should be virtual. > Source/WebCore/platform/win/PEImage.cpp:54 > +{ Maybe you should ASSERT(isValid()) here, and/or return 0 if !isValid().
Comment on attachment 90582 [details] Give windowless plugins' context menus an owner window in the same thread as the plugin View in context: https://bugs.webkit.org/attachment.cgi?id=90582&action=review > Source/WebKit2/Platform/win/ModuleWin.cpp:53 > +static void overwriteReadOnlyMemory(void* destination, const void* source, size_t size) Perhaps using memcpy in the function name would be clearer, especially since the parameter list is the same. memcpyToReadOnlyMemory()?
Comment on attachment 90581 [details] Add code to enumerate all the functions imported by a particular Windows binary View in context: https://bugs.webkit.org/attachment.cgi?id=90581&action=review >> Source/WebCore/platform/win/DelayLoadedModulesEnumerator.cpp:80 >> + // relative. > > Seems like this comment could be all on one line. Fixed. >> Source/WebCore/platform/win/ImportedModulesEnumeratorBase.h:42 >> + ~ImportedModulesEnumeratorBase() { } > > ~ImportedModulesEnumeratorBase() should be virtual. You are so right! >> Source/WebCore/platform/win/PEImage.cpp:54 >> +{ > > Maybe you should ASSERT(isValid()) here, and/or return 0 if !isValid(). This function can operate just fine on invalid modules, though of course it is meaningless. I think I'll add an assertion to try to bring the meaninglessness to people's attention.
Comment on attachment 90582 [details] Give windowless plugins' context menus an owner window in the same thread as the plugin View in context: https://bugs.webkit.org/attachment.cgi?id=90582&action=review >> Source/WebKit2/Platform/win/ModuleWin.cpp:53 >> +static void overwriteReadOnlyMemory(void* destination, const void* source, size_t size) > > Perhaps using memcpy in the function name would be clearer, especially since the parameter list is the same. memcpyToReadOnlyMemory()? Good idea. I'll use that name.
Comment on attachment 90582 [details] Give windowless plugins' context menus an owner window in the same thread as the plugin View in context: https://bugs.webkit.org/attachment.cgi?id=90582&action=review > Source/WebKit2/WebProcess/Plugins/Netscape/win/NetscapePluginWin.cpp:32 > +#include <WebCore/WebCoreInstanceHandle.h> This seems to be sorted wrong. Not sure why style bot didn't catch it. > Source/WebKit2/WebProcess/Plugins/Netscape/win/NetscapePluginWin.cpp:354 > + hWnd = currentPlugin->m_contextMenuOwnerWindow; It seems a bit strange to overwrite the hWnd argument here instead of using a local, but that's probably just because I'm not used to seeing it.
Comment on attachment 90582 [details] Give windowless plugins' context menus an owner window in the same thread as the plugin View in context: https://bugs.webkit.org/attachment.cgi?id=90582&action=review >> Source/WebKit2/WebProcess/Plugins/Netscape/win/NetscapePluginWin.cpp:32 >> +#include <WebCore/WebCoreInstanceHandle.h> > > This seems to be sorted wrong. Not sure why style bot didn't catch it. Hm, true! I'll fix it. >> Source/WebKit2/WebProcess/Plugins/Netscape/win/NetscapePluginWin.cpp:354 >> + hWnd = currentPlugin->m_contextMenuOwnerWindow; > > It seems a bit strange to overwrite the hWnd argument here instead of using a local, but that's probably just because I'm not used to seeing it. I don't personally find it strange. And overwriting the hWnd argument is what this function is all about! But I'm happy to change it if you think it would be more readable using a local.
Attachment 90582 [details] did not build on win: Build output: http://queues.webkit.org/results/8498099
(In reply to comment #19) > Attachment 90582 [details] did not build on win: > Build output: http://queues.webkit.org/results/8498099 That's because you didn't apply attachment 90581 [details] first!
Comment on attachment 90582 [details] Give windowless plugins' context menus an owner window in the same thread as the plugin View in context: https://bugs.webkit.org/attachment.cgi?id=90582&action=review >>> Source/WebKit2/WebProcess/Plugins/Netscape/win/NetscapePluginWin.cpp:354 >>> + hWnd = currentPlugin->m_contextMenuOwnerWindow; >> >> It seems a bit strange to overwrite the hWnd argument here instead of using a local, but that's probably just because I'm not used to seeing it. > > I don't personally find it strange. And overwriting the hWnd argument is what this function is all about! But I'm happy to change it if you think it would be more readable using a local. It's fine, I just am not as used to seeing that style.
Comment on attachment 90581 [details] Add code to enumerate all the functions imported by a particular Windows binary View in context: https://bugs.webkit.org/attachment.cgi?id=90581&action=review >>> Source/WebCore/platform/win/PEImage.cpp:54 >>> +{ >> >> Maybe you should ASSERT(isValid()) here, and/or return 0 if !isValid(). > > This function can operate just fine on invalid modules, though of course it is meaningless. I think I'll add an assertion to try to bring the meaninglessness to people's attention. Actually, I can't add that assertion because this function is called from the constructor, before the PEImage's validity has been determined.
Fixed in r84636 and r84638 http://trac.webkit.org/changeset/84636 http://trac.webkit.org/changeset/84638