Create parent protocol for PCompositorBridge

RESOLVED FIXED in Firefox 56

Status

()

Core
Graphics: WebRender
P3
normal
RESOLVED FIXED
5 months ago
4 days ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(6 attachments, 26 obsolete attachments)

8.64 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
3.99 KB, patch
dvander
: review+
Details | Diff | Splinter Review
43.43 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
14.11 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
45.83 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
1.62 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
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.
(Assignee)

Updated

5 months ago
Assignee: nobody → aosmond
Blocks: 1311790, 1331944
Status: NEW → ASSIGNED
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Priority: -- → P3
Whiteboard: gfx-noted
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.
Created attachment 8869399 [details] [diff] [review]
[WIP] Add PCompositorBridge protocol manager, v1

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.
Created attachment 8869440 [details] [diff] [review]
[WIP] Add PCompositorBridge protocol manager, v2

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.
Attachment #8869399 - Attachment is obsolete: true
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00d574e4521bca0eaea352d051f5293fe896e25e
Created attachment 8869507 [details] [diff] [review]
[WIP] Add PCompositorBridge protocol manager, v3

Fix build errors, some new comments/asserts.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=509e9403cdd62835d0455637dae39dd114a5cfa6
Attachment #8869440 - Attachment is obsolete: true
Created attachment 8869562 [details] [diff] [review]
[WIP] Add PCompositorBridge protocol manager, v4

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
Attachment #8869507 - Attachment is obsolete: true
Created attachment 8869594 [details] [diff] [review]
[WIP] Add PCompositorBridge protocol manager, v5

Try again on Windows and Android. Remove compositor thread reference in CompositorBridgeParent.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71e8a64db1430f0834b729ed19e8d8d7bfbf39c4
Attachment #8869562 - Attachment is obsolete: true
Created attachment 8870392 [details] [diff] [review]
[WIP] Add PCompositorBridge protocol manager, v6

Okay, no more build errors please.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f8b360145a18db6db2e45f49e4111378370c3df
Attachment #8869594 - Attachment is obsolete: true
Created attachment 8870427 [details] [diff] [review]
[WIP] Add PCompositorBridge protocol manager, v7

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
Attachment #8870392 - Attachment is obsolete: true
Created attachment 8870803 [details] [diff] [review]
Part 1. Add missing headers and other build housekeeping., v1
Attachment #8870427 - Attachment is obsolete: true
Created attachment 8870804 [details] [diff] [review]
Part 2. Add minimal PCompositorManager protocol., v1
Created attachment 8870805 [details] [diff] [review]
Part 3. Move PCompositorBridge toplevel protocol overrides to PCompositorManager., v1
Created attachment 8870806 [details] [diff] [review]
Part 4. Replace PCompositorBridge integration hooks with PCompositorManager., v1
Created attachment 8870807 [details] [diff] [review]
Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., v1
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b8a4a64f3950c9e2a914c629b2f5d3fb86a3081
Created attachment 8870829 [details] [diff] [review]
Part 6. Remove CompositorThreadHolder reference from CompositorBridgeParent., v1

I thought I did this already...apparently not.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26bd0d302d46a16c461cffac7e09880ed041bb22
(Assignee)

Updated

5 months ago
Attachment #8870803 - Flags: review?(dvander)
(Assignee)

Updated

5 months ago
Attachment #8870804 - Flags: review?(dvander)
(Assignee)

Updated

5 months ago
Attachment #8870805 - Flags: review?(dvander)
(Assignee)

Updated

5 months ago
Attachment #8870806 - Flags: review?(dvander)
(Assignee)

Updated

5 months ago
Attachment #8870807 - Flags: review?(dvander)
Created attachment 8870863 [details] [diff] [review]
Part 2. Add minimal PCompositorManager protocol., v2

Fix some old comments that carried over from bug 1331944 lamenting the creation of what this patch set adds ;).
Attachment #8870804 - Attachment is obsolete: true
Attachment #8870804 - Flags: review?(dvander)
Attachment #8870863 - Flags: review?(dvander)
Created attachment 8871727 [details] [diff] [review]
Part 2. Add minimal PCompositorManager protocol., v3

- 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.
Attachment #8870863 - Attachment is obsolete: true
Attachment #8870863 - Flags: review?(dvander)
Created attachment 8871729 [details] [diff] [review]
Part 3. Move PCompositorBridge toplevel protocol overrides to PCompositorManager., v2

Rebase.
Attachment #8870805 - Attachment is obsolete: true
Attachment #8870805 - Flags: review?(dvander)
Attachment #8871729 - Flags: review?(dvander)
(Assignee)

Updated

5 months ago
Attachment #8871727 - Flags: review?(dvander)
Created attachment 8871730 [details] [diff] [review]
Part 4. Replace PCompositorBridge integration hooks with PCompositorManager., v2

- Switch from CompositorManagerChild::Reinit to CompositorManagerChild::Init. See part 2 update.
Attachment #8870806 - Attachment is obsolete: true
Attachment #8870806 - Flags: review?(dvander)
Attachment #8871730 - Flags: review?(dvander)
Created attachment 8871734 [details] [diff] [review]
Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., v2

