Closed Bug 665315 Opened 9 years ago Closed 9 years ago

OOPP Contextmenu loses messages


(Core :: Plug-ins, defect)

Windows 7
Not set





(Reporter: ajax16384, Assigned: ajax16384)




(1 file, 3 obsolete files)

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
Attached patch os_modal_loop backport (obsolete) — Splinter Review
Version: unspecified → Trunk
Component: IPC → Plug-ins
QA Contact: ipc → plugins
Comment on attachment 540263 [details] [diff] [review]
os_modal_loop backport

Benjamin, I presume this is yours or jimm's territory.
Attachment #540263 - Flags: review?(benjamin)
Comment on attachment 540263 [details] [diff] [review]
os_modal_loop backport

This is entirely jmathies' territory ;-)
Attachment #540263 - Flags: review?(benjamin) → review?(jmathies)
Blocks: 538902
 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.
Attached patch os_modal_loop backport rev2 (obsolete) — Splinter Review
Attachment #540263 - Attachment is obsolete: true
Attachment #540263 - Flags: review?(jmathies)
diff rev2 should fix WinlessHandleEvent reentrancy problem
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).
Attached patch os_modal_loop backport rev3 (obsolete) — Splinter Review
Attachment #545896 - Attachment is obsolete: true
To use AutoRestore pattern i "opened" MessageLoop::os_modal_loop property (bool const -> bool &). Anyway here is rev3
(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.
it could be any multimedia flash site with heavy ipc traffic - continues repainting background or something like this
here is good example
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.
Attachment #545953 - Flags: review+
Keywords: checkin-needed
Assignee: nobody → ajax16384
Ever confirmed: true
ajax16384, do you want this to be committed with your real name? If so, could you add your user info to the patch? See
with real name
Attachment #545953 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [inbound]
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Depends on: 725306
Depends on: 762456
You need to log in before you can comment on or make changes to this bug.