The code that landed in bug 1191971 created a parenting relationship between the browser window owned by the browser process main thread and a child window in the GPU process owned by a background compositor thread [1]. Due to this window parenting relationship the two threads may have their message input queues attached, which means Windows will synchronize processing of events in both queues. The side effect of this is that a pause in event processing on one thread will pause event processing on the other. A good example: a hang on the compositor thread (drivers doing time consuming tasks, dll loads, long winded composition processing) will also hang the firefox front end UI.

Another side effect of this involves security. One of the switches we have for a GPU process sandbox involves lowering the integrity level of the GPU process. Low integrity windows can't parent to medium integrity windows, so in order for this parenting relationship to work, we can't turn integrity isolation on for the GPU process. This switch locks down access to various areas [2] of the operating system to code running in the GPU process. If the GPU process were compromised, attackers would have access to these areas.

Possible workarounds -

1. Detach the thread input queues after the parenting relationship is set up using AttachThreadInput [3].

This might solve the hang risk, but it doesn't solve the security risk. In addition, this configuration is largely unsupported by Windows. We've experimented around with this with plugin child windows in the past. Side effects included broken input event processing, window focus issues, and in some cases desktop hangs. Generally probably not a good approach due to unknown side effects.

2. Parent the child window in the GPU process to a parent window on another thread in the main process.

This solves hang risk we would see in the main Firefox UI, but it prevents us from implementing integrity isolation for the GPU process. We could live with the security implications here, until hackers take advantage of it at some future point in time.

3. Don't parent a child window in the GPU process with a window in the browser process.

Preferred fix but it might prevent the graphics team from shipping direct composition.

[1] https://searchfox.org/mozilla-central/source/gfx/webrender_bindings/RenderThread.cpp
[2] https://en.wikipedia.org/wiki/Mandatory_Integrity_Control
[3] https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-attachthreadinput

[Tracking Requested - why for this release]:
potential performance related issue

From looking over the docs on this, it isn't clear why we used the browser window as a parent. Looks like chrome creates the parent in the gpu as a hidden popup. So maybe there's a fourth workaround - create the parent and child in the gpu.

