With WebRender more things need to communicate data between the UI process and the GPU process, and the content processes and the GPU process, but for all PCompositorBridge objects instead of just one. Shared images and fonts among them. While each of these things could be made their own top level protocols, making it the manager of PCompositorBridge has advantages without many drawbacks. There will be a single toplevel manager between the GPU process and its peer. This will be a convenient place to store/manager data that only needs to be shared once with the process. We will have fewer synchronization concerns. When we share the same child thread (main thread), parent thread (compositor thread) and IPC channel, we have stronger ordering guarantees relative to each protocol instance in the same manager tree. This allows us to avoid sync messages by design. (E.g. While constructing a display list for a PWebRenderBridge, we can share images on demand with the new toplevel protocol manager, and know that the async share will be processed before the display list end, rather than require synchronization later). We should not be introducing new IO-bound issues because the actual reading/writing to these IPC channels is restricted to the same IO thread as before. We will also have fewer IPC channels to setup (for each widget compositor from the UI process, and also for new protocols from alternative designs), which will save us some resources. The core semantics of PCompositorBridge (i.e. CompositorBridgeParent vs CrossProcessCompositorBridgeParent) should not be impacted. The only thing that will change is that on startup we create the toplevel protocol manager, which in turn will create the PCompositorBridge, rather than creating the PCompositorBridge directly. And similar on teardown.

I like this idea for another reason: shutting down PCompositorBridges is hard. By having a top-level protocol we can ensure they're all shut down together and not worry about ticking down CompositorThreadHolders as ActorDestroys come in (which depends on widgets shutting down properly). Note that we'll need an instance of the top-level protocol from the main thread to the in-process compositor thread as well.

WIP that seems to work for UI+GPU combined and UI/GPU separated scenarios, but there is a crash on shutdown if there were multiple content processes. From what I can tell at this point, it seems a WebRenderTextureHost's actor was destroyed with its owning CompositorBridgeParent, however the WebRenderTextureHost itself did not get freed (and the underlying ShmemTextureHost) until another CompositorBridgeParent was freed. This resulted in trying to free the ShmemTextureHost against an already freed CompositorBridgeParent and then crashing. Investigation on that continues.

Fixed the shutdown bug. CompositorBridgeParent/Child used to be the toplevel protocol, which means if you had a strong reference to it, you know you could free shared memory (even if the IPC channel was already shutdown). When it is a managed protocol, the manager also needs to be retained in order to free shared memory (because the managee delegates to the toplevel protocol), but the CompositorManagerParent/Child was getting freed too early. Now the CompositorBridgeParent/Child have a strong reference to their manager -- this technically introduces a reference cycle, but it is broken by the protocol shutdown routines.

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

Fix build errors, some new comments/asserts. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=509e9403cdd62835d0455637dae39dd114a5cfa6

Fix yet more build errors, and restructure things on the same process path to allow the PCompositorBridge constructor to be async instead of sync. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1c57e042683d0dc54d089c04d8f896ee8e838d6

Try again on Windows and Android. Remove compositor thread reference in CompositorBridgeParent. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71e8a64db1430f0834b729ed19e8d8d7bfbf39c4

Okay, no more build errors please. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f8b360145a18db6db2e45f49e4111378370c3df

Switching PCompositorBridge to be a managed protocol revealed that we try to send messages during CompositorBridgeParent::ActorDestroy. When it is a toplevel protocol, it just fails, but for a managed protocol, it trips an assert because it changes the internal protocol state to __Dead before calling ActorDestroy. For some reason this only shows up on Android (!). try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5946cdb548d07a1dde74febd02337db323faf8fd

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

I thought I did this already...apparently not. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26bd0d302d46a16c461cffac7e09880ed041bb22

Fix some old comments that carried over from bug 1331944 lamenting the creation of what this patch set adds ;).

- Rename CompositorManagerChild::mClosed to mCanSend (and invert condition). - Remove CompositorManagerChild::Reinit and make CompositorManagerChild::Init serve both purposes. This ensures we properly reinit the UI process and GPU process PCompositorManager after the GPU process crashes. - Make CompositorManagerChild::IsInitialized return false if we have an instance object but its actor was destroyed. - Fix typos.

