Note: There are a few cases of duplicates in user autocompletion which are being worked on.

IPC message pump for windows should use WinUtils::WaitForMessage

RESOLVED FIXED in Firefox 33

Status

()

Core
IPC
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

Trunk
mozilla35
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, firefox35 fixed)

Details

Attachments

(1 attachment)

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

3 years ago
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?
(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?
Flags: needinfo?(jmathies)

Comment 3

3 years ago
(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.
Flags: needinfo?(jmathies)
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.
Attachment #8495422 - Flags: review?(jmathies)

Updated

3 years ago
Attachment #8495422 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/171afe599b29
https://hg.mozilla.org/mozilla-central/rev/171afe599b29
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Attachment #8495422 - Flags: approval-mozilla-beta?
Attachment #8495422 - Flags: approval-mozilla-aurora?
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → fixed
Attachment #8495422 - Flags: approval-mozilla-beta?
Attachment #8495422 - Flags: approval-mozilla-beta+
Attachment #8495422 - Flags: approval-mozilla-aurora?
Attachment #8495422 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/1296abcca24d
https://hg.mozilla.org/releases/mozilla-beta/rev/737fbc0e3df4
status-firefox33: affected → fixed
status-firefox34: affected → fixed
You need to log in before you can comment on or make changes to this bug.