Possible deadlock in APZC

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

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

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

I was writing a test for some code in APZC and I ran into a case where gtest was warning of a possible deadlock. In general I think we code where APZCTreeManager holds the mTreeLock and then calls into AsyncPanZoomController, which picks up mMonitor. Bug 940706 added the reverse, where in functions like OnSingleTapUp, we hold mMonitor and then call ConvertToGecko, which calls APZCTreeManager::GetInputTransforms, which picks up mTreeLock. This is bad as it can potentially result in deadlock depending on the threading scheme being used.
Posted patch Remove unnecessary locking (obsolete) — Splinter Review
I don't believe this locking is needed any more
Attachment #8344646 - Flags: review?(botond)
Attachment #8344646 - Flags: review?(bgirard)
Comment on attachment 8344646 [details] [diff] [review]
Remove unnecessary locking

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

Does AsyncPanZoomController::ConvertToGecko http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#789 need a lock then? We should avoid holding the lock when calling into the controller.
Actually that's a good point. I thought ConvertToGecko didn't need a lock but it does access mFrameMetrics in one place which will need a lock. I'll update the patch.
Attachment #8344646 - Attachment is obsolete: true
Attachment #8344646 - Flags: review?(botond)
Attachment #8344646 - Flags: review?(bgirard)
Attachment #8344733 - Flags: review?(botond)
Attachment #8344733 - Flags: review?(bgirard)
blocking-b2g: --- → 1.3+
I pushed this to try along with my patches and is looking green: https://tbpl.mozilla.org/?tree=Try&rev=e0905361a062
Comment on attachment 8344733 [details] [diff] [review]
Remove unnecessary locking

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

The patch looks good.

I wonder if we could be a little more proactive in avoiding the potential deadlock here. For example, can we add a private GetTreeManager() method to APZC which asserts that the APZC does not hold the mMonitor (since methods called on the TM will likely grab the tree lock), and enforce that an APZC only accesses the tree manager through that function?
Attachment #8344733 - Flags: review?(botond) → review+
Enforcing that the tree manager is only accessed through that function is the hard bit. We have the same problem with mFrameMetrics - we have a GetFrameMetrics() function that checks if the monitor is held but we're still using mFrameMetrics directly all over the place. Anthony Jones' work in bug 897017 might be helpful here but it seems to have stalled for now.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> Enforcing that the tree manager is only accessed through that function is
> the hard bit. We have the same problem with mFrameMetrics - we have a
> GetFrameMetrics() function that checks if the monitor is held but we're
> still using mFrameMetrics directly all over the place. Anthony Jones' work
> in bug 897017 might be helpful here but it seems to have stalled for now.

I think there is value in introducing the function and adding a comment saying that you should call it instead of accessing mTreeManager, even if we don't enforce it yet.
Attachment #8344733 - Flags: review?(bgirard) → review+
(In reply to Botond Ballo [:botond] from comment #8)
> I think there is value in introducing the function and adding a comment
> saying that you should call it instead of accessing mTreeManager, even if we
> don't enforce it yet.

There is some value in that, yes. I've filed bug 949008 for it though since I didn't want to do it here.
https://hg.mozilla.org/mozilla-central/rev/68c4271ae6ce
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.