1074401 - Ensure all repaint requests dispatched from APZ code are on the same thread

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Currently we call RequestContentRepaint from both the compositor thread and the controller thread. Although the implementation of this method in RenderFrameParent bumps them all onto the gecko main thread, it's still prone to races and improper synchronization.

Comment 1

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

Attachment: Make GeckoCC::RequestContentRepaint always main-thread

After some thought I decided that since the implementation of RequestContentRepaint always needs to run on the main thread anyway (since it pokes at stuff in gecko), it made sense to ensure that GeckoCC::RequestContentRepaint calls always come in on the main thread. This simplifies all the GeckoCC implementations and also fixes bug 1074691 on master.

I think this approach is also future-compatible for when we change the controller thread on B2G to be something other than the main thread. The unthrottled paint request on touch-start will happen on the (new) input thread, and get queued on the main thread's event queue before the touch-start event itself gets queued on the main thread's event queue (which happens after the APZ returns the untransformed event). So the repaint should happen before the touch-start is hit-tested by Gecko and everything should just work.

I considered trying to move the NS_DispatchToMainThread part higher up in the call stack (e.g. before the paint throttling) but it's harder to do because the paint throttler's TaskComplete() notification always comes in on the compositor thread. I can do it, but in the interests of having a safer uplift patch I'd rather defer that to a follow-up bug.

Comment 2

[Botond Ballo [:botond]]

Comment on attachment 8498911 [details] [diff] [review]
Make GeckoCC::RequestContentRepaint always main-thread

Review of attachment 8498911 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, this does simplify things quite a bit!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2402,5 @@
> +    if (NS_IsMainThread()) {
> +      controller->RequestContentRepaint(aFrameMetrics);
> +    } else {
> +      NS_DispatchToMainThread(NS_NewRunnableMethodWithArg<FrameMetrics>(
> +        controller, &GeckoContentController::RequestContentRepaint, aFrameMetrics));

Do we not have a MessageLoop / Task* interface for this the way we do for the UI thread?

Comment 3

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

I think we'd need extra plumbing to get a handle to the main-thread message loop. MessageLoop::current() won't return the right one from non-main-thread.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dbe7877aaef

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0dbe7877aaef

Comment 6

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

Comment on attachment 8498911 [details] [diff] [review]
Make GeckoCC::RequestContentRepaint always main-thread

Approval Request Comment
[Feature/regressing bug #]: original APZ implementation
[User impact if declined]: things like bug 1074691 (which is nommed for blocking 2.0) can happen, where the user's finger is out of sync with what actually gets the touch events and clicks
[Describe test coverage new/current, TBPL]: code is covered by existing gtests, and no failures were triggered by this change. assertions in the code enforce the new behaviour.
[Risks and why]: low-medium risk. the patch is pretty big but conceptually it's pretty simple, and a most of the patch is just shifting code from one place to another. B2G and metro are affected; the changes to Fennec and APZ are in code that is disabled by default
[String/UUID change made/needed]: none

Comment 7

[bhavana bajaj [:bajaj]]

Comment on attachment 8498911 [details] [diff] [review]
Make GeckoCC::RequestContentRepaint always main-thread

Approving in 2.1 , keeping in mind the area of bugs filed in past release around this. It will be helpful if QA can help verify https://bugzilla.mozilla.org/show_bug.cgi?id=1074691 and talk to kats on other areas that can be tested here to mitigate the risk of fallouts.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b7d0647fee6

Comment 9

[Yeojin Chung [:YeojinC]]

Unable to verify at Q Analyst as the bug and the dependency involve back end issues.