Rebase.

- Switch from CompositorManagerChild::Reinit to CompositorManagerChild::Init. See part 2 update.

- Refactor destroy a little. Don't call PCompositorBridgeChild::Send__delete__ if the actor was already destroyed before CompositorBridgeChild::Destroy was called (e.g. GPU process crashed).

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

Sorry for the delay, I'll review this Tuesday.

Comment on attachment 8871727 [details] [diff] [review] Part 2. Add minimal PCompositorManager protocol., v3 Review of attachment 8871727 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, I'd like to see one more version. ::: gfx/layers/ipc/CompositorManagerChild.cpp @@ +13,5 @@ > + > +/* static */ bool > +CompositorManagerChild::IsInitialized() > +{ > + return sInstance && sInstance->mCanSend; Please assert this is only called on the main thread. @@ +28,5 @@ > + > + RefPtr<CompositorManagerParent> parent = > + CompositorManagerParent::CreateSameProcess(); > + sInstance = new CompositorManagerChild(parent, aNamespace); > + return sInstance->mCanSend; You can just return true here. @@ +38,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + if (sInstance) { > + MOZ_ASSERT(sInstance->mNamespace != aNamespace); > + if (NS_WARN_IF(sInstance->mCanSend)) { It should be okay to replace this NS_WARN_IF with a MOZ_RELEASE_ASSERT(sInstance->CanSend()). Instead of accessing mCanSend directly, it's better to add a helper function. An old anti-pattern in IPDL is accessing mCanSend from the wrong thread, and having an accessor lets us assert that that doesn't happen. @@ +62,5 @@ > + if (sInstance->mCanSend) { > + sInstance->Close(); > + } else if (sInstance->OtherPid() == base::GetCurrentProcId()) { > + CompositorManagerParent::ShutdownSameProcess(); > + } You can replace this all with an unconditional sInstance->Close, and remove ShutdownSameProcess. The parent side will still receive an ActorDestroy/DeallocPCompositorManagerParent and it's safe to clear sInstance there. And Close() will just early return if the channel isn't connected. ::: gfx/layers/ipc/CompositorManagerParent.cpp @@ +64,5 @@ > + RefPtr<CompositorManagerParent> parent; > + { > + StaticMutexAutoLock lock(sMutex); > + MOZ_ASSERT(sInstance); > + parent = sInstance.forget(); Do you need to hold a ref outside the lock? I didn't look further ahead but I could see it being necessary if the destructor grabs the lock in a later patch. @@ +90,5 @@ > +} > + > +void > +CompositorManagerParent::ActorDestroy(ActorDestroyReason aReason) > +{ } The { and } should be on separate lines, here and elsewhere. ::: gfx/layers/ipc/PCompositorManager.ipdl @@ +9,5 @@ > +namespace layers { > + > +/** > + * The PCompositorManager protocol is the top-level protocol between the > + * compositor thread and those dependent on it. There is a single logical There are other things that depend on the compositor thread, but do not originate from the UI thread and won't fall under this protocol. So this comment should be explicit about what it manages.

Another header missing in WebRenderScrollData.

Incorporate all review feedback. Fix a double release bug in CompositorManagerParent in the same process case (I don't know how I did not hit it before locally and on try...).

Rebase due to earlier parts changing.

Rebase due to earlier parts changing.

Incorporate review feedback on braces for empty functions from part 2.

Rebase due to earlier parts changing. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94587e09d9bc41d5c1992e78cebef220c662ff09

Comment on attachment 8872993 [details] [diff] [review] Part 2. Add minimal PCompositorManager protocol., v4 Review of attachment 8872993 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/CompositorManagerChild.h @@ +56,5 @@ > + } > + > + bool CanSend() const > + { > + return mCanSend; Can you add a thread assertion here? ::: gfx/layers/ipc/CompositorManagerParent.cpp @@ +89,5 @@ > + if (sInstance == this) { > + // In the same process case, we did not AddRef as it is stored in sInstance. > + sInstance = nullptr; > + } else { > + Release(); It'd be cleaner to just always Release() here, and always AddRef in the in-process case, like you do for CompositorBridgeChild.

Comment on attachment 8872995 [details] [diff] [review] Part 4. Replace PCompositorBridge integration hooks with PCompositorManager., v3 Review of attachment 8872995 [details] [diff] [review]: ----------------------------------------------------------------- In retrospect, PCompositorManager should be called "PCompositorBridge", since it is weird that content processes have a manager. And PCompositorBridge should really have a different name, like "PCompositor" (my fault, it used to be that). And layers::Compositor should be CompositorDevice or something. Anyway, it's fine as-is, I'm just complaining. ::: gfx/ipc/GPUProcessManager.cpp @@ +716,5 @@ > // VideoDeocderManager is only supported in the GPU process, so we allow this to be > // fallible. > CreateContentVideoDecoderManager(aOtherProcess, aOutVideoManager); > + // Allocates 3 namaspaces(for CompositorManagerChild, CompositorBridgeChild and ImageBridgeChild) > + aNamespaces->SetCapacity(3); What is this SetCapacity for?

Comment on attachment 8872997 [details] [diff] [review] Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., v3 Review of attachment 8872997 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/CompositorManagerParent.cpp @@ +158,5 @@ > + StaticMutexAutoLock lock(sMutex); > + MOZ_ASSERT(sInstance == this); > + MOZ_ASSERT(!mPendingCompositorBridges.IsEmpty()); > + > + auto bridge = mPendingCompositorBridges[0].forget().take(); Do we need this mechanism? Can we just construct in-process compositors over IPDL? ::: gfx/layers/ipc/CrossProcessCompositorBridgeParent.cpp @@ +53,5 @@ > > void > CrossProcessCompositorBridgeParent::ActorDestroy(ActorDestroyReason aWhy) > { > + mCanSend = false; I don't see any mCanSend in this class, was it added somewhere else? It doesn't appear to be used.

(In reply to David Anderson [:dvander] from comment #32) > Comment on attachment 8872995 [details] [diff] [review] > Part 4. Replace PCompositorBridge integration hooks with > PCompositorManager., v3 > > Review of attachment 8872995 [details] [diff] [review]: > ----------------------------------------------------------------- > > In retrospect, PCompositorManager should be called "PCompositorBridge", > since it is weird that content processes have a manager. And > PCompositorBridge should really have a different name, like "PCompositor" > (my fault, it used to be that). And layers::Compositor should be > CompositorDevice or something. > > Anyway, it's fine as-is, I'm just complaining. Yes, I had trouble naming this because the natural name was taken, but then I didn't want to confuse PCompositorBridge -> PCompositor with layers::Compositor :). > > ::: gfx/ipc/GPUProcessManager.cpp > @@ +716,5 @@ > > // VideoDeocderManager is only supported in the GPU process, so we allow this to be > > // fallible. > > CreateContentVideoDecoderManager(aOtherProcess, aOutVideoManager); > > + // Allocates 3 namaspaces(for CompositorManagerChild, CompositorBridgeChild and ImageBridgeChild) > > + aNamespaces->SetCapacity(3); > > What is this SetCapacity for? The first alloc just allocate what you need, and after that it will do a power of 2 rounded of what you need. SetCapacity saves us an extra malloc and free. Although looking at it with fresh eyes, it would be preferable if I just slapped it on the stack via AutoTArray by updating the callers.

(In reply to David Anderson [:dvander] from comment #33) > Comment on attachment 8872997 [details] [diff] [review] > Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., > v3 > > Review of attachment 8872997 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/ipc/CompositorManagerParent.cpp > @@ +158,5 @@ > > + StaticMutexAutoLock lock(sMutex); > > + MOZ_ASSERT(sInstance == this); > > + MOZ_ASSERT(!mPendingCompositorBridges.IsEmpty()); > > + > > + auto bridge = mPendingCompositorBridges[0].forget().take(); > > Do we need this mechanism? Can we just construct in-process compositors over > IPDL? > I once did and you can see how that worked in attachment 8869507 [details] [diff] [review] and how it changed to the current method in attachment 8869562 [details] [diff] [review] (see also comment 6). In the original implementation, PCompositorManager::PCompositorBridgeConstructor was sync, which I wanted to avoid. However we had the problem of needing both the child and parent objects in the in process compositor case at the time the child itself was created. Overloading the constructor to have one async and one sync method isn't an option in IPDL (as far as know!). So I inverted the creation of CompositorBridgeParent to be done on the main thread (the current thread context for the in process compositor code), instead of on the compositor thread. Looking at it again, another option would be to block on a condition variable until the compositor thread handles our request. If you prefer that, I can update the patch. Either way I will improve the comments in this area to make it clear what is going on and why. > ::: gfx/layers/ipc/CrossProcessCompositorBridgeParent.cpp > @@ +53,5 @@ > > > > void > > CrossProcessCompositorBridgeParent::ActorDestroy(ActorDestroyReason aWhy) > > { > > + mCanSend = false; > > I don't see any mCanSend in this class, was it added somewhere else? It > doesn't appear to be used. It inherits mCanSend from CompositorBridgeParentBase where it is used to avoid doing some IPC calls after the IPDL actors are destroyed. As I recall, I had to add these checks during testing to avoid shutdown crashes.

Incorporate review feedback. Minor fixes for unified build / missing header messiness.

Incorporate review feedback. Remove SetCapacity and replace arrays with AutoTArray of appropriate size. Remove Moves on array which had no impact (and make no sense with stack allocation).

Added comments about the same process compositor bridge variant. More headers missing but hidden due to unified builds. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32f5c1860fdc7bf3f5f0bac64d42340b7178d203

Comment on attachment 8875361 [details] [diff] [review] Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., v4 Review of attachment 8875361 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, r=me with comments addressed. ::: gfx/layers/ipc/CompositorBridgeChild.cpp @@ +111,2 @@ > { > + if (!mActorDestroyed) { Can we just use mCanSend here and avoid having an extra state bit? It doesn't look like that would change any existing behavior. ::: gfx/layers/ipc/CompositorBridgeChild.h @@ +300,5 @@ > // True until the beginning of the two-step shutdown sequence of this actor. > + bool mCanSend : 1; > + > + // False until the actor is destroyed. > + bool mActorDestroyed : 1; No need for bit packing here. There's like 1 of these objects per session on average. ::: gfx/layers/ipc/CompositorManagerChild.cpp @@ +192,5 @@ > +PCompositorBridgeChild* > +CompositorManagerChild::AllocPCompositorBridgeChild(const CompositorBridgeOptions& aOptions) > +{ > + RefPtr<CompositorBridgeChild> child = new CompositorBridgeChild(this); > + return child.forget().take(); Cleaner to just ->AddRef() and not store in a RefPtr. Or add helpers like AddIPDLReference and DropIPDLReference that wrap AddRef and Release. @@ +199,5 @@ > +bool > +CompositorManagerChild::DeallocPCompositorBridgeChild(PCompositorBridgeChild* aActor) > +{ > + RefPtr<CompositorBridgeChild> child = > + dont_AddRef(static_cast<CompositorBridgeChild*>(aActor)); Cleaner as: static_cast<CompositorBridgeChild*>(aActor)->Release() ::: gfx/layers/ipc/CompositorManagerParent.cpp @@ +85,5 @@ > + // content process setup) and create the parent ahead of time. It will pull > + // the preinitialized parent from the queue when it receives the message and > + // give that to the IPDL plumbing. > + > + StaticMutexAutoLock lock(sMutex); It's worth adding a comment that this lock isn't just to grab |sInstance|, it's also needed since we're modifying mPendingCompositorBridges off the IPDL thread. @@ +96,5 @@ > + > + RefPtr<CompositorBridgeParent> bridge = > + new CompositorBridgeParent(sInstance, aScale, vsyncRate, aOptions, > + aUseExternalSurfaceSize, aSurfaceSize); > + sInstance->mPendingCompositorBridges.AppendElement(bridge); Thanks for adding the explanation above! @@ +147,5 @@ > + new CrossProcessCompositorBridgeParent(this); > + return bridge.forget().take(); > + } > + case CompositorBridgeOptions::TWidgetCompositorOptions: { > + // Only the UI process is allowed to create widget compositors. s/widget compositors/widget compositors in the compositor process/ @@ +174,5 @@ > + StaticMutexAutoLock lock(sMutex); > + MOZ_ASSERT(sInstance == this); > + MOZ_ASSERT(!mPendingCompositorBridges.IsEmpty()); > + > + auto bridge = mPendingCompositorBridges[0].forget().take(); I'd much prefer explicitly leaking a ref for these, as opposed to code that gives the perception that it's just saving refcount traffic. Someone is much less likely to mistakenly refactor code with an explicit AddRef() call. This applies to the other cases as well. @@ +189,5 @@ > +bool > +CompositorManagerParent::DeallocPCompositorBridgeParent(PCompositorBridgeParent* aActor) > +{ > + RefPtr<CompositorBridgeParentBase> bridge = > + dont_AddRef(static_cast<CompositorBridgeParentBase*>(aActor)); Cleaner as: static_cast<CompositorBridgeParentBase*>(aActor)->Release();

Fix try static analysis failure (cannot AddRef directly off RefPtr).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddb45872f832c1acb7951687732519870587c84b (In reply to David Anderson [:dvander] from comment #39) > Comment on attachment 8875361 [details] [diff] [review] > Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., > v4 > > Review of attachment 8875361 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, r=me with comments addressed. > > ::: gfx/layers/ipc/CompositorBridgeChild.cpp > @@ +111,2 @@ > > { > > + if (!mActorDestroyed) { > > Can we just use mCanSend here and avoid having an extra state bit? It > doesn't look like that would change any existing behavior. > The challenge here is that we need SendWillClose to be processed by the other side before we destroy the actor, in order to process any incoming messages. Thus it must wait before issuing Send__delete__ (which will immediately destroy the actor rather than wait for an ack). If I take away mActorDestroyed, I cannot set mCanSend to false after issuing SendWillClose because I need to be able to issue Send__delete__ in the graceful destroy case -- this means new IPC messages could get sent after SendWillClose and that's bad. > ::: gfx/layers/ipc/CompositorBridgeChild.h > @@ +300,5 @@ > > // True until the beginning of the two-step shutdown sequence of this actor. > > + bool mCanSend : 1; > > + > > + // False until the actor is destroyed. > > + bool mActorDestroyed : 1; > > No need for bit packing here. There's like 1 of these objects per session on > average. > Done. > ::: gfx/layers/ipc/CompositorManagerChild.cpp > @@ +192,5 @@ > > +PCompositorBridgeChild* > > +CompositorManagerChild::AllocPCompositorBridgeChild(const CompositorBridgeOptions& aOptions) > > +{ > > + RefPtr<CompositorBridgeChild> child = new CompositorBridgeChild(this); > > + return child.forget().take(); > > Cleaner to just ->AddRef() and not store in a RefPtr. Or add helpers like > AddIPDLReference and DropIPDLReference that wrap AddRef and Release. > Done. > @@ +199,5 @@ > > +bool > > +CompositorManagerChild::DeallocPCompositorBridgeChild(PCompositorBridgeChild* aActor) > > +{ > > + RefPtr<CompositorBridgeChild> child = > > + dont_AddRef(static_cast<CompositorBridgeChild*>(aActor)); > > Cleaner as: > static_cast<CompositorBridgeChild*>(aActor)->Release() > Done. > ::: gfx/layers/ipc/CompositorManagerParent.cpp > @@ +85,5 @@ > > + // content process setup) and create the parent ahead of time. It will pull > > + // the preinitialized parent from the queue when it receives the message and > > + // give that to the IPDL plumbing. > > + > > + StaticMutexAutoLock lock(sMutex); > > It's worth adding a comment that this lock isn't just to grab |sInstance|, > it's also needed since we're modifying mPendingCompositorBridges off the > IPDL thread. > Done. > @@ +96,5 @@ > > + > > + RefPtr<CompositorBridgeParent> bridge = > > + new CompositorBridgeParent(sInstance, aScale, vsyncRate, aOptions, > > + aUseExternalSurfaceSize, aSurfaceSize); > > + sInstance->mPendingCompositorBridges.AppendElement(bridge); > > Thanks for adding the explanation above! > > @@ +147,5 @@ > > + new CrossProcessCompositorBridgeParent(this); > > + return bridge.forget().take(); > > + } > > + case CompositorBridgeOptions::TWidgetCompositorOptions: { > > + // Only the UI process is allowed to create widget compositors. > > s/widget compositors/widget compositors in the compositor process/ > Done. > @@ +174,5 @@ > > + StaticMutexAutoLock lock(sMutex); > > + MOZ_ASSERT(sInstance == this); > > + MOZ_ASSERT(!mPendingCompositorBridges.IsEmpty()); > > + > > + auto bridge = mPendingCompositorBridges[0].forget().take(); > > I'd much prefer explicitly leaking a ref for these, as opposed to code that > gives the perception that it's just saving refcount traffic. Someone is much > less likely to mistakenly refactor code with an explicit AddRef() call. This > applies to the other cases as well. > Done. > @@ +189,5 @@ > > +bool > > +CompositorManagerParent::DeallocPCompositorBridgeParent(PCompositorBridgeParent* aActor) > > +{ > > + RefPtr<CompositorBridgeParentBase> bridge = > > + dont_AddRef(static_cast<CompositorBridgeParentBase*>(aActor)); > > Cleaner as: > static_cast<CompositorBridgeParentBase*>(aActor)->Release(); Done.

No functional change, just added a comment in AfterDestroy to more clearly explain why it needs to check mActorDestroyed instead of mCanSend.

(In reply to Andrew Osmond [:aosmond] from comment #41) > Created attachment 8875697 [details] [diff] [review] > Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., > v5 [carries r=dvander] > > try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=ddb45872f832c1acb7951687732519870587c84b > > (In reply to David Anderson [:dvander] from comment #39) > > Comment on attachment 8875361 [details] [diff] [review] > > Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., > > v4 > > > > Review of attachment 8875361 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Looks good, r=me with comments addressed. > > > > ::: gfx/layers/ipc/CompositorBridgeChild.cpp > > @@ +111,2 @@ > > > { > > > + if (!mActorDestroyed) { > > > > Can we just use mCanSend here and avoid having an extra state bit? It > > doesn't look like that would change any existing behavior. > > > > The challenge here is that we need SendWillClose to be processed by the > other side before we destroy the actor, in order to process any incoming > messages. Thus it must wait before issuing Send__delete__ (which will > immediately destroy the actor rather than wait for an ack). If I take away > mActorDestroyed, I cannot set mCanSend to false after issuing SendWillClose > because I need to be able to issue Send__delete__ in the graceful destroy > case -- this means new IPC messages could get sent after SendWillClose and > that's bad. Out of curiosity were you following the big comment in CompositorBridgeChild.cpp, saying we have to process delayed shmem messages? That comment is referring to an old quirk. Shmems used to be forcefully deleted during MessageChannelChannel::Close, so gfx had to paper over a lot of weird behavior. Now they are refcounted. Furthermore shmems are attached to IToplevelProtocol, not normal actors, so PCompositorBridge isn't even holding shmems alive anymore. And to top it off PCompositorBridge now supports being killed at any time, for the compositor process, and it's hard to imagine what state needs to be received before tearing down everything. It might just be dead code. It's fine as-is since you're just preserving old behavior, but if you actually had tried reverting it at some point and ran into problems, I'd be curious what they were.

I could also see the parent sending random messages back during shutdown and those triggering processing errors - if that's the case maybe we could make Send__delete__ synchronous and drop WillClose. (But no need to do it in this bug.)

(In reply to David Anderson [:dvander] from comment #43) > Out of curiosity were you following the big comment in > CompositorBridgeChild.cpp, saying we have to process delayed shmem messages? > > That comment is referring to an old quirk. Shmems used to be forcefully > deleted during MessageChannelChannel::Close, so gfx had to paper over a lot > of weird behavior. Now they are refcounted. Furthermore shmems are attached > to IToplevelProtocol, not normal actors, so PCompositorBridge isn't even > holding shmems alive anymore. And to top it off PCompositorBridge now > supports being killed at any time, for the compositor process, and it's hard > to imagine what state needs to be received before tearing down everything. > It might just be dead code. > > It's fine as-is since you're just preserving old behavior, but if you > actually had tried reverting it at some point and ran into problems, I'd be > curious what they were. I approached this fairly cautiously so I didn't even think to try to remove it. However I can file a follow up bug and see if we can get rid of it. (In reply to David Anderson [:dvander] from comment #44) > I could also see the parent sending random messages back during shutdown and > those triggering processing errors - if that's the case maybe we could make > Send__delete__ synchronous and drop WillClose. (But no need to do it in this > bug.) I'll check that out too.

Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/12a093fb86a4 Part 1. Add missing headers and other build housekeeping. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/961e782cf7c7 Part 2. Add minimal PCompositorManager protocol. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/144ac6cc76f8 Part 3. Move PCompositorBridge toplevel protocol overrides to PCompositorManager. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/f4954c824f5c Part 4. Replace PCompositorBridge integration hooks with PCompositorManager. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1dee5a6c42 Part 5. Make PCompositorManager the manager protocol of PCompositorBridge. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/11fc0da380a2 Part 6. Remove CompositorThreadHolder reference from CompositorBridgeParent. r=dvander

Backing these out for build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=106792384&repo=mozilla-inbound So far, just seeing this on Windows and Android, could be more to come. https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0caf02d29423964f43f0bb83b2cb17ea33144d

I don't fully understand why this fixes the Android build (locally anyways), but the unified build reordering revealed PVideoDecoderManager somehow has a dependency on PTexture. The .pp file indicated it should have everything it needs already before this change *sigh*. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=529074d9b7e742e1a6f8ffee1c3810551d909e14

(In reply to Andrew Osmond [:aosmond] from comment #48) > Created attachment 8877607 [details] [diff] [review] > Part 1. Add missing headers and other build housekeeping., v3 [carries > r=dvander] > > I don't fully understand why this fixes the Android build (locally anyways), > but the unified build reordering revealed PVideoDecoderManager somehow has a > dependency on PTexture. The .pp file indicated it should have everything it > needs already before this change *sigh*. > > try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=529074d9b7e742e1a6f8ffee1c3810551d909e14 For posterity, the undefined symbols are defined in GfxMessageUtils.h. But that header is already included implicitly via a few different paths, including MediaIPCUtils.h. Including said header explicitly has no impact. I tried including the includes from PTexture into this protocol, but the only one that satisfied it was PVideoBridge -- which includes PTexture.

Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4b887f406a Part 1. Add missing headers and other build housekeeping. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/b8eb40812053 Part 2. Add minimal PCompositorManager protocol. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2bc148ecd1 Part 3. Move PCompositorBridge toplevel protocol overrides to PCompositorManager. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/ee612156fa6f Part 4. Replace PCompositorBridge integration hooks with PCompositorManager. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/32de56131efd Part 5. Make PCompositorManager the manager protocol of PCompositorBridge. r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/d81dd3f033be Part 6. Remove CompositorThreadHolder reference from CompositorBridgeParent. r=dvander

https://hg.mozilla.org/mozilla-central/rev/cb4b887f406a https://hg.mozilla.org/mozilla-central/rev/b8eb40812053 https://hg.mozilla.org/mozilla-central/rev/8b2bc148ecd1 https://hg.mozilla.org/mozilla-central/rev/ee612156fa6f https://hg.mozilla.org/mozilla-central/rev/32de56131efd https://hg.mozilla.org/mozilla-central/rev/d81dd3f033be