Tighten up threading assertions and behaviour for IAPZCTreeManager interface

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(8 attachments, 8 obsolete attachments)

59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
@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

Last year
mozreview-review
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.
Attachment #8958970 - Flags: review?(rhunt) → review+

Comment 13

Last year
mozreview-review
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?
Attachment #8958971 - Flags: review?(rhunt) → review+

Comment 14

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

https://reviewboard.mozilla.org/r/227826/#review233946
Attachment #8958972 - Flags: review?(rhunt) → review+

Comment 15

Last year
mozreview-review
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
Attachment #8958973 - Flags: review?(rhunt) → review+

Comment 16

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

https://reviewboard.mozilla.org/r/227830/#review233950
Attachment #8958974 - Flags: review?(rhunt) → review+

Comment 17

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

https://reviewboard.mozilla.org/r/227832/#review233952
Attachment #8958975 - Flags: review?(rhunt) → review+

Comment 18

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

https://reviewboard.mozilla.org/r/227834/#review233954
Attachment #8958976 - Flags: review?(rhunt) → review+

Comment 19

Last year
mozreview-review
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
Attachment #8958977 - Flags: review?(rhunt) → review+
(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

Last year
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
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.
Flags: needinfo?(bugmail)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8958970 - Attachment is obsolete: true
Attachment #8958971 - Attachment is obsolete: true
Attachment #8958972 - Attachment is obsolete: true
Attachment #8958973 - Attachment is obsolete: true
Attachment #8958974 - Attachment is obsolete: true
Attachment #8958975 - Attachment is obsolete: true
Attachment #8958976 - Attachment is obsolete: true
Attachment #8958977 - Attachment is obsolete: true
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.
Attachment #8959270 - Flags: review?(rhunt) → review+
Attachment #8959271 - Flags: review?(rhunt) → review+
Attachment #8959272 - Flags: review?(rhunt) → review+
Attachment #8959273 - Flags: review?(rhunt) → review+
Attachment #8959274 - Flags: review?(rhunt) → review+
Attachment #8959275 - Flags: review?(rhunt) → review+
Attachment #8959276 - Flags: review?(rhunt) → review+
Attachment #8959277 - Flags: review?(rhunt) → review+

Comment 34

Last year
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
Depends on: 1454430
You need to log in before you can comment on or make changes to this bug.