- 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).
Attachment #8870807 - Attachment is obsolete: true
Attachment #8870807 - Flags: review?(dvander)
Attachment #8871734 - Flags: review?(dvander)
Created attachment 8871738 [details] [diff] [review]
Part 6. Remove CompositorThreadHolder reference from CompositorBridgeParent., v2

Rebase.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3bbf03701ffc52448ee4163a729befbbae0e20c
Attachment #8870829 - Attachment is obsolete: true
Attachment #8871738 - Flags: review?(dvander)
Sorry for the delay, I'll review this Tuesday.
Attachment #8870803 - Flags: review?(dvander) → review+
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.
Attachment #8871727 - Flags: review?(dvander)
Attachment #8871729 - Flags: review?(dvander) → review+
Created attachment 8872991 [details] [diff] [review]
Part 1. Add missing headers and other build housekeeping., v2 [carries r=dvander]

Another header missing in WebRenderScrollData.
Attachment #8870803 - Attachment is obsolete: true
Attachment #8872991 - Flags: review+
Created attachment 8872993 [details] [diff] [review]
Part 2. Add minimal PCompositorManager protocol., v4

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...).
Attachment #8871727 - Attachment is obsolete: true
Attachment #8872993 - Flags: review?(dvander)
Created attachment 8872994 [details] [diff] [review]
Part 3. Move PCompositorBridge toplevel protocol overrides to PCompositorManager., v3 [carries r=dvander]

Rebase due to earlier parts changing.
Attachment #8871729 - Attachment is obsolete: true
Attachment #8872994 - Flags: review+
Created attachment 8872995 [details] [diff] [review]
Part 4. Replace PCompositorBridge integration hooks with PCompositorManager., v3

Rebase due to earlier parts changing.
Attachment #8871730 - Attachment is obsolete: true
Attachment #8871730 - Flags: review?(dvander)
Attachment #8872995 - Flags: review?(dvander)
Created attachment 8872997 [details] [diff] [review]
Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., v3

Incorporate review feedback on braces for empty functions from part 2.
Attachment #8871734 - Attachment is obsolete: true
Attachment #8871734 - Flags: review?(dvander)
Attachment #8872997 - Flags: review?(dvander)
Created attachment 8872998 [details] [diff] [review]
Part 6. Remove CompositorThreadHolder reference from CompositorBridgeParent., v3

Rebase due to earlier parts changing.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94587e09d9bc41d5c1992e78cebef220c662ff09
Attachment #8871738 - Attachment is obsolete: true
Attachment #8871738 - Flags: review?(dvander)
Attachment #8872998 - Flags: review?(dvander)
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.
Attachment #8872993 - Flags: review?(dvander) → review+
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?
Attachment #8872995 - Flags: review?(dvander) → review+
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.
Created attachment 8875358 [details] [diff] [review]
Part 2. Add minimal PCompositorManager protocol., v5 [carries r=dvander]

Incorporate review feedback. Minor fixes for unified build / missing header messiness.
Attachment #8872993 - Attachment is obsolete: true
Attachment #8875358 - Flags: review+
Created attachment 8875360 [details] [diff] [review]
Part 4. Replace PCompositorBridge integration hooks with PCompositorManager., v4 [carries r=dvander]

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).
Attachment #8872995 - Attachment is obsolete: true
Attachment #8875360 - Flags: review+
Created attachment 8875361 [details] [diff] [review]
Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., v4

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
Attachment #8872997 - Attachment is obsolete: true
Attachment #8872997 - Flags: review?(dvander)
Attachment #8875361 - Flags: review?(dvander)
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();
Attachment #8875361 - Flags: review?(dvander) → review+
Created attachment 8875696 [details] [diff] [review]
Part 2. Add minimal PCompositorManager protocol., v6 [carries r=dvander]

Fix try static analysis failure (cannot AddRef directly off RefPtr).
Attachment #8875358 - Attachment is obsolete: true
Attachment #8875696 - Flags: review+
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.

> ::: 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.
Attachment #8875361 - Attachment is obsolete: true
Attachment #8875697 - Flags: review+
Created attachment 8875702 [details] [diff] [review]
Part 5. Make PCompositorManager the manager protocol of PCompositorBridge., v6 [carries r=dvander]

No functional change, just added a comment in AfterDestroy to more clearly explain why it needs to check mActorDestroyed instead of mCanSend.
Attachment #8875697 - Attachment is obsolete: true
Attachment #8875702 - Flags: review+
(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.)
Attachment #8872998 - Flags: review?(dvander) → review+
(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.

Comment 46

4 months ago
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
Flags: needinfo?(aosmond)
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
Attachment #8872991 - Attachment is obsolete: true
Flags: needinfo?(aosmond)
Attachment #8877607 - Flags: review+
(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.

Comment 50

4 months ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

4 months ago
Depends on: 1373540
(Assignee)

Updated

4 months ago
Depends on: 1374278
(Assignee)

Updated

4 months ago
Depends on: 1376590
(Assignee)

Updated

4 months ago
Depends on: 1377869
(Assignee)

Updated

2 months ago
Depends on: 1389021

Updated

4 days ago
Depends on: 1408514
You need to log in before you can comment on or make changes to this bug.