Closed Bug 1289650 Opened 3 years ago Closed 3 years ago

Rearchitect APZ IPDL for moving to a compositor process

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

Attachments

(5 files, 1 obsolete file)

Enable APZ when we have a RemoteCompositorSession, and fix any issues related to this.
The first roadblock is getting GeckoContentControllers to work in the GPU process. Most of the work is already there, and just had to be moved around. We have two kinds of GeckoContentControllers to worry about, ones living in the chrome/parent process, and those living in the content/child process. Before these patches, the content controllers living in the chrome process were just shared in memory, while the content controllers in the content process were remoted using the PAPZ protocol. The PAPZ protocol also served as a method to talk with the APZCTreeManager.

These patches remove the methods related to APZCTreeManager from PAPZ, and instead use PAPZCTreeManager. They then turn PAPZ into an interface just for GeckoContentControllers. This requires converting code in the child process to use PAPZCTreeManager, and also moving PAPZ to run on PCompositorBridge. Once this is done, we should be able to remote any GeckoContentController.
kats: sorry this is a lot! I wrote up some notes about what's going on and was going to put them as descriptions on the patches, but then decided to give mozreview a shot, which doesn't seem to have a way to add a description? Maybe I could've added them in a seperate line in the commit description.

The first patch removes methods from PAPZ that correspond to communicating with the APZCTreeManager. Instead of using PAPZ then, every TabChild creates a PAPZCTreeManager over it's PCompositorBridge, and uses that to communicate with its APZCTreeManager. PAPZ originally used an implicit layer id that was used to create it in some of it's messages, and so that had to be made explicit when moving over to PAPZCTreeManager. A few of PAPZCTreeManager's methods were moved around, and one method of PAPZ was renamed but no parameters were changed.

The second patch moves PAPZ to be a subprotocol of PCompositorBridge. Some work had to be done to remove dependencies on TabParent from RemoteContentController, because it might not be living in the main process anymore so it might not have access to it.

PAPZ was originally created in ContentParent when a LayerTreeId was allocated. This doesn't work anymore because the child needs to construct PAPZ on a different protocol. Now when that happens a message is sent to TabChild notifying it that a LayerTreeId was allocated, which then will ask the Compositor to construct a PAPZ.

RemoteContentController can no longer access the TabParent of the tab it is communicating with, because that lives in the main process which may be different. The two places impacted are when we send a MouseScrollTestEvent, and when we send a HandleTap. MouseScrollTestEvent was moved from PBrowser to PAPZ, which then hands off the event to the TabChild. HandleTap needed the TabParent because it added the child processes offset to the tap event. Looking at TabChild\TabParent, they track the GetChildProcessOffset's value and sync it between parent and child (ChromeDisplacement). So the child has it's own offset. This patch moves the translation from in RemoteContentController to inside APZChild, so we don't need to sync it to the GPU process.

There are also some threading changes to RemoteContentController because it is no longer created and managed on the main thread, but now lives on the compositor thread. Which complicates things, but I think is workable.

One last issue was APZChild manually deleting it's protcol when a Destroy message was received. I'm not sure if this is necessary, and it was throwing some IPDL errors. That's been removed, but if it is necessary can be looked into more.

And finally the third patch takes the content process specific logic out of APZChild and puts it in a GeckoContentController. Then APZChild is converted to take a GeckoContentController and forward the messages it receives to it. This makes APZChild into a simple wrapper around any GeckoContentController, so that we can use APZChild in the main process for ChromeContentControllers.
Comment on attachment 8780331 [details]
Bug 1289650 - Use PAPZCTreeManager in content process instead of PAPZ.

https://reviewboard.mozilla.org/r/71056/#review69192

I'm still jetlagged so not in the best state to review this right now but I'll look through it tomorrow. I did have one concern that came to mind immediately after reading your comments and taking a quick look at the first patch.

::: dom/ipc/TabChild.cpp:711
(Diff revision 2)
> -  return mAPZChild->SendUpdateZoomConstraints(aPresShellId, aViewId,
> -                                              aConstraints);
> +  ScrollableLayerGuid guid = ScrollableLayerGuid(mLayersId, aPresShellId, aViewId);
> +
> +  mApzcTreeManager->UpdateZoomConstraints(guid, aConstraints);

In general the parent process should not be relying on a layers id coming from the child process. It's a security issue - if a child process is compromised/hijacked, then it could send a layers id for a different process or tab in this message, and do malicious things.

