Closed Bug 1280594 Opened 8 years ago Closed 8 years ago

Fix up compositor pausing

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

defect
Not set
normal

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: snorp, Assigned: droeh)

Details

Attachments

(1 file, 1 obsolete file)

It looks to me like the way we pause the compositor is incorrect, and also prone to deadlocks (see ANR reports here: https://telemetry.mozilla.org/hang/anr/). I think we should pause the compositor directly, going from the UI thread to the Compositor thread, but it looks like we bounce it off the Gecko thread right now. This makes deadlocks more likely, and AFAICT there is no need to go through Gecko.
Some things I noted when looking:

* I don't think GLController.serverSurfaceDestroyed() needs to be synchronized -- just the part where we set mServerSurfaceValid
* We call pauseCompositor() there, which ends up in nsWindow::PauseCompositor(), which is dispatched to the main thread synchronously via the JNI stuff. The only thing nsWindow needs to change when pausing the compositor is mCompositorPaused, but we can do that similar to how we do resume. Basically I think we should be calling nsWindow::SyncPauseCompositor() instead.
Attached patch Proposed patch (obsolete) — Splinter Review
This pauses directly on the compositor thread, and only locks in serverSurfaceDestroyed where necessary.
Attachment #8763229 - Flags: review?(snorp)
Comment on attachment 8763229 [details] [diff] [review]
Proposed patch

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

Looks good with nits

::: widget/android/nsWindow.cpp
@@ +915,5 @@
>                      mozilla::Move(aCall)), &GLControllerEvent::MakeEvent);
>              return;
>  
> +        } else if (aCall.IsTarget(&GLControllerSupport::PauseCompositor))
> +        {

Same line as the 'else if'
Attachment #8763229 - Flags: review?(snorp) → review+
Actually it looks like mCompositorPaused is already handled by SyncPauseCompositor(), so you don't need the OnCompositorPaused() stuff
Now with nits fixed, carrying over snorp's r+.
Attachment #8763229 - Attachment is obsolete: true
Attachment #8763244 - Flags: review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1835bfbad7
Pause the compositor directly instead of bouncing off the Gecko thread. r=snorp
https://hg.mozilla.org/mozilla-central/rev/3f1835bfbad7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: