To do this I think we need the trifecta of CompositorWidget, APZCTreeManager, and CompositorVsyncDispatcher remoted over IPDL. (APZCTM technically optional to bootstrap, but would be good to have.)

There's a bit of a problem - CompositorBridgeParent needs widget, APZCTM, and vsync objects to initialize. However at least two of these will be actors on the PCompositorBridge protocol, so they can't be constructed until after the protocol exists. This patch splits CompositorBridgeParent initialization out of the constructor so we can delay initialization in the out-of-process case. This introduces another problem, which is that CompositorBridgeParent holds a reference to itself. The only way this is released is through ActorDestroy, meaning the actor has been bound to IPDL. So this patch changes that as well: the self reference is now assigned as part of being bound to IPDL, so if we never initialize the compositor, it won't leak. Lastly, CompositorBridgeChild is now responsible for creating in-process CompositorBridgeParents. This just puts the in-process initialization logic in one place.

Comment on attachment 8766641 [details] [diff] [review] part 1, 2-step init Bill, is this MOZ_RELEASE_ASSERT okay? It looks like in-process Open() calls are infallible, but better to make sure.

Comment on attachment 8766641 [details] [diff] [review] part 1, 2-step init Review of attachment 8766641 [details] [diff] [review]: ----------------------------------------------------------------- Well, in theory this could fail. If the channel opens and then quickly dies due to an error, I think. However, we already assert that doesn't happen: http://searchfox.org/mozilla-central/rev/261fe13dcd88cfd2e99e65755e7ca4b7a2e583df/ipc/glue/MessageChannel.cpp#705 So your assertion isn't going to hurt anyone.

This just moves InProcessCompositorSession into its own file.

This moves layer tree ID allocation into GPUProcessManager. The GPM is going to maintain a map from layer tree id => compositor session, among other things, so it makes more sense to have it control the ids.

The GPU process is going to host a number of top-level actors: VsyncBridge, ImageBridge, and a CompositorBridge per-window. If the process dies, the order "ActorDestroy" is called for these actors in the UI process is undefined. While we will have to safely clean up each protocol individually, we don't want to block painting on each individual actor being cleaned up. This patch lets GPUProcessManager receive a notification when an actor it creates has unexpectedly died. It will then be responsible for revoking all existing actors from Gecko, pushing them out of the way so we can create a new GPU process as quickly as possible. The old actors will still hang around waiting to be cleaned up, but GPUProcessManager won't be waiting on them, and widgets will no longer have a CompositorSession.

Comment on attachment 8769009 [details] [diff] [review] part 4, synchronize ActorDestroys Review of attachment 8769009 [details] [diff] [review]: ----------------------------------------------------------------- Note: the token's job is just to be a nonce. We can use the layers id instead if you think that's better.

s/the layers id/an id/

Bill, could you review the IPDL-related changes in this patch? The initialization process for a RemoteCompositorSession is: 1. Create endpoints 2. Bind child endpoint 3. Send parent endpoint to GPU process 4. Bind parent endpoint in compositor thread of GPU process 5. Send PCompositorWidget constructor 6. Send PCompositorBridge initialization We attempt steps 5-6 without waiting on step 4. In addition CompositorBridgeChild uses the ActorDestroy notification from part 4, which is otherwise unused so far except to kill the process. Finally ProcessingError accounts for MsgDropped spam.

With these patches we get as far as instantiating a CompositorBridgeParent in the remote process, but of course immediately die trying to access gfxPlatform. That's bug 1282364.

Comment on attachment 8769009 [details] [diff] [review] part 4, synchronize ActorDestroys Review of attachment 8769009 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I think a monotonically increasing uint64_t instead of GPUProcessToken would be simpler. ::: gfx/ipc/GPUProcessToken.h @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- c-basic-offset should be 2. @@ +3,5 @@ > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef _include_mozilla_gfx_ipc_GPUProcessToken_h_ Please follow the coding style here: mozilla_gfx_ipc_GPUProcessToken_h.

Using monotonic counter instead of nonce pointer.

Rebased for part 4 changes.

When the GPU process terminates early, it asks GPUProcessManager to handle the error. The GPUProcessManager then asks the process to shutdown. This was causing MessageChannel::Close() to be called twice, once in ActorDestroy and another time in GPUProcessHost. This patch fixes that and adds a comment to help clarify the order of events.

This is the same as the previous patch, but I moved RemoteCompositorSession construction to GPUProcessManager. With the vsync patches the number of arguments we have to pass around were getting ridiculous. GPUProcessManager is a better place since it knows where everything is.

This replaces the existing PCompositorBridge::Opens code with endpoints instead. This will allow us to bridge the GPU process to the content process.

This implements the bridge from the GPU process to the content process. With this and the vsync stuff, both the UI and content process successfully render to the GPU process. * ** *** * With the vsync patches, and some hacks to disable gfxPlatform. ** Software compositing only, so far. *** Does not work with content sandboxing for some reason. Will file separate bug for that.

Comment on attachment 8769949 [details] [diff] [review] part 4, synchronize ActorDestroys Review of attachment 8769949 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/ipc/GPUProcessHost.h @@ +75,5 @@ > } > > + // Return a unique id for this process, guaranteed not to be shared with any > + // past or future instance of GPUProcessHost. > + const uint64_t& GetProcessToken() const; Why not uint64_t instead of const uint64_t& ? ::: gfx/ipc/GPUProcessManager.cpp @@ +1,1 @@ > /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ Please change c-basic-offset to 2. @@ +8,2 @@ > #include "mozilla/layers/InProcessCompositorSession.h" > +#include "mozilla/StaticPtr.h" No need for StaticPtr anymore I assume. @@ +162,5 @@ > +void > +GPUProcessManager::NotifyRemoteActorDestroyed(const uint64_t& aProcessToken) > +{ > + if (!NS_IsMainThread()) { > + NS_DispatchToMainThread(mTaskFactory.NewTask<NotifyRemoteActorDestroyedTask>( If you use mTaskFactory.NewRunnableMethod here, I think you can avoid the new to write a custom runnable.

Comment on attachment 8770027 [details] [diff] [review] part 5, RemoteCompositorSession Review of attachment 8770027 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/ipc/GPUParent.h @@ +22,5 @@ > IPC::Channel* aChannel); > > bool RecvInit(nsTArray<GfxPrefSetting>&& prefs) override; > bool RecvUpdatePref(const GfxPrefSetting& pref) override; > + bool RecvNewWindowCompositor( Given how overloaded the term "window" is, I feel like NewChromeCompositor or something would be a better name. ::: gfx/layers/ipc/CompositorBridgeParent.cpp @@ +613,5 @@ > #endif > { > + // Always run destructor on the main thread > + MOZ_ASSERT(NS_IsMainThread()); > + SetMessageLoopToPostDestructionTo(MessageLoop::current()); Do we still need this? It used to be that actors had to be destroyed on the main thread, but this is no longer the case (bug 1121676). Might be possible to simplify some code here. Reading AtomicRefCountedWithFinalize.h makes me want to barf.

(In reply to Bill McCloskey (:billm) from comment #21) > Comment on attachment 8770027 [details] [diff] [review] > part 5, RemoteCompositorSession > > Review of attachment 8770027 [details] [diff] [review]: > ----------------------------------------------------------------- > > Given how overloaded the term "window" is, I feel like NewChromeCompositor > or something would be a better name. I'll go with "Widget" unless you think that's overloaded too. > ::: gfx/layers/ipc/CompositorBridgeParent.cpp > @@ +613,5 @@ > > #endif > > { > > + // Always run destructor on the main thread > > + MOZ_ASSERT(NS_IsMainThread()); > > + SetMessageLoopToPostDestructionTo(MessageLoop::current()); > > Do we still need this? It used to be that actors had to be destroyed on the > main thread, but this is no longer the case (bug 1121676). Might be possible > to simplify some code here. Reading AtomicRefCountedWithFinalize.h makes me > want to barf. Interesting - probably not. I'll try this in a separate bug.

Filed bug 1287232.

Pushed by danderson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/842631f306bc Split up CompositorBridgeParent initialization. (bug 1282348 part 1, r=mattwoodrow,billm) https://hg.mozilla.org/integration/mozilla-inbound/rev/31ee282cfbbd Split InProcessCompositorBridge to its own file. (bug 1282348 part 2, r=mattwoodrow) https://hg.mozilla.org/integration/mozilla-inbound/rev/f26200be9e80 Move layers ID allocation to GPUProcessManager. (bug 1282348 part 3, r=mattwoodrow) https://hg.mozilla.org/integration/mozilla-inbound/rev/127687211494 Allow top-level protocols the ability to notify GPUProcessManager when their actors are unexpectedly destroyed. (bug 1282348 part 4, r=billm) https://hg.mozilla.org/integration/mozilla-inbound/rev/e6bd9062617f Add a remote implementation of CompositorSession. (bug 1282348 part 5, r=billm) https://hg.mozilla.org/integration/mozilla-inbound/rev/0520526e9ff3 Don't call Close twice when the GPU process unexpectedly terminates. (bug 1282348 part 6, r=billm) https://hg.mozilla.org/integration/mozilla-inbound/rev/39636df7a0fe Send content compositor bridges using endpoints rather than Opens. (bug 1282348 part 7, r=billm) https://hg.mozilla.org/integration/mozilla-inbound/rev/b09494ecac5f Support compositor bridges from the content process to the GPU process. (bug 1282348 part 8, r=billm)

https://hg.mozilla.org/mozilla-central/rev/842631f306bc https://hg.mozilla.org/mozilla-central/rev/31ee282cfbbd https://hg.mozilla.org/mozilla-central/rev/f26200be9e80 https://hg.mozilla.org/mozilla-central/rev/127687211494 https://hg.mozilla.org/mozilla-central/rev/e6bd9062617f https://hg.mozilla.org/mozilla-central/rev/0520526e9ff3 https://hg.mozilla.org/mozilla-central/rev/39636df7a0fe https://hg.mozilla.org/mozilla-central/rev/b09494ecac5f