Closed Bug 1433579 Opened 2 years ago Closed 2 years ago

Deadlock in APZ code with WR enabled

Categories

(Core :: Graphics: WebRender, enhancement, P1)

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

I think the root of the problem here is that the non-WR codepath calls AdvanceAnimations in a loop over the Layer(Metrics) tree, while the WR codepath calls AdvanceAnimations in a loop over the hit testing tree; the former does not involve holding the tree lock but the latter does.

Currently, AdvanceAnimations assumes the tree lock is not held when it's called, thus running into a problem with the WR codepath.

Since the Layer tree is going away in the long term, perhaps one approach to fixing this would be to have the non-WR codepath (AsyncCompositionManager) call a method like APZCTreeManager::AdvanceAnimations(), which loops over the hit testing tree (like the WR codepath) to call AsyncPanZoomController::AdvanceAnimations(). AsyncPanZoomController::AdvanceAnimations() can then assume the tree lock is held when it's called, and not try to acquire it itself.

Another approach would be to just make the tree lock recursive.
Assignee: nobody → bugmail
Priority: -- → P1
(In reply to Botond Ballo [:botond] from comment #1)
> Since the Layer tree is going away in the long term, perhaps one approach to
> fixing this would be to have the non-WR codepath (AsyncCompositionManager)
> call a method like APZCTreeManager::AdvanceAnimations(), which loops over
> the hit testing tree (like the WR codepath) to call
> AsyncPanZoomController::AdvanceAnimations().
> AsyncPanZoomController::AdvanceAnimations() can then assume the tree lock is
> held when it's called, and not try to acquire it itself.

I tried doing it this way, but BuildOverscrollHandoffChain() (which is where the nested lock acquisition happens, that we would want to remove) is called from a bunch of different places. So all of those places would then have to acquire the tree lock as well. I got halfway through implementing it and realized that it just makes the code more convoluted.

> Another approach would be to just make the tree lock recursive.

I'll try this
Comment on attachment 8947172 [details]
Bug 1433579 - Allow recursively holding the APZ tree lock.

https://reviewboard.mozilla.org/r/216946/#review222812
Attachment #8947172 - Flags: review?(botond) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd7ae44342ff
Allow recursively holding the APZ tree lock. r=botond
https://hg.mozilla.org/mozilla-central/rev/cd7ae44342ff
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
No longer blocks: 1434593
Duplicate of this bug: 1434593
You need to log in before you can comment on or make changes to this bug.