Bug 51063 - REGRESSION (WebKit2): No context menu appears when right-clicking on windowless Flash plugin
Summary: REGRESSION (WebKit2): No context menu appears when right-clicking on windowle...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL: http://cuteoverload.com/2010/12/12/ho...
Keywords: InRadar, PlatformOnly
Depends on: 59102
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-14 15:25 PST by Adam Roben (:aroben)
Modified: 2011-04-22 10:01 PDT (History)
3 users (show)

See Also:


Attachments
Add code to enumerate all the functions imported by a particular Windows binary (32.31 KB, patch)
2011-04-21 12:51 PDT, Adam Roben (:aroben)
sam: review+
Details | Formatted Diff | Diff
Give windowless plugins' context menus an owner window in the same thread as the plugin (15.59 KB, patch)
2011-04-21 12:52 PDT, Adam Roben (:aroben)
bweinstein: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-12-14 15:25:42 PST
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.
Comment 1 Adam Roben (:aroben) 2010-12-14 15:27:00 PST
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
Comment 2 Adam Roben (:aroben) 2010-12-14 15:27:42 PST
<rdar://problem/8769281>
Comment 3 Adam Roben (:aroben) 2011-04-11 14:57:30 PDT
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.
Comment 4 Adam Roben (:aroben) 2011-04-11 15:00:13 PDT
Heh, I should read my own comments before writing new ones. :-)
Comment 5 Adam Roben (:aroben) 2011-04-11 15:00:45 PDT
Interestingly, some windowless Flash plugins do show context menus (e.g., <http://www.communitymx.com/content/source/E5141/wmodetrans.htm>).
Comment 6 Adam Roben (:aroben) 2011-04-11 15:40:04 PDT
http://msdn.microsoft.com/en-us/library/ms809762.aspx seems very relevant if we are to take the Import Address Table-hooking approach.
Comment 7 Adam Roben (:aroben) 2011-04-12 06:18:12 PDT
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
Comment 9 Adam Roben (:aroben) 2011-04-14 14:35:38 PDT
(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.
Comment 10 Adam Roben (:aroben) 2011-04-20 10:13:33 PDT
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!
Comment 11 Adam Roben (:aroben) 2011-04-21 12:51:43 PDT
Created attachment 90581 [details]
Add code to enumerate all the functions imported by a particular Windows binary
Comment 12 Adam Roben (:aroben) 2011-04-21 12:52:51 PDT
Created attachment 90582 [details]
Give windowless plugins' context menus an owner window in the same thread as the plugin
Comment 13 Jeff Miller 2011-04-21 13:21:04 PDT
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 14 Jeff Miller 2011-04-21 13:29:38 PDT
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 15 Adam Roben (:aroben) 2011-04-21 13:31:38 PDT
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 16 Adam Roben (:aroben) 2011-04-21 13:32:11 PDT
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 17 Brian Weinstein 2011-04-21 13:33:19 PDT
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 18 Adam Roben (:aroben) 2011-04-21 13:36:11 PDT
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.
Comment 19 Build Bot 2011-04-21 13:42:26 PDT
Attachment 90582 [details] did not build on win:
Build output: http://queues.webkit.org/results/8498099
Comment 20 Adam Roben (:aroben) 2011-04-21 13:43:54 PDT
(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 21 Brian Weinstein 2011-04-21 13:44:55 PDT
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 22 Adam Roben (:aroben) 2011-04-22 09:35:53 PDT
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.