665315 - OOPP Contextmenu loses messages

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Andrei Kurushin]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/14.0.794.0 Safari/535.1
Build Identifier: 

ipc framework peeks messages that belong to TrackPopupMenu modal loop. 
consequences:
- contextmenu items may "swallow" clicks (WM_LBUTTONDOWN passed to ipc insead of system loop)
- contextmenu of page may arise along with plugin menu (WM_RBUTTONUP passed to ipc may trigger nsWindow:WM_CONTEXTMENU)

Reproducible: Sometimes

Steps to Reproduce:
1.open flash contextmenu with customized menuitems
2.press custom item


Actual Results:  
nothing (in case of have ipc traffic -  wm_lbutton will be peeked by ipc and passed through ipc channel instead of peeking in TrackModalLoop)

Expected Results:  
item click event

in attachment i add backport of chromium changes to fix this issue

Comment 1

[Andrei Kurushin]

Attachment: os_modal_loop backport

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 540263 [details] [diff] [review]
os_modal_loop backport

Benjamin, I presume this is yours or jimm's territory.

Comment 3

[Benjamin Smedberg]

Comment on attachment 540263 [details] [diff] [review]
os_modal_loop backport

This is entirely jmathies' territory ;-)

Comment 4

[Jim Mathies [:jimm]]

 LRESULT CALLBACK
 PluginModuleChild::NestedInputEventHook(int nCode, WPARAM wParam, LPARAM lParam)
 {
     PluginModuleChild* self = current();
     PRUint32 len = self->mIncallPumpingStack.Length();
+    MessageLoop* loop = MessageLoop::current();
     if (nCode >= 0 && len && !self->mIncallPumpingStack[len - 1]._spinning) {
         self->SendProcessNativeEventsInRPCCall();
         IncallFrame& f = self->mIncallPumpingStack[len - 1];
         f._spinning = true;
-        MessageLoop* loop = MessageLoop::current();
         f._savedNestableTasksAllowed = loop->NestableTasksAllowed();
         loop->SetNestableTasksAllowed(true);
     }
+    loop->set_os_modal_loop(true);
 
     return CallNextHookEx(NULL, nCode, wParam, lParam);
 }

----------

     handled = mPluginIface->event(&mData, reinterpret_cast<void*>(&event));
 
     sWinlessPopupSurrogateHWND = NULL;
+    
+    MessageLoop* loop = MessageLoop::current();
+    loop->set_os_modal_loop(false);


This doesn't seem right. The mPluginIface->event call may create UI, say a print dialog, which triggers multiple messages delivered to NestedInputEventHook. In there, you're setting set_os_modal_loop(true) on every windowing event vs. just once on the first message received. You then set loop->set_os_modal_loop(false) when the stack unwinds, or in any case where another event is delivered higher up in the stack. Maybe we should be setting set_os_modal_loop(true) only once on the first hook message received, and use a AutoRestore<bool> to remember the current state of loop->set_os_modal_loop on calls into WinlessHandleEvent and reset it on exit.

Comment 5

[Andrei Kurushin]

Attachment: os_modal_loop backport rev2

Comment 6

[Andrei Kurushin]

diff rev2 should fix WinlessHandleEvent reentrancy problem

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 545896 [details] [diff] [review]
os_modal_loop backport rev2

Review of attachment 545896 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't tested this myself, but I think it should work.

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +1631,4 @@
>      }
>  
> +    MessageLoop* loop = MessageLoop::current();
> +    bool restoreModalLoop = !loop->os_modal_loop();

AutoRestore<bool> modalLoop(loop->os_modal_loop());

On the call into WinlessHandleEvent prior to ui being shown, this should save a false value.

@@ +1636,1 @@
>      handled = mPluginIface->event(&mData, reinterpret_cast<void*>(&event));

(here os_modal_loop() may get set to true in the hook.)

@@ +1639,5 @@
> +    
> +    if (restoreModalLoop) {
> +        loop = MessageLoop::current();
> +        loop->set_os_modal_loop(false);
> +    }

You can remove this w/AutoRestore, which will set the value back when it's destroyed. If we receive a another call into WinlessHandleEvent farther up the statck, the value will persist as true.

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +2287,1 @@
>  

You should only have to set this once in the block above - 

'if (nCode >= 0 && len && !self->mIncallPumpingStack[len - 1]._spinning)'

will be true on the first Windows message received in the hook (the purpose here is to send an event back to the parent process telling it to spin an event loop). So this should work fine if you put the MessageLoop getter back in the block where it was, and just below it set loop->set_os_modal_loop(true).

Comment 8

[Andrei Kurushin]

Attachment: os_modal_loop backport rev3

Comment 9

[Andrei Kurushin]

To use AutoRestore pattern i "opened" MessageLoop::os_modal_loop property (bool const -> bool &). Anyway here is rev3

Comment 10

[Jim Mathies [:jimm]]

(In reply to comment #9)
> To use AutoRestore pattern i "opened" MessageLoop::os_modal_loop property
> (bool const -> bool &). Anyway here is rev3

Looks good. Do you know of a good flash test case for testing? I was looking around for a site with flash &customized menus but couldn't find any.

Comment 11

[Andrei Kurushin]

it could be any multimedia flash site with heavy ipc traffic - continues repainting background or something like this

Comment 12

[Andrei Kurushin]

http://sphaeraobscura.com/context/index.html
here is good example

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 545953 [details] [diff] [review]
os_modal_loop backport rev3

Definitely better behavior. Without the test cases the browser + context menu nearly lock up.

Comment 14

[Dão Gottwald [:dao]]

ajax16384, do you want this to be committed with your real name? If so, could you add your user info to the patch? See https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f

Comment 15

[Andrei Kurushin]

Attachment: os_modal_loop backport rev3

with real name

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6b7ac58e6e96