1274149 - Restrict main-thread access to CompositorBridgeParent

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

With out-of-process compositing, callers on the main thread won't be able to access CompositorBridgeParent directly, since it might live in the GPU process. I'm proposing a new GPUProcessManager class to provide these functions instead. Initially, it'll just wrap CompositorBridgeParent, but we'll try to ensure that no new direct callers appear.

GPUProcessManager will later be responsible for launching the GPU process and maintaining state that has to migrate from out-of-process to in-process, or vice versa, in the event of a device reset or crash.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, GPUProcessManager

Skeleton class, with CompositorSession instantiation moved over.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, mv GetAPZCTreeManager

Make GetAPZCTreeManager private, move calls to GPUProcessManager.

I changed the CBP version to return already_AddRefed since it wasn't clear how threadsafe it is to return the bare pointer.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8754215 [details] [diff] [review]
part 1, GPUProcessManager

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

::: gfx/ipc/GPUProcessManager.h
@@ +20,5 @@
> +namespace gfx {
> +
> +// The GPUProcessManager is a singleton responsible for creating GPU-bound
> +// objects that may live in another process. Currently, it provides access
> +// to the compositor via CompositorBridgeParent.

CompositorSession?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, move Alloc/DeallocLayerTreeId calls

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, move TabParent calls

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, move ContentParent calls

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to David Anderson [:dvander] from comment #0)
> With out-of-process compositing, callers on the main thread won't be able to
> access CompositorBridgeParent directly, since it might live in the GPU
> process. I'm proposing a new GPUProcessManager class to provide these
> functions instead. Initially, it'll just wrap CompositorBridgeParent, but
> we'll try to ensure that no new direct callers appear.
> 
> GPUProcessManager will later be responsible for launching the GPU process
> and maintaining state that has to migrate from out-of-process to in-process,
> or vice versa, in the event of a device reset or crash.

Follow-up note: this state includes layers ID allocation (which will have to move to the GPUProcessManager) as well as observers, content controllers, and possibly the APZ input queue. When compositing has to migrate from out-of-process to in-process, GPUProcessManager will have to ensure the new compositor begins with this state.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8754215 [details] [diff] [review]
part 1, GPUProcessManager

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

::: gfx/ipc/GPUProcessManager.h
@@ +29,5 @@
> +  static void Shutdown();
> +  static GPUProcessManager* Get();
> +
> +  GPUProcessManager();
> +  ~GPUProcessManager();

The constructor should probably be protected or private, if this is a singleton

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a22a53c503a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/9932689ab717
https://hg.mozilla.org/integration/mozilla-inbound/rev/057223823e5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/540cb6d10745
https://hg.mozilla.org/integration/mozilla-inbound/rev/1117598ac875

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out because it seems like the most likely cause of frequent intermittent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=28431370&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b45aeb5288a

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looking further back, this failure seems to have started prior to this landing. I think you should be good to reland this once the tree reopens.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e667524c874d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ff484f408a
https://hg.mozilla.org/integration/mozilla-inbound/rev/082be8ab538c
https://hg.mozilla.org/integration/mozilla-inbound/rev/bca90b6efa44
https://hg.mozilla.org/integration/mozilla-inbound/rev/49fbf8516de9

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e667524c874d
https://hg.mozilla.org/mozilla-central/rev/b3ff484f408a
https://hg.mozilla.org/mozilla-central/rev/082be8ab538c
https://hg.mozilla.org/mozilla-central/rev/bca90b6efa44
https://hg.mozilla.org/mozilla-central/rev/49fbf8516de9