947931 - Possible deadlock in APZC

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Remove unnecessary locking

I don't believe this locking is needed any more

Comment 2

[Benoit Girard (:BenWa)]

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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Remove unnecessary locking

Comment 5

[Dale Harvey (:daleharvey)]

I pushed this to try along with my patches and is looking green: https://tbpl.mozilla.org/?tree=Try&rev=e0905361a062

Comment 6

[Botond Ballo [:botond]]

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?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 8

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/b2g-inbound/rev/68c4271ae6ce

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68c4271ae6ce

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/08035fdd06d3