1072752 - IPC message pump for windows should use WinUtils::WaitForMessage

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

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

[Jim Mathies [:jimm]]

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

[(No longer employed by Mozilla) Aaron Klotz]

(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

[Jim Mathies [:jimm]]

(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

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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 5

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/171afe599b29

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/171afe599b29

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1296abcca24d
https://hg.mozilla.org/releases/mozilla-beta/rev/737fbc0e3df4