(In reply to Jim Mathies [:jimm] from comment #2)

From looking over the docs on this, it isn't clear why we used the browser window as a parent. Looks like chrome creates the parent in the gpu as a hidden popup. So maybe there's a fourth workaround - create the parent and child in the gpu.

Thanks for the detailed information. Current implementation mimics how chromium implements except the followings
-[1] Do not use HiddenPopupWindow as initial parent window.
-[2] Do not call ::SetParent() in UI process for setting parent window from HiddenPopupWindow to MaingWindow

In chromium, HiddenPopupWindow was created just for initial parent window. The parent window is soon replaced by HWND of Main window in UI Process by calling ::SetParent() in UI process like the following sequence.

ChildWindowWin::Initialize()
->GLES2CommandBufferStub::DidCreateAcceleratedSurfaceChildWindow()
->GpuServiceImpl::SendCreatedChildWindow()
->GpuHostProxy::SetChildSurface()
->// IPC
->// In UI process
->GpuHostImpl::SetChildSurface()
->RenderingWindowManager::RegisterChild()
->::SetParent(child, parent);

I removed [1] and [2] since, it succeeded to create child window of MainWindow in GPU process. Reviving [1] and [2] seemed to address the problem of Bug 1347710 Comment 20.

Fix build failure.

attachment 9041124 [details] [diff] [review] addressed try problem with security.sandbox.gpu.level=1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61d3ec33c2ef9629a0eab11f1535e8af0c72ad4b

Just default m-c with security.sandbox.gpu.level=1 caused the problem
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b2d9207aee3485ecf45954a70ac46615c77d295

(In reply to Sotaro Ikeda [:sotaro] from comment #3)

(In reply to Jim Mathies [:jimm] from comment #2)

From looking over the docs on this, it isn't clear why we used the browser window as a parent. Looks like chrome creates the parent in the gpu as a hidden popup. So maybe there's a fourth workaround - create the parent and child in the gpu.

Thanks for the detailed information. Current implementation mimics how
chromium implements except the followings
-[1] Do not use HiddenPopupWindow as initial parent window.
-[2] Do not call ::SetParent() in UI process for setting parent window from
HiddenPopupWindow to MaingWindow

In chromium, HiddenPopupWindow was created just for initial parent window.
The parent window is soon replaced by HWND of Main window in UI Process by
calling ::SetParent() in UI process like the following sequence.

Really? Is the thread in the GPU process their compositor or renderer?

AFAICT you're still going to have attached thread input queues. What are the implications of this? Does the GPU thread involved here ever get bogged down or hang long periods?

Also I'd be curious, since this has been out on mc for a while, I wonder if we've seen any regressions in main thread event processing performance around the time this landed.

Also, what's the situation if the gpu process isn't running? Is this code in use there as well?

Bob and I were discussing this today, looks like chromium creates a separate even queue for this child window.

https://cs.chromium.org/chromium/src/gpu/ipc/service/child_window_win.cc?type=cs&q=ChildWindowWin&g=0&l=133

(In reply to Jim Mathies [:jimm] from comment #8)

AFAICT you're still going to have attached thread input queues. What are the implications of this? Does the GPU thread involved here ever get bogged down or hang long periods?

Assuming the "GPU thread involved here" is the GPU process main thread, there's basically nothing that runs there. We do some APZ input event processing on it but otherwise it's mostly idle. The APZ input event processing is already synchronous with respect to the UI process main thread so attaching the input queues shouldn't make any difference there.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)

(In reply to Jim Mathies [:jimm] from comment #8)

AFAICT you're still going to have attached thread input queues. What are the implications of this? Does the GPU thread involved here ever get bogged down or hang long periods?

Assuming the "GPU thread involved here" is the GPU process main thread,
there's basically nothing that runs there. We do some APZ input event
processing on it but otherwise it's mostly idle. The APZ input event
processing is already synchronous with respect to the UI process main thread
so attaching the input queues shouldn't make any difference there.

Ok great. (We were concerned rendering was taking place on this thread.) A comment in the class that manages it noting the thread is bound to the browser main thread might be good, something that lets devs know they shouldn't use it for heavy lifting.

Thanks for the updates to allow for a GPU sandbox!

(In reply to Jim Mathies [:jimm] from comment #10)

Bob and I were discussing this today, looks like chromium creates a separate even queue for this child window.

https://cs.chromium.org/chromium/src/gpu/ipc/service/child_window_win.cc?type=cs&q=ChildWindowWin&g=0&l=133

WinCompositorWindowThread creates a MessageLoop::TYPE_UI thread for compositor window.
https://searchfox.org/mozilla-central/source/widget/windows/WinCompositorWindowThread.cpp#37

(In reply to Sotaro Ikeda [:sotaro] from comment #13)

(In reply to Jim Mathies [:jimm] from comment #10)

Bob and I were discussing this today, looks like chromium creates a separate even queue for this child window.

https://cs.chromium.org/chromium/src/gpu/ipc/service/child_window_win.cc?type=cs&q=ChildWindowWin&g=0&l=133

When we are talking about "event queue," we are not meaning at the application level; we are talking about the level of the window manager in the OS.

HWNDs have thread affinity: internally to the window manager, they share a single message queue that is owned by the thread on which the window was created. It is this per-thread message queue that becomes synchronized with the per-thread message queue on the thread whose HWND has been parented. You simply cannot assign an HWND a distinct queue; this is built into the window manager.

(And no, calling GetMessage or PeekMessage with a specific HWND does not fix this problem, and in fact may exacerbate it)

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See How Do You Triage for more information

Given comments 11 and 12, this no longer need tracking for 67, I am setting the priority back to p4.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca6aa20ab9e91c4196370cad06a4d0ab5ed0d11d

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