Closed
Bug 1280594
Opened 8 years ago
Closed 8 years ago
Fix up compositor pausing
Categories
(Firefox for Android Graveyard :: Toolbar, defect)
Firefox for Android Graveyard
Toolbar
Tracking
(firefox50 fixed)
RESOLVED
FIXED
Firefox 50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: snorp, Assigned: droeh)
Details
Attachments
(1 file, 1 obsolete file)
3.49 KB,
patch
|
droeh
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
This pauses directly on the compositor thread, and only locks in serverSurfaceDestroyed where necessary.
Attachment #8763229 -
Flags: review?(snorp)
Reporter | ||
Comment 3•8 years ago
|
||
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+
Reporter | ||
Comment 4•8 years ago
|
||
Actually it looks like mCompositorPaused is already handled by SyncPauseCompositor(), so you don't need the OnCompositorPaused() stuff
Assignee | ||
Comment 5•8 years ago
|
||
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
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3f1835bfbad7
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•