1066242 - D3D9 compositor thread creates UI on Windows but doesn't pump windowing messages

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jim Mathies [:jimm]]

The basic rule of win32 programming is, if you create native UI, you have to pump windowing messages so the thread can handle things like SendMessage calls. If you don't you risk random deadlocks.

DeviceManagerD3D9::Init() creates windows, and runs on a background chromium thread that does not use the right message pump - 

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/DeviceManagerD3D9.cpp#179

This is causing random deadlocks when we enumerate our windows for screen sharing.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch v.1

It'd probably be better if we just do this for d3d9, since afaict that's the only compositor that needs this. I'm not seeing a way to detect that here where we create the thread in question.

Comment 2

[Jim Mathies [:jimm]]

Note this bug will pretty much deadlock any application that tries to query this window for information. Screen readers are good candidates, also debugger tools, and of course our own webrtc code.

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 8488155 [details] [diff] [review]
patch v.1

Review of attachment 8488155 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: gfx/layers/ipc/CompositorParent.cpp
@@ +149,5 @@
>     * Compositor hangs seen in the wild, but is short enough to not miss getting
>     * native hang stacks. */
>    options.permanent_hang_timeout = 2048; // milliseconds
> +#if defined(_WIN32)
> +  options.message_loop_type = MessageLoop::TYPE_UI;

nit: Tiny comment would be helpful probably :-).

Comment 4

[Jim Mathies [:jimm]]

(In reply to Bas Schouten (:bas.schouten) from comment #3)
> 
> nit: Tiny comment would be helpful probably :-).

will do.

try push - 

https://tbpl.mozilla.org/?tree=Try&rev=f4857492ab30

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8488155 [details] [diff] [review]
patch v.1

Patch works for me (and I'm the one who ran into the problem; likely due to my old 7600GT graphics card)

Comment 6

[Jim Mathies [:jimm]]

We put together a work around for this for screensharing over in bug 1060738, but that fix isn't working right. It turns out that Windows attaches an IME window to this d3d window, and when we enumerate that, it also locks up the browser. jesup tested the proper fix (posted here) and found no issues. So we're curious if the gfx team feels comfortable uplifting this fix to beta along with the work in bug  	1060738.

From jesup, our options are:
a) land real fix, uplift to beta.  I like this.
b) ask if the hwnd is d3d9 or a child of it (I assume that can be done).
c) Disable screensharing on dx9 windows boxes (I really dislike this - but it only affects older hardware.  Downside is we have no UI for showing why they can't use it)

Comment 7

[Jim Mathies [:jimm]]

Attachment: patch to land

Comment 8

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc38537998a

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ccc38537998a

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jim Mathies [:jimm] from comment #6)
> ...So we're curious if the gfx team feels comfortable
> uplifting this fix to beta along with the work in bug 1060738.

I'm comfortable if Bas is.

> 
> From jesup, our options are:
> a) land real fix, uplift to beta.  I like this.
> b) ask if the hwnd is d3d9 or a child of it (I assume that can be done).
> c) Disable screensharing on dx9 windows boxes (I really dislike this - but
> it only affects older hardware.  Downside is we have no UI for showing why
> they can't use it)

Comment 11

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #10)
> (In reply to Jim Mathies [:jimm] from comment #6)
> > ...So we're curious if the gfx team feels comfortable
> > uplifting this fix to beta along with the work in bug 1060738.
> 
> I'm comfortable if Bas is.
> 
> > 
> > From jesup, our options are:
> > a) land real fix, uplift to beta.  I like this.
> > b) ask if the hwnd is d3d9 or a child of it (I assume that can be done).
> > c) Disable screensharing on dx9 windows boxes (I really dislike this - but
> > it only affects older hardware.  Downside is we have no UI for showing why
> > they can't use it)

I am.

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 8488297 [details] [diff] [review]
patch to land

Approval Request Comment
[Feature/regressing bug #]:
Issue with the original implementation of OMTC on windows.
[User impact if declined]:
Random lock ups of 3rd party applications that try to interact with our browser, and internal lockups in webrtc screen sharing code.
[Describe test coverage new/current, TBPL]:
Landed on mc this morning. I'll land on aurora/beta beginning of next week.
[Risks and why]:
It's a fairly big under the hood change for the main compositor thread on Windows. The new code path is well tested however.
[String/UUID change made/needed]:
none.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

I guess we won't uplift this that to ESR.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c693ee0aa738
https://hg.mozilla.org/releases/mozilla-beta/rev/e3fe616ef9a2

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a128f3f1ce1f
The correct patch