Closed Bug 1066242 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Layers, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- wontfix
firefox33 --- fixed
firefox34 --- fixed
firefox35 --- fixed
firefox-esr31 --- wontfix

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(2 files)

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.
Attached patch patch v.1Splinter Review
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.
Assignee: nobody → jmathies
Attachment #8488155 - Flags: review?(bas)
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.
Depends on: 1060738
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 :-).
Attachment #8488155 - Flags: review?(bas) → review+
(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 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)
Attachment #8488155 - Flags: feedback+
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)
Flags: needinfo?(milan)
https://hg.mozilla.org/mozilla-central/rev/ccc38537998a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(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)
Flags: needinfo?(milan) → needinfo?(bas)
(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.
Flags: needinfo?(bas)
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.
Attachment #8488297 - Flags: approval-mozilla-beta?
Attachment #8488297 - Flags: approval-mozilla-aurora?
Blocks: 1060738
No longer depends on: 1060738
Attachment #8488297 - Flags: approval-mozilla-beta?
Attachment #8488297 - Flags: approval-mozilla-beta+
Attachment #8488297 - Flags: approval-mozilla-aurora?
Attachment #8488297 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.