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

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Blocks 1 bug, {verifyme})

unspecified
mozilla35
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.0 unaffected, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment)

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.

Updated

5 years ago
Blocks: 1074691
No longer blocks: 1061142
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.
Assignee: nobody → bugmail.mozilla
Attachment #8498911 - Flags: review?(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?
Attachment #8498911 - Flags: review?(botond) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/0dbe7877aaef
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Attachment #8498911 - Flags: approval-mozilla-aurora?
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.
Attachment #8498911 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Depends on: 1084323
Unable to verify at Q Analyst as the bug and the dependency involve back end issues.
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.