1445662 - Tighten up threading assertions and behaviour for IAPZCTreeManager interface

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement)

Description

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

As a precursor to bug 1441324 I went through the functions on IAPZCTreeManager and sorted them into things that run on the controller thread vs things that run on the sampler thread. There's a few codepaths involving this interface:

1. The root widget (nsBaseWidget) calls some methods on this interface via the mAPZC pointer that it keeps. The nsBaseWidget methods themselves run on the UI process main thread, but sometimes do RunOnControllerThread wrappers around the mAPZC function calls. We can further subcategorize these code paths:

1a. If there's a GPU process, the UI process main thread is marked as the "controller thread" in the UI process, and the compositor thread is marked as the "controller thread" in the GPU process, and the PAPZCTreeManager bridge connects the two. So direct mAPZC->whatever and RunOnControllerThread({ mAPZC->whatever }) calls are effectively equivalent in that they send a message from the UI process main thread to the GPU process compositor thread. In some of the APZCTreeManagerParent function implementations there's a RunOnControllerThread wrapper, but because the controller thread is the compositor thread in the GPU process, the wrappers are no-ops for this code path, and the concrete APZCTreeManager is entered on the same thread.

1b. If we're on Android, the java UI thread is the "controller thread", and this is different from the UI process "main thread" on which nsBaseWidget functions are invoked. So direct mAPZC->whatever calls run on the main thread, and RunOnControllerThread({ mAPZC->whatever }) calls are bounced to the java UI thread. In both cases there is no PAPZCTreeManager bridge, and mAPZC is the actual concrete APZCTreeManager. So already we see that the direct mAPZC->whatever calls are getting run on the UI process main thread, which might be unexpected. Calls to SetDPI and SetKeyboardMap fall into this category.

1c. If neither of the above cases are true, then we're on a desktop platform with no GPU process. In this case the UI process main thread is the APZ controller thread, and mAPZC is the concrete APZCTreeManager instance. So this is similar to case 1a in that both the direct and RunOnControllerThread variations are effectively equivalent and run the function on the controller thread.

2. The content process (TabChild) calls methods on this interface via the mApzcTreeManager pointer that it keeps. In all cases this goes over the PAPZCTreeManager bridge, so it goes from the content process main thread to the GPU (or UI) process compositor thread. On the compositor thread, the APZCTreeManagerParent function implementations sometimes do a RunOnControllerThread, which will bounce the message to the UI process main thread. So some functions (the ones without RunOnControllerThread) enter the concrete APZCTreeManager on the compositor thread and others (the ones with RunOnControllerThread) enter on the controller thread.


The main problem I'm having now is how to deal with calls to UpdateZoomConstraints and ZoomToRect. Conceptually I believe these should run on the sampler thread because they take ScrollableLayerGuids which can refer to scroll ids that APZ only learns about via a layers update (on the sampler thread). So conceptually the calls to UpdateZoomConstraints and ZoomToRect should always follow the layers update, and that means running them on the sampler thread. However, in practice they can currently be invoked on the controller thread (in scenario 1c) or the UI process main thread (in scenario 1b) because they get called directly from nsBaseWidget without a RunOnControllerThread wrapper. The thing is, these functions are really only relevant on Android, and their implementations are such that I don't think any of the conceptual errors would ever manifest. i.e. UpdateZoomConstraints already handles scrollids that it doesn't know about and applies them on a subsequent layers update, and ZoomToRect is only ever called with the root document's scrollId which will almost certainly already be layerized. So in practice they aren't problems, but I would still like to ensure they get assigned to run on a particular thread and always run there. I particularly want to ensure that in case 1b they don't just run on the UI process main thread, which from APZ's point of view is neither the controller thread nor the sampler thread, but just "some random thread".

Comment 1

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

With respect to UpdateZoomConstraints, I think running it on the sampler thread makes the most sense. It seems conceptually similar to a layers update and a focus state update, both of which run on the sampler thread. It also means we can lift the restriction that APZCTreeManager::mZoomConstraints only be accessed while holding the tree lock, because all accesses will be on the sampler thread.

