Last Comment Bug 665315 - OOPP Contextmenu loses messages
: OOPP Contextmenu loses messages
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla8
Assigned To: Andrei Kurushin
: Benjamin Smedberg [:bsmedberg]
Depends on: 762456 725306
Blocks: 538902
  Show dependency treegraph
Reported: 2011-06-18 13:21 PDT by Andrei Kurushin
Modified: 2012-06-07 19:31 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

os_modal_loop backport (8.27 KB, patch)
2011-06-18 13:23 PDT, Andrei Kurushin
no flags Details | Diff | Splinter Review
os_modal_loop backport rev2 (8.55 KB, patch)
2011-07-14 05:49 PDT, Andrei Kurushin
no flags Details | Diff | Splinter Review
os_modal_loop backport rev3 (8.62 KB, patch)
2011-07-14 10:59 PDT, Andrei Kurushin
jmathies: review+
Details | Diff | Splinter Review
os_modal_loop backport rev3 (8.79 KB, patch)
2011-07-27 04:54 PDT, Andrei Kurushin
no flags Details | Diff | Splinter Review

Description Andrei Kurushin 2011-06-18 13:21:32 PDT
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. 
- 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: flash contextmenu with customized menuitems 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 2011-06-18 13:23:12 PDT
Created attachment 540263 [details] [diff] [review]
os_modal_loop backport
Comment 2 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-06-18 13:36:59 PDT
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 [:bsmedberg] 2011-06-23 14:28:55 PDT
Comment on attachment 540263 [details] [diff] [review]
os_modal_loop backport

This is entirely jmathies' territory ;-)
Comment 4 Jim Mathies [:jimm] 2011-07-13 08:35:20 PDT
 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) {
         IncallFrame& f = self->mIncallPumpingStack[len - 1];
         f._spinning = true;
-        MessageLoop* loop = MessageLoop::current();
         f._savedNestableTasksAllowed = loop->NestableTasksAllowed();
+    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 2011-07-14 05:49:37 PDT
Created attachment 545896 [details] [diff] [review]
os_modal_loop backport rev2
Comment 6 Andrei Kurushin 2011-07-14 05:52:21 PDT
diff rev2 should fix WinlessHandleEvent reentrancy problem
Comment 7 Jim Mathies [:jimm] 2011-07-14 08:23:28 PDT
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 2011-07-14 10:59:34 PDT
Created attachment 545953 [details] [diff] [review]
os_modal_loop backport rev3
Comment 9 Andrei Kurushin 2011-07-14 11:01:33 PDT
To use AutoRestore pattern i "opened" MessageLoop::os_modal_loop property (bool const -> bool &). Anyway here is rev3
Comment 10 Jim Mathies [:jimm] 2011-07-14 12:50:01 PDT
(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 2011-07-19 05:45:32 PDT
it could be any multimedia flash site with heavy ipc traffic - continues repainting background or something like this
Comment 12 Andrei Kurushin 2011-07-25 05:06:47 PDT
here is good example
Comment 13 Jim Mathies [:jimm] 2011-07-26 08:20:12 PDT
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] 2011-07-27 04:06:12 PDT
ajax16384, do you want this to be committed with your real name? If so, could you add your user info to the patch? See
Comment 15 Andrei Kurushin 2011-07-27 04:54:06 PDT
Created attachment 548750 [details] [diff] [review]
os_modal_loop backport rev3

with real name

Note You need to log in before you can comment on or make changes to this bug.