Last Comment Bug 1072752 - IPC message pump for windows should use WinUtils::WaitForMessage
: IPC message pump for windows should use WinUtils::WaitForMessage
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: x86 Windows 7
-- normal (vote)
: mozilla35
Assigned To: Aaron Klotz [:aklotz]
:
: Bill McCloskey (:billm)
Mentors:
Depends on:
Blocks: 937306
  Show dependency treegraph
 
Reported: 2014-09-24 22:31 PDT by Aaron Klotz [:aklotz]
Modified: 2014-09-29 11:50 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
Make chromium message pump call into WinUtils::WaitForMessage (5.15 KB, patch)
2014-09-25 11:28 PDT, Aaron Klotz [:aklotz]
jmathies: review+
sledru: approval‑mozilla‑aurora+
sledru: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Aaron Klotz [:aklotz] 2014-09-24 22:31:56 PDT
I think that the ipc/chromium message pump for Windows should use the same algorithm as the main thread. I suggest that we delegate the implementation of MessagePumpForUI::WaitForWork to WinUtils::WaitForMessage.
Comment 1 User image Jim Mathies [:jimm] 2014-09-25 06:24:11 PDT
The problem ted ran into is related to a lost posted message to the main dispatch loop (the native event callback) afaiui. I don't understand how changing the chromium TYPE_UI dispatch loop plays into this?
Comment 2 User image Aaron Klotz [:aklotz] 2014-09-25 10:28:23 PDT
(In reply to Jim Mathies [:jimm] from comment #1)
> The problem ted ran into is related to a lost posted message to the main
> dispatch loop (the native event callback) afaiui. I don't understand how
> changing the chromium TYPE_UI dispatch loop plays into this?

Let's use out of process plugins as an example. The plugin container creates a window whose parent is set to a window in the Firefox process. The plugin container's main thread input queue is now implicitly attached to the main thread input queue in Firefox. Since they are synchronized, both threads' message pumps need to pump messages correctly.

If plugin container's message pump falls into the WaitMessage path then it may be leaving unprocessed messages that are no longer considered "new" in its queue. As long as the plugin window doesn't see any new input, it will not pump messages. This will also cause trouble with the main thread because it will not be able to successfully perform an unfiltered PeekMessage as long as the plugin thread is waiting.

I think this has similarities to what Raymond Chen describes in
http://blogs.msdn.com/b/oldnewthing/archive/2013/06/06/10424006.aspx

Does that make sense?
Comment 3 User image Jim Mathies [:jimm] 2014-09-25 10:56:49 PDT
(In reply to Aaron Klotz [:aklotz] from comment #2)
> (In reply to Jim Mathies [:jimm] from comment #1)
> > The problem ted ran into is related to a lost posted message to the main
> > dispatch loop (the native event callback) afaiui. I don't understand how
> > changing the chromium TYPE_UI dispatch loop plays into this?
> 
> Let's use out of process plugins as an example. The plugin container creates
> a window whose parent is set to a window in the Firefox process. The plugin
> container's main thread input queue is now implicitly attached to the main
> thread input queue in Firefox. Since they are synchronized, both threads'
> message pumps need to pump messages correctly.
> 
> If plugin container's message pump falls into the WaitMessage path then it
> may be leaving unprocessed messages that are no longer considered "new" in
> its queue. As long as the plugin window doesn't see any new input, it will
> not pump messages. This will also cause trouble with the main thread because
> it will not be able to successfully perform an unfiltered PeekMessage as
> long as the plugin thread is waiting.
> 
> I think this has similarities to what Raymond Chen describes in
> http://blogs.msdn.com/b/oldnewthing/archive/2013/06/06/10424006.aspx
> 
> Does that make sense?

Yes, to some extent. I wish we had a stack of the child stuck in WaitMessage for proof, but I'm ok with experimenting around with fixes. The chen article was very interesting. Note we also have 

Note we have MessagePumpForNonMainUIThreads as well now

http://mxr.mozilla.org/mozilla-central/source/ipc/glue/MessagePump.cpp#382

which also calls WaitForWork.
Comment 4 User image Aaron Klotz [:aklotz] 2014-09-25 11:28:32 PDT
Created attachment 8495422 [details] [diff] [review]
Make chromium message pump call into WinUtils::WaitForMessage

This patch adds an optional delay parameter to WaitForMessage and modifies the chromium stuff to call into it.
Comment 6 User image Ryan VanderMeulen [:RyanVM] 2014-09-25 13:56:12 PDT
https://hg.mozilla.org/mozilla-central/rev/171afe599b29
Comment 7 User image Aaron Klotz [:aklotz] 2014-09-29 09:41:21 PDT
Comment on attachment 8495422 [details] [diff] [review]
Make chromium message pump call into WinUtils::WaitForMessage

[Feature/regressing bug #]: Windows OMTC
[User impact if declined]: Browser unresponsive for some users unless there is mouse activity.
[Describe test coverage new/current, TBPL]: Landed on m-c
[Risks and why]: Low; This patch modifies non-main UI threads to use the same WaitForMessage code as the main thread already does.
[String/UUID change made/needed]: None

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