ZoomToRect might make sense on the controller thread. In a future world where we have subframe zooming we might want to zoom to a rect in an unlayerized subframe, which would require using some sort of layers-dependent codepath, but much like SetConfirmedTargetApzc can we add a layers-dependent version of that on PLayerTransaction/PWebRenderBridge.

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=647f7bb1e0c8a9186b8bfe47611cc2706f25182e

The try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0df6bcb5fb9a2fe7edf4eed1d78aea2d2fa89ac7 also has the patches for this bug, plus the stuff I wrote for bug 1441324.

I did a bunch of local testing to exercise the various codepaths, and everything seems good. I just missed the ZoomToRect call in ChromeProcessController so exercising that codepath triggered an assertion on Android. I fixed that up locally, and the patchset I post will have that fix.

Comment 3

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

Attachment: Bug 1445662 - Ensure the keyboard map access is threadsafe.

- The change in APZCTreeManagerParent is functionally a no-op because it
  only ever runs in the GPU process on the controller thread. But it
  allows moving the controller thread to some other thread.
- The change in nsBaseWidget is a no-op for desktop platforms, because
  in the UI process the main thread is the controller thread. But on
  Android it moves the call from the main thread to the Java UI thread.

Review commit: https://reviewboard.mozilla.org/r/227822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227822/

Comment 4

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

Attachment: Bug 1445662 - Make the DPI non-static and bound to the controller thread.

Since we can have multiple browser windows on multiple different
displays with different DPIs, it doesn't make sense to have a single
static DPI value shared across all APZCTreeManagers. Instead, each
APZCTM should store its own DPI value for the display the window is on.
Since the DPI is only ever read from the controller thread, we can make
it bound to that thread, and update the setter code to also set it on
that thread.

As with the previous patch, the change in APZCTreeManagerParent is a
no-op but allows making some other thread in the GPU process the controller
thread. And the change in nsBaseWidget is a no-op everywhere except
Android.

Review commit: https://reviewboard.mozilla.org/r/227824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227824/

Comment 5

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

Attachment: Bug 1445662 - Remove ProcessTouchVelocity from PAPZCTreeManager.ipdl.

This function is never actually called over IPDL. It is called directly
on the concrete APZCTreeManager instance by the
AndroidDynamicToolbarAnimator code, both of which live in the
compositor. So we don't need to expose this method on IAPZCTreeManager
or over PAPZCTreeManager.

Review commit: https://reviewboard.mozilla.org/r/227826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227826/

Comment 6

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

Attachment: Bug 1445662 - Assert that IAPZCTreeManager's helper methods are always on the controller thread.

These two functions (UpdateWheelTransaction and ProcessUnhandledEvent)
are only ever called on the concrete APZCTreeManager when the APZ code
is living in the GPU process. This is because the calls are made by the
IAPZCTreeManager implementation which lives in the UI process, and
remoted over PAPZCTreeManager. So the assertion is safe, and will help
us guard against inadvertent breakage when we try making a different
thread the controller thread in the GPU process.

In addition, the WillHandleInput function can be called in the GPU
process on the compositor thread, but we will allow it to be called on
the main thread as well. In that case we need to ensure we don't try
running EventStateManager pref-reading code in the GPU process, and
instead preserve the current behaviour of just returning true.

Review commit: https://reviewboard.mozilla.org/r/227828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227828/

Comment 7

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

Attachment: Bug 1445662 - Ensure UpdateZoomConstraints runs on the sampler thread.

Without this patch, UpdateZoomConstraints can get called on:
a) the compositor/sampler thread (over PAPZCTreeManager)
b) the controller thread which is also the UI process main thread (on
   desktop platforms without a GPU process)
c) the UI process main thread when it's *not* the controller thread (on
   Android).
Instead of having to reason about all these scenarios separately, we can
try to unify them a little bit by ensuring the function contents always
run on the sampler thread, which is the thread that seems to make the
most sense for it.

Review commit: https://reviewboard.mozilla.org/r/227830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227830/

Comment 8

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

Attachment: Bug 1445662 - Ensure ZoomToRect runs on the controller thread.

