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".

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.

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.

@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 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 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 on attachment 8958972 [details] Bug 1445662 - Remove ProcessTouchVelocity from PAPZCTreeManager.ipdl. https://reviewboard.mozilla.org/r/227826/#review233946

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 on attachment 8958974 [details] Bug 1445662 - Ensure UpdateZoomConstraints runs on the sampler thread. https://reviewboard.mozilla.org/r/227830/#review233950

Comment on attachment 8958975 [details] Bug 1445662 - Ensure ZoomToRect runs on the controller thread. https://reviewboard.mozilla.org/r/227832/#review233952

Comment on attachment 8958976 [details] Bug 1445662 - Annotate remaining PAPZCTreeManager-invoked methods with threading constraints. https://reviewboard.mozilla.org/r/227834/#review233954

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

(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.

Pushed by kgupta@mozilla.com: 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

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

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.

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

Comment on attachment 8959270 [details] Bug 1445662 - Ensure the keyboard map access is threadsafe. I'll push to inbound. Carrying r=rhunt from before.

Pushed by kgupta@mozilla.com: 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

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