That originally came up in bug 898563, where I added code to ensure the parent process did not rely on a layers id being sent over from a child process in an IPDL message. peterv had to maintain that invariant in bug 1020199 when he did some of his refactoring. We'll have to find some way to maintain it here as well.
Comment on attachment 8780331 [details]
Bug 1289650 - Use PAPZCTreeManager in content process instead of PAPZ.

https://reviewboard.mozilla.org/r/71056/#review69480

Ok, so I gave it some thought and do think that the layers id being passed from the child to the parent the way you are doing in this patch is a problem. However there's a relatively simple fix, I think. At the point that the APZCTreeManagerParent is created (in CompositorBridgeParent::AllocPAPZCTreeManagerParent and CrossProcessCompositorBridgeParent::AllocPAPZCTreeManagerParent), you have access to the layers id. You should stash that layers id as a member inside the APZCTreeManagerParent, so that you don't need to rely on it being sent over from the child process. You should be able to undo all the guid creation you added in TabChild, and move it into the APZCTreeManagerParent instead (if needed).

The rest of the patch mostly looks ok, but I'll do another pass through once you've made the above changes.

::: gfx/layers/ipc/APZChild.h:40
(Diff revision 2)
> -                             const Modifiers& aModifiers,
> +                     const Modifiers& aModifiers,
> -                             const ScrollableLayerGuid& aGuid,
> +                     const ScrollableLayerGuid& aGuid,
> -                             const uint64_t& aInputBlockId,
> +                     const uint64_t& aInputBlockId,
> -                             const bool& aCallTakeFocusForClickFromTap) override;
> +                     const bool& aCallTakeFocusForClickFromTap) override;
>  
> -  virtual bool RecvNotifyAPZStateChange(const ViewID& aViewId,
> +  bool RecvNotifyAPZStateChange(const ScrollableLayerGuid& aGuid,

This change of ViewID -> ScrollableLayerGuid also seems unnecessary, can you revert it?
Attachment #8780331 - Flags: review?(bugmail) → review-
Comment on attachment 8780331 [details]
Bug 1289650 - Use PAPZCTreeManager in content process instead of PAPZ.

https://reviewboard.mozilla.org/r/71056/#review69480

Okay that makes sense. I have a patch that validates any LayersIds in APZCTreeManagerParent (which is the receiver of all the code that was removed from RemoteContentController) to the LayerId used to construct it, which does what you said. The rest of RemoteContentController (which is now the GeckoContentController interface) should not need any validation code because it only really sends out any messages. The only message it receives is UpdateHitRegions which seems fine.

The only problem is securing the construction of PAPZCTreeManager, because the content child directly asks the GPU process for a PAPZCTreeManager. Right now we just let them get a PAPZCTreeManager for any layer tree, which prevents the patch I have from being useful.

The GPU process is probably going to need to have a synced copy of the Tab->LayerIds map, so it can validate whether it's okay for that content process to access that layer tree's APZCTreeManager. I think syncing it through GPUProcessManager::{Allocate,Deallocate}LayerId will work, but it will require a little bit of work to get right.

> This change of ViewID -> ScrollableLayerGuid also seems unnecessary, can you revert it?

Yes, I noticed that last night and that should not be the case. I think that just got mixed up in the patch.
(In reply to Ryan Hunt [:rhunt] from comment #12)
> The only problem is securing the construction of PAPZCTreeManager, because
> the content child directly asks the GPU process for a PAPZCTreeManager.
> Right now we just let them get a PAPZCTreeManager for any layer tree, which
> prevents the patch I have from being useful.

Hm, that's an interesting point. It seems like the PLayerTransactionParent creation suffers from a similar problem, maybe. For example at [1] the id is passed in from the child process, and the parent hooks up the LayerTransactionParent with the layer tree for that id.

dvander, do you know if there's some way this is protected against malicious child processes passing in layers ids for different processes? If there is some protection it's not obvious to me.

[1] http://searchfox.org/mozilla-central/rev/03b3c20a6ec60435feb995a2a23c5926188c85a1/gfx/layers/ipc/CompositorBridgeParent.cpp#2421
Flags: needinfo?(dvander)
I talked with dvander, and I think the solution is to hook into {Allocate,Deallocate}LayerTreeId in ContentParent at [1], and sync that ownership information to the compositor process (if it exists). Then in the compositor process we will check at creation for PAPZ, PAPZCTreeManager, and potentially PLayerTransaction to see whether that layerId can be accessed by that PID, which we can get from IPDL. We just need to be careful to make sure we don't break nested content processes.

[1] http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1721
Flags: needinfo?(dvander)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)

> dvander, do you know if there's some way this is protected against malicious
> child processes passing in layers ids for different processes? If there is
> some protection it's not obvious to me.

I don't see that we protect against this, either.
Comment on attachment 8780333 [details]
Bug 1289650 - Check the owning PID when creating PAPZCTreeManager, PLayerTransaction.

https://reviewboard.mozilla.org/r/71060/#review70214

::: dom/ipc/ContentParent.cpp:1759
(Diff revision 3)
> -  } else {
>      // You can't deallocate layer tree ids that you didn't allocate
>      KillHard("DeallocateLayerTreeId");
>    }
> +
> +  GPUProcessManager::Get()->DeallocateLayerTreeId(aId);

Looks like we were never removing the mapping from NestedBrowserLayerIds(). We should do that, but maybe in another bug since it's a pre-existing issue.

::: gfx/ipc/GPUParent.cpp:151
(Diff revision 3)
>  }
>  
> +bool
> +GPUParent::RecvAddLayerTreeIdMapping(const uint64_t& aLayersId, const ProcessId& aOwnerId)
> +{
> +  LayerTreeOwnerTracker::Get()->Map(aLayersId, aOwnerId);

Arguments are backwards here - LTOT::Map takes process id first, layers id second. Please make sure you keep the argument order consistent throughout. My preference is to always have the layers id first, process id second, and to have the std::map from layers id -> process id.

::: gfx/layers/ipc/CompositorBridgeChild.cpp:166
(Diff revision 3)
> +  if (!apzEnabled)
> +    return nullptr;
> +

braces

::: gfx/layers/ipc/CompositorBridgeChild.cpp:170
(Diff revision 3)
> +  if (!child)
> +    return nullptr;

braces

::: gfx/layers/ipc/LayerTreeOwnerTracker.h:56
(Diff revision 3)
> +
> +private:
> +  LayerTreeOwnerTracker();
> +
> +  mozilla::Mutex mLayerIdsLock;
> +  std::map<base::ProcessId, std::set<uint64_t> > mLayerIds;

Seems like it would be simpler if you reversed the map, so that the layersId was the key and the processId was the value. That way you wouldn't need the nested set
Comment on attachment 8780333 [details]
Bug 1289650 - Check the owning PID when creating PAPZCTreeManager, PLayerTransaction.

https://reviewboard.mozilla.org/r/71060/#review70216

Minusing for the argument ordering mismatch. The rest of the patch looks fine. Thanks!
Attachment #8780333 - Flags: review?(bugmail) → review-
Comment on attachment 8780333 [details]
Bug 1289650 - Check the owning PID when creating PAPZCTreeManager, PLayerTransaction.

https://reviewboard.mozilla.org/r/71060/#review70436

Looks good. Some nits and stuff below.

::: gfx/layers/ipc/CompositorBridgeParent.cpp:2519
(Diff revision 4)
>  }
>  
>  bool
> +CrossProcessCompositorBridgeParent::RecvAsyncPanZoomEnabled(const uint64_t& aLayersId, bool* aHasAPZ)
> +{
> +  MonitorAutoLock lock(*sIndirectLayerTreesLock);

Probably a good idea to also do the IsMapped check here. I don't think there's a reason a child process should be wanting to check if APZ is enabled for a layers id for some random other process.

::: gfx/layers/ipc/LayerTreeOwnerTracker.h:26
(Diff revision 4)
> +namespace layers {
> +
> +/**
> + * A utility class for tracking which content processes should be allowed
> + * to access which layer trees. The data is synced between main process
> + * and the GPU process. This is a seperate class from GPUProcessManager

s/seperate/separate/

::: gfx/layers/ipc/LayerTreeOwnerTracker.h:41
(Diff revision 4)
> +  static void Initialize();
> +  static void Shutdown();
> +  static LayerTreeOwnerTracker* Get();
> +
> +  /**
> +   * Map aLayersId and aProcessId together so that that process 

nit: trailing whitespace

::: gfx/layers/ipc/LayerTreeOwnerTracker.cpp:29
(Diff revision 4)
> +}
> +
> +void
> +LayerTreeOwnerTracker::Initialize()
> +{
> +  sSingleton = new LayerTreeOwnerTracker();

For safety add a MOZ_ASSERT(!sSingleton) at the start of this function

::: gfx/layers/ipc/LayerTreeOwnerTracker.cpp:47
(Diff revision 4)
> +}
> +
> +void
> +LayerTreeOwnerTracker::Map(uint64_t aLayersId, base::ProcessId aProcessId)
> +{
> +  MutexAutoLock lock(mLayerIdsLock);

Might be better to scope this lock so that it covers just the assignment into mLayerIds, rather than also covering the IPC stuff below.
Attachment #8780333 - Flags: review?(bugmail) → review+
Comment on attachment 8780333 [details]
Bug 1289650 - Check the owning PID when creating PAPZCTreeManager, PLayerTransaction.

https://reviewboard.mozilla.org/r/71060/#review70214

> Looks like we were never removing the mapping from NestedBrowserLayerIds(). We should do that, but maybe in another bug since it's a pre-existing issue.

Yes that makes sense to me, I think that would be a good addition in another bug.

> Seems like it would be simpler if you reversed the map, so that the layersId was the key and the processId was the value. That way you wouldn't need the nested set

That makes sense, I ported this code from ContentParent without thinking about that. That would make it so that each layerId can be accessed by only one process, which I think is how it works now. I also did a try run to see if there would be any problems and it looks good to me. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c08b7edfafae
Comment on attachment 8780331 [details]
Bug 1289650 - Use PAPZCTreeManager in content process instead of PAPZ.

https://reviewboard.mozilla.org/r/71056/#review70456

This looks pretty good with the previous patch. However see the comment below on AdoptChild, we will need to handle that properly before this can land.

::: gfx/layers/ipc/APZChild.h:40
(Diff revision 4)
> -                             const Modifiers& aModifiers,
> +                     const Modifiers& aModifiers,
> -                             const ScrollableLayerGuid& aGuid,
> +                     const ScrollableLayerGuid& aGuid,
> -                             const uint64_t& aInputBlockId,
> +                     const uint64_t& aInputBlockId,
> -                             const bool& aCallTakeFocusForClickFromTap) override;
> +                     const bool& aCallTakeFocusForClickFromTap) override;
>  
> -  virtual bool RecvNotifyAPZStateChange(const ViewID& aViewId,
> +  bool RecvNotifyAPZStateChange(const ScrollableLayerGuid& aGuid,

Undo this change from ViewID to ScrollableLayerGuid

::: gfx/layers/ipc/RemoteContentController.cpp
(Diff revision 4)
> -  // Clear the cached APZCTreeManager.
> -  MutexAutoLock lock(mMutex);
> -  mApzcTreeManager = nullptr;

So this ChildAdopted() call is no longer needed, but we'll need equivalent code somewhere else. This codepath is executed when e.g. a tab is dragged from one window to another, which results in the layers id getting shifted from one compositor to another. I believe that in the new world we will need to re-bind the APZCTreeManagerParent to a different APZCTreeManager instance when this happens. It might be a little tricky - I think we might need to keep a handle to the APZCTreeManagerParent in the sIndirectLayerTrees structure, and then in CompositorBridgeParent::RecvAdoptChild we would update the APZCTreeManager on it (the code in that function already updates other things like the mParent, mLayerManager, etc.).
Attachment #8780331 - Flags: review?(bugmail) → review-
Comment on attachment 8780331 [details]
Bug 1289650 - Use PAPZCTreeManager in content process instead of PAPZ.

https://reviewboard.mozilla.org/r/71056/#review71030

::: gfx/layers/ipc/CompositorBridgeParent.cpp:1810
(Diff revision 6)
>    // Calling ChildAdopted on controller will acquire a lock, to avoid a
>    // potential deadlock between that lock and sIndirectLayerTreesLock we
>    // release sIndirectLayerTreesLock first before calling ChildAdopted.

Remove this comment as it's no longer true. Although to be honest I don't know what's running on what thread anymore, and maybe we do need add a mutex in APZCTreeManagerParent to guard mTreeManager? I'm not sure.
Attachment #8780331 - Flags: review?(bugmail) → review+
Comment on attachment 8780332 [details]
Bug 1289650 - Move PAPZ from PContent to PCompositorBridge.

https://reviewboard.mozilla.org/r/71058/#review71034

This looks good. Just one main thing to fix, see below. It should be relatively straightforward. (Also sorry to keep r-'ing these patches one at a time, it's taking me a long time to think through these changes and wrap my head around everything).

I'd also like dvander to review all the actor creation/deletion stuff in this patch - it seems fine to me but I'd feel happier with a second pair of eyes on it because I'm no expert on that stuff.

::: gfx/layers/apz/src/APZCTreeManager.cpp:630
(Diff revision 6)
> -  NS_DispatchToMainThread(NewRunnableMethod(
> +  state->mController->NotifyFlushComplete();
> -    state->mController, &GeckoContentController::NotifyFlushComplete));

See my comment below, this will need to be changed to state->mController->DispatchToRepaintThread. Note that the thread redispatch here is unconditional, so that it always follows any inflight repaint requests, and we need to maintain that.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
(Diff revision 6)
> -  // Reinvoke this method on the main thread if it's not there already. It's
> -  // important to do this before the call to CalculatePendingDisplayPort, so
> -  // that CalculatePendingDisplayPort uses the most recent available version of
> -  // mFrameMetrics, just before the paint request is dispatched to content.
> -  if (!NS_IsMainThread()) {
> -    // use the local variable to resolve the function overload.
> -    auto func = static_cast<void (AsyncPanZoomController::*)()>
> -        (&AsyncPanZoomController::RequestContentRepaint);
> -    NS_DispatchToMainThread(NewRunnableMethod(this, func));
> -    return;
> -  }

This code that you're deleting is important, and necessary to avoid race conditions in the order of repaint requests.

It looks like you're shifting this code to the ChromeProcessController::RequestContentRepaint (for the in-process case) and the RemoteContentController::RequestContentRepaint (for the gpu process case), but those functions happen later, and so it's possible to get an improper sequence of repaint requests. In a nutshell some of the calls will already be on the right thread, and they might "overtake" the ones the have to be redispatched from one thread to another.

I would rather keep the redispatch inside this function. I presume the problem with that is this code hard-codes NS_IsMainThread and NS_DispatchToMainThread, whereas in the gpu process case we want to be using the compositor thread to send the IPDL message. I propose that we handle that by adding a two new APIs on GeckoContentController that effectively abstract NS_IsMainThread and NS_DispatchToMainThread.

So this code in AsyncPanZoomController::RequestContentRepaint would look like:

RefPtr<GeckoContentController> controller = GetGeckoContentController();
if (!controller) {
  return;
}
if (!controller->IsRepaintThread()) {
  auto func = ...;
  controller->DispatchToRepaintThread(..func..);
  return;
}
MOZ_ASSERT(controller->IsRepaintThread());

and the the ChromeProcessController implementations of IsRepaintThread and DispatchToRepaintThread would just invoke NS_IsMainThread and NS_DispatchToMainThread. The RemoteContentController implementations would check MessageLoop::current() == mCompositorThread and do mCompositorThread->PostTask.

Does that sound reasonable?

::: gfx/layers/ipc/CompositorBridgeParent.cpp:2565
(Diff revision 6)
> +
> +  RefPtr<RemoteContentController> controller = new RemoteContentController(aLayersId);
> +
> +  MonitorAutoLock lock(*sIndirectLayerTreesLock);
> +  CompositorBridgeParent::LayerTreeState& state = sIndirectLayerTrees[aLayersId];
> +  state.mController = controller;

Add a MOZ_ASSERT(!state.mController) before this assignment.

::: gfx/layers/ipc/RemoteContentController.cpp:46
(Diff revision 6)
> -  MOZ_ASSERT(NS_IsMainThread());
> -  if (CanSend()) {
> +  if (MessageLoop::current() != mCompositorThread) {
> +    // We have to send messages from the compositor thread
> +    mCompositorThread->PostTask(NewRunnableMethod<FrameMetrics>(this,
> +                                          &RemoteContentController::RequestContentRepaint,
> +                                          aFrameMetrics));
> +    return;
> +  }
> +
> +  if (mCanSend) {

As per comment above this should end up looking like:

MOZ_ASSERT(IsRepaintThread());
if (mCanSend) {
  Unused << SendRequestContentRepaint(aFrameMetrics);
}
Attachment #8780332 - Flags: review?(bugmail) → review-
Comment on attachment 8783424 [details]
Bug 1289650 - Simplify RemoteContentController destruction.

https://reviewboard.mozilla.org/r/73230/#review71048

LGTM, but again would prefer if dvander also looked at this.
Attachment #8783424 - Flags: review?(bugmail) → review+
Comment on attachment 8782168 [details]
Bug 1289650 - Convert APZChild into a wrapper around GeckoContentController.

https://reviewboard.mozilla.org/r/72372/#review71080

::: gfx/layers/ipc/APZChild.h:44
(Diff revision 4)
> +  bool RecvSetScrollingRootContent(const bool& isRootContent) override;
> +
> +  bool RecvNotifyMozMouseScrollEvent(const ViewID& aScrollId,
>                                       const nsString& aEvent) override;
>  
> -  bool RecvNotifyAPZStateChange(const ViewID& aViewId,
> +  bool RecvNotifyAPZStateChange(const ScrollableLayerGuid& aGuid,

This ViewID -> ScrollableLayerGuid conversion can be dropped (looks like it got shifted from a previous patch into this one, probably during your rebasing).

::: gfx/layers/ipc/APZChild.cpp:34
(Diff revision 4)
> +APZChild::SetController(RefPtr<GeckoContentController> aController)
> +{
> +  if (mController) {
> +    mController->Destroy();

Are there any cases where SetController is called while mController is already non-null?

::: gfx/layers/ipc/APZChild.cpp:129
(Diff revision 4)
> +}
> +
> +bool
> +APZChild::RecvDestroy()
> +{
> +  mDestroyed = true;

Doesn't look like mDestroyed is needed any more, can probably take it out.

::: gfx/layers/ipc/PAPZ.ipdl:60
(Diff revision 4)
> -  async NotifyMozMouseScrollEvent(uint64_t aLayersId, ViewID aScrollId, nsString aEvent);
> +  async UpdateOverscrollVelocity(float aX, float aY);
>  
> -  async NotifyAPZStateChange(ViewID aViewId, APZStateChange aChange, int aArg);
> +  async UpdateOverscrollOffset(float aX, float aY);
> +
> +  async SetScrollingRootContent(bool isRootContent);
> +

Note that Randall changed the signatures of these slightly in https://hg.mozilla.org/mozilla-central/rev/e96e6002ed72 which landed a few days ago. Just a heads up for when you rebase to master.
Attachment #8782168 - Flags: review?(bugmail) → review+
Comment on attachment 8783425 [details]
Bug 1289650 - Allow main process to create PAPZ, PAPZCTreeManager.

https://reviewboard.mozilla.org/r/73232/#review71092

LGTM.

::: gfx/layers/ipc/CompositorBridgeParent.cpp:660
(Diff revision 1)
>  CompositorBridgeParent::RecvInitialize(const uint64_t& aRootLayerTreeId)
>  {
>    mRootLayerTreeID = aRootLayerTreeId;
> +  mApzcTreeManager = new APZCTreeManager();

This function only gets called in the gpu process case, correct? Otherwise InitSameProcess() is called?

::: gfx/layers/ipc/CompositorBridgeParent.cpp:1354
(Diff revision 1)
>  CompositorBridgeParent::AllocPAPZCTreeManagerParent(const uint64_t& aLayersId)
>  {
> -  return nullptr;
> +  // The main process should only be using the mRootLayerTreeID.
> +  MOZ_ASSERT(aLayersId == mRootLayerTreeID);
> +
> +  return new APZCTreeManagerParent(mRootLayerTreeID, GetAPZCTreeManager());

For consistency and safety it might be better to also store this APZCTreeManagerParent into sIndirectLayerTrees[aLayersId].mApzcTreeManagerParent, the same way you do for the CrossProcessCompositorBridgeParent. It's not required since we don't really use it, but it would be nice to have the data structure populated consistently.
Attachment #8783425 - Flags: review?(bugmail) → review+
Attachment #8783425 - Attachment is obsolete: true
Comment on attachment 8780332 [details]
Bug 1289650 - Move PAPZ from PContent to PCompositorBridge.

https://reviewboard.mozilla.org/r/71058/#review71404

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2822
(Diff revision 7)
> -  // Reinvoke this method on the main thread if it's not there already. It's
> -  // important to do this before the call to CalculatePendingDisplayPort, so
> -  // that CalculatePendingDisplayPort uses the most recent available version of
> -  // mFrameMetrics, just before the paint request is dispatched to content.
> -  if (!NS_IsMainThread()) {
> +  RefPtr<GeckoContentController> controller = GetGeckoContentController();
> +  if (!controller) {
> +    return;
> +  }
> +  if (!controller->IsRepaintThread()) {

Leave this comment in the code, with s/main thread/repaint thread/

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3564
(Diff revision 7)
> +    if (!controller)
> +      return;

nit: braces
Attachment #8780332 - Flags: review?(bugmail) → review+
Comment on attachment 8782168 [details]
Bug 1289650 - Convert APZChild into a wrapper around GeckoContentController.

https://reviewboard.mozilla.org/r/72372/#review71408

::: gfx/layers/apz/util/ContentProcessController.cpp:194
(Diff revision 5)
> +ContentProcessController::DispatchToRepaintThread(already_AddRefed<Runnable> aTask)
> +{
> +  NS_DispatchToMainThread(Move(aTask));

This function should pretty much never get called, right? For the same reason that PostDelayedTask doesn't get called. It's fine to leave this implementation in, but just wondering if you saw it getting called at all.
Comment on attachment 8782168 [details]
Bug 1289650 - Convert APZChild into a wrapper around GeckoContentController.

https://reviewboard.mozilla.org/r/72372/#review71408

> This function should pretty much never get called, right? For the same reason that PostDelayedTask doesn't get called. It's fine to leave this implementation in, but just wondering if you saw it getting called at all.

I changed APZChild to check with the GeckoContentController it is wrapping to see if it is on the repaint thread, and if not to DispatchToRepaintThread. Mostly just to be 'complete' because in both the main process and content process APZChild should always be running on the repaint thread so it should never be dispatched. So yes that function shouldn't be used, but the IsRepaintThread() will be used.
Ok. It might be a little more robust to just assert that we're on the repaint thread, in APZChild. That way if something else changes upstream it won't silently start behaving differently.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #52)
> Ok. It might be a little more robust to just assert that we're on the
> repaint thread, in APZChild. That way if something else changes upstream it
> won't silently start behaving differently.

Yeah that makes more sense. Done.
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21cd933e4c32
Check the owning PID when creating PAPZCTreeManager, PLayerTransaction. r=kats,dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa5ccaa8c6c
Use PAPZCTreeManager in content process instead of PAPZ. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7162137f66
Move PAPZ from PContent to PCompositorBridge. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/beb32bdb4d1c
Simplify RemoteContentController destruction. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc6e2713327
Convert APZChild into a wrapper around GeckoContentController. r=kats
Keywords: checkin-needed
Resolved minor conflict with bug 1292781 and landed. ^
Keywords: leave-open
Comment on attachment 8780333 [details]
Bug 1289650 - Check the owning PID when creating PAPZCTreeManager, PLayerTransaction.

https://reviewboard.mozilla.org/r/71060/#review71784
Attachment #8780333 - Flags: review?(dvander) → review+
Keywords: leave-open
Summary: Get APZ working with GPU Process → Rearchitect APZ IPDL for moving to a compositor process
Ryan is going to test out a fix for this on try.

The problem is that the ChromeProcessController is destroyed off the main thread - this is ok, but it also holds the last reference to the mAPZEventState object, and so that also gets destroyed off the main thread, hence the assertion. We'll try to null out the mAPZEventState pointer in ChromeProcessController::Destroy on the main thread.
Flags: needinfo?(bugmail)
The try looks good. I'll reland once inbound is open again.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a883a5ff328
Check the owning PID when creating PAPZCTreeManager, PLayerTransaction. r=kats,dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/12b61fd41c99
Use PAPZCTreeManager in content process instead of PAPZ. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad7640633fb
Move PAPZ from PContent to PCompositorBridge. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/a98e4065514c
Simplify RemoteContentController destruction. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0d9956e1251
Convert APZChild into a wrapper around GeckoContentController. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/600e4f0568c4
Ensure the APZEventState object is destroyed on the main thread. r=kats
A number of intermittent test failures started spiking yesterday, probably due to these changes. At least some of them are probably bugs in the code so we should investigate and fix them. Bug 1298173, bug 1291151, bug 1297910, bug 1276077 so far.
The spike in bug 1291151 actually predates this landing, it might be a regression from bug 1286179 (or something else) instead. Given that all of 1291151, 1297910, and 1276077 are android-only and probably related, they are likely in the same boat (i.e. not caused by this bug).
You need to log in before you can comment on or make changes to this bug.