Currently the ZoomToRect function is only ever called on Android, on the
UI process main thread, which is neither the controller nor the sampler
thread. Instead of allowing "random" threads to run inside APZ, we
ensure that callers run it on the controller thread.

Review commit: https://reviewboard.mozilla.org/r/227832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227832/

Comment 9

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

Attachment: Bug 1445662 - Annotate remaining PAPZCTreeManager-invoked methods with threading constraints.

These methods are already guaranteed to be called on the controller
thread.

Review commit: https://reviewboard.mozilla.org/r/227834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227834/

Comment 10

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

Attachment: Bug 1445662 - Update RemoteContentController to allow the GPU process controller thread to be different from the compositor thread.

A couple of RemoteContentController methods get called on the controller
thread in the GPU process. This is the same as the compositor thread so
we could just do compositor-thread stuff here, but we will want to
support the controller thread being the main thread instead of the
compositor thread. So we detect those cases and bounce the message
accordingly.

Review commit: https://reviewboard.mozilla.org/r/227836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227836/

Comment 11

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

@rhunt: Botond is away this week and you're familiar with much of this code so I flagged you for review, but feel free to bounce anything to botond (or ask for clarifications).

Comment 12

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8958970 [details]
Bug 1445662 - Ensure the keyboard map access is threadsafe.

https://reviewboard.mozilla.org/r/227822/#review233944

