Bug 51063 – REGRESSION (WebKit2): No context menu appears when right-clicking on windowless Flash plugin

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

<rdar://problem/8769281>

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

Heh, I should read my own comments before writing new ones. :-)

[Adam Roben (:aroben)]

Interestingly, some windowless Flash plugins do show context menus (e.g., <http://www.communitymx.com/content/source/E5141/wmodetrans.htm>).

[Adam Roben (:aroben)]

http://msdn.microsoft.com/en-us/library/ms809762.aspx seems very relevant if we are to take the Import Address Table-hooking approach.

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

(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

[Adam Roben (:aroben)]

(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.

[Adam Roben (:aroben)]

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!

[Adam Roben (:aroben)]

Created attachment 90581 [details]
Add code to enumerate all the functions imported by a particular Windows binary

[Adam Roben (:aroben)]

Created attachment 90582 [details]
Give windowless plugins' context menus an owner window in the same thread as the plugin

[Jeff Miller]

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().

[Jeff Miller]

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()?

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

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.

[Brian Weinstein]

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.

[Adam Roben (:aroben)]

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.

[Build Bot]

Attachment 90582 [details] did not build on win:
Build output: http://queues.webkit.org/results/8498099

[Adam Roben (:aroben)]

(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!

[Brian Weinstein]

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.

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

Fixed in r84636 and r84638

http://trac.webkit.org/changeset/84636
http://trac.webkit.org/changeset/84638