::: widget/nsBaseWidget.cpp:945
(Diff revision 1)
>  
>    mAPZC->SetDPI(GetDPI());
>  
>    if (gfxPrefs::APZKeyboardEnabled()) {
> -    mAPZC->SetKeyboardMap(nsXBLWindowKeyHandler::CollectKeyboardShortcuts());
> +    KeyboardMap map = nsXBLWindowKeyHandler::CollectKeyboardShortcuts();
> +    // On Android the main thread is not the controller thread

Comment and change is good, but as a side note 'apz.keyboard.enabled' is false on android so this code wasn't running before.

Comment 13

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8958971 [details]
Bug 1445662 - Make the DPI non-static and bound to the controller thread.

https://reviewboard.mozilla.org/r/227824/#review233936

This is a good change, I always wondered why this was static. Is this going to be a user visible change?

Comment 14

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8958972 [details]
Bug 1445662 - Remove ProcessTouchVelocity from PAPZCTreeManager.ipdl.

https://reviewboard.mozilla.org/r/227826/#review233946

Comment 15

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8958973 [details]
Bug 1445662 - Assert that IAPZCTreeManager's helper methods are always on the controller thread.

https://reviewboard.mozilla.org/r/227828/#review233948

Comment 16

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8958974 [details]
Bug 1445662 - Ensure UpdateZoomConstraints runs on the sampler thread.

https://reviewboard.mozilla.org/r/227830/#review233950

Comment 17

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8958975 [details]
Bug 1445662 - Ensure ZoomToRect runs on the controller thread.

https://reviewboard.mozilla.org/r/227832/#review233952

Comment 18

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8958976 [details]
Bug 1445662 - Annotate remaining PAPZCTreeManager-invoked methods with threading constraints.

https://reviewboard.mozilla.org/r/227834/#review233954

Comment 19

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8958977 [details]
Bug 1445662 - Update RemoteContentController to allow the GPU process controller thread to be different from the compositor thread.

https://reviewboard.mozilla.org/r/227836/#review233956

Comment 20

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

(In reply to Ryan Hunt [:rhunt] from comment #12)
> > +    KeyboardMap map = nsXBLWindowKeyHandler::CollectKeyboardShortcuts();
> > +    // On Android the main thread is not the controller thread
> 
> Comment and change is good, but as a side note 'apz.keyboard.enabled' is
> false on android so this code wasn't running before.

Ah, good point, I forgot about that. So it's still not running but if we decide to turn on that pref on Android things shouldn't break.

(In reply to Ryan Hunt [:rhunt] from comment #13)
> 
> This is a good change, I always wondered why this was static. Is this going
> to be a user visible change?

In theory it might have user-visible effects, yes. Right now if the user opens two windows on separate displays, the DPI from the second one will start getting used for the first one as well. With this change, that will stop happening. That being said, the DPI is mostly used for scaling distances in touch event handling so any user-visible change would be restricted to users who (a) have multiple displays with different DPIs, (b) one of those display is a touchscreen and (c) a window is opened on the touchscreen and then later another window is opened on another display. So probably pretty rare.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/121cd3a0490f
Ensure the keyboard map access is threadsafe. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/41d8b7a6b339
Make the DPI non-static and bound to the controller thread. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/b5f2ceda75bd
Remove ProcessTouchVelocity from PAPZCTreeManager.ipdl. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/8fadda7d555e
Assert that IAPZCTreeManager's helper methods are always on the controller thread. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/ca1e29c9b439
Ensure UpdateZoomConstraints runs on the sampler thread. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/20c79dee1905
Ensure ZoomToRect runs on the controller thread. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/13f4f51d7bd1
Annotate remaining PAPZCTreeManager-invoked methods with threading constraints. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/d514b05d1f6a
Update RemoteContentController to allow the GPU process controller thread to be different from the compositor thread. r=rhunt

Comment 22

[Cristina Coroiu [:ccoroiu]]

Backed out 8 changesets (bug 1445662) for bustage at build/src/gfx/layers/apz/util/ChromeProcessController.cp on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d514b05d1f6a60aae21ba4a22c6b7af43fc06d76

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168277618&repo=autoland&lineNumber=18523

Backout: https://hg.mozilla.org/integration/autoland/rev/72a41cf593c7416619b3da744603c65108bf173f

Comment 23

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

I missed an #include in the change I made after the last try push, and it worked fine on the platforms I compiled locally due to unified compilation :/

Testing the fix locally before I repush the patches.

Comment 24

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

Attachment: Bug 1445662 - Ensure the keyboard map access is threadsafe.

- The change in APZCTreeManagerParent is functionally a no-op because it
  only ever runs in the GPU process on the controller thread. But it
  allows moving the controller thread to some other thread.
- The change in nsBaseWidget is a no-op for desktop platforms, because
  in the UI process the main thread is the controller thread. But on
  Android it moves the call from the main thread to the Java UI thread.

Review commit: https://reviewboard.mozilla.org/r/228126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228126/

Comment 25

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

Attachment: Bug 1445662 - Make the DPI non-static and bound to the controller thread.

Since we can have multiple browser windows on multiple different
displays with different DPIs, it doesn't make sense to have a single
static DPI value shared across all APZCTreeManagers. Instead, each
APZCTM should store its own DPI value for the display the window is on.
Since the DPI is only ever read from the controller thread, we can make
it bound to that thread, and update the setter code to also set it on
that thread.

As with the previous patch, the change in APZCTreeManagerParent is a
no-op but allows making some other thread in the GPU process the controller
thread. And the change in nsBaseWidget is a no-op everywhere except
Android.

Review commit: https://reviewboard.mozilla.org/r/228128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228128/

Comment 26

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

Attachment: Bug 1445662 - Remove ProcessTouchVelocity from PAPZCTreeManager.ipdl.

This function is never actually called over IPDL. It is called directly
on the concrete APZCTreeManager instance by the
AndroidDynamicToolbarAnimator code, both of which live in the
compositor. So we don't need to expose this method on IAPZCTreeManager
or over PAPZCTreeManager.

Review commit: https://reviewboard.mozilla.org/r/228130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228130/

Comment 27

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

Attachment: Bug 1445662 - Assert that IAPZCTreeManager's helper methods are always on the controller thread.

These two functions (UpdateWheelTransaction and ProcessUnhandledEvent)
are only ever called on the concrete APZCTreeManager when the APZ code
is living in the GPU process. This is because the calls are made by the
IAPZCTreeManager implementation which lives in the UI process, and
remoted over PAPZCTreeManager. So the assertion is safe, and will help
us guard against inadvertent breakage when we try making a different
thread the controller thread in the GPU process.

In addition, the WillHandleInput function can be called in the GPU
process on the compositor thread, but we will allow it to be called on
the main thread as well. In that case we need to ensure we don't try
running EventStateManager pref-reading code in the GPU process, and
instead preserve the current behaviour of just returning true.

Review commit: https://reviewboard.mozilla.org/r/228132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228132/

Comment 28

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

Attachment: Bug 1445662 - Ensure UpdateZoomConstraints runs on the sampler thread.

Without this patch, UpdateZoomConstraints can get called on:
a) the compositor/sampler thread (over PAPZCTreeManager)
b) the controller thread which is also the UI process main thread (on
   desktop platforms without a GPU process)
c) the UI process main thread when it's *not* the controller thread (on
   Android).
Instead of having to reason about all these scenarios separately, we can
try to unify them a little bit by ensuring the function contents always
run on the sampler thread, which is the thread that seems to make the
most sense for it.

Review commit: https://reviewboard.mozilla.org/r/228134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228134/

Comment 29

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

Attachment: Bug 1445662 - Ensure ZoomToRect runs on the controller thread.

Currently the ZoomToRect function is only ever called on Android, on the
UI process main thread, which is neither the controller nor the sampler
thread. Instead of allowing "random" threads to run inside APZ, we
ensure that callers run it on the controller thread.

Review commit: https://reviewboard.mozilla.org/r/228136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228136/

Comment 30

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

Attachment: Bug 1445662 - Annotate remaining PAPZCTreeManager-invoked methods with threading constraints.

These methods are already guaranteed to be called on the controller
thread.

Review commit: https://reviewboard.mozilla.org/r/228138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228138/

Comment 31

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

Attachment: Bug 1445662 - Update RemoteContentController to allow the GPU process controller thread to be different from the compositor thread.

A couple of RemoteContentController methods get called on the controller
thread in the GPU process. This is the same as the compositor thread so
we could just do compositor-thread stuff here, but we will want to
support the controller thread being the main thread instead of the
compositor thread. So we detect those cases and bounce the message
accordingly.

Review commit: https://reviewboard.mozilla.org/r/228140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228140/

Comment 32

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

Uhh I dont' know why MozReview decided these were all new patches. Sorry!

Comment 33

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

Comment on attachment 8959270 [details]
Bug 1445662 - Ensure the keyboard map access is threadsafe.

I'll push to inbound. Carrying r=rhunt from before.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/988b67ba9014
Ensure the keyboard map access is threadsafe. r=rhunt
https://hg.mozilla.org/integration/mozilla-inbound/rev/c077aee87a2e
Make the DPI non-static and bound to the controller thread. r=rhunt
https://hg.mozilla.org/integration/mozilla-inbound/rev/a27ceb86fbd0
Remove ProcessTouchVelocity from PAPZCTreeManager.ipdl. r=rhunt
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6393fca03cb
Assert that IAPZCTreeManager's helper methods are always on the controller thread. r=rhunt
https://hg.mozilla.org/integration/mozilla-inbound/rev/09df6079857e
Ensure UpdateZoomConstraints runs on the sampler thread. r=rhunt
https://hg.mozilla.org/integration/mozilla-inbound/rev/2900766520b2
Ensure ZoomToRect runs on the controller thread. r=rhunt
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e87464ac860
Annotate remaining PAPZCTreeManager-invoked methods with threading constraints. r=rhunt
https://hg.mozilla.org/integration/mozilla-inbound/rev/26936557cab6
Update RemoteContentController to allow the GPU process controller thread to be different from the compositor thread. r=rhunt

Comment 35

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/988b67ba9014
https://hg.mozilla.org/mozilla-central/rev/c077aee87a2e
https://hg.mozilla.org/mozilla-central/rev/a27ceb86fbd0
https://hg.mozilla.org/mozilla-central/rev/e6393fca03cb
https://hg.mozilla.org/mozilla-central/rev/09df6079857e
https://hg.mozilla.org/mozilla-central/rev/2900766520b2
https://hg.mozilla.org/mozilla-central/rev/2e87464ac860
https://hg.mozilla.org/mozilla-central/rev/26936557cab6