949008 - Add a GetTreeManager() private function in APZC that enforces the lock is held

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=947931#c6

We should probably also update things to use GetFrameMetrics() rather than using mFrameMetrics directly.

Comment 1

[Nick Lebedev [:nl]]

Attachment: Add private method to access the treeManager which ensures that lock isn't held

This patch should fix possible deadlocks which may happen when:
apzc locks an apzc's instance lock and then calls some method of the apzctm which in its turn locks its own tree lock and starts operating with the apzc instances which try to lock their instance locks (as first one).

So in this situation we might get the following deadlock:
apzc lock -> apzctm lock -> apzc lock (deadlock)

And to prevent such issues i'm adding an assert that will warn about it.

also basic try results: https://tbpl.mozilla.org/?tree=Try&rev=7194cfd0fe4b

Comment 2

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

Comment on attachment 8486450 [details] [diff] [review]
Add private method to access the treeManager which ensures that lock isn't held

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

This looks good. Bug 1063224 added two more functions which use mApzcTreeManager, so you'll want to pull the latest inbound code and update those two functions as well. r- since if this lands as-is on inbound it will miss these two new functions.

Also, when generating patches please generate with 8 lines of diff context instead of only 3 lines, which makes it easier to review the patch. you can do this by passing -U8 to the |hg diff| command or by adding this to your ~/.hgrc file:

[diff]
unified = 8

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1455,1 @@
>    if (treeManagerLocal) {

Since you're touching this code anyway, can you combine these two lines to be:

if (APZCTreeManager* treeManagerLocal = GetApzcTreeManager()) {
  ...

@@ +1927,2 @@
>      return treeManagerLocal->BuildOverscrollHandoffChain(this);
>    } else {

Again, since you're touching this code anyway, can you remove the else-after-return style violation here? Change it to look like this:

if (...) {
  return ...;
}
...

Comment 3

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> This looks good. Bug 1063224 added two more functions which use
> mApzcTreeManager, so you'll want to pull the latest inbound code and update
> those two functions as well. r- since if this lands as-is on inbound it will
> miss these two new functions.

Oh, bug 1063224 got merged to m-c already so you can pull the latest m-c instead of inbound code.

Comment 4

[Nick Lebedev [:nl]]

Attachment: Add private method to access the treeManager which ensures that lock isn't held. v2.

Done. Btw, nice catch about Bug 1063224.

Comment 5

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

Comment on attachment 8487122 [details] [diff] [review]
Add private method to access the treeManager which ensures that lock isn't held. v2.

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

Looks good, thanks!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1967,2 @@
>    }
> +  

nit: remove trailing whitespace

Comment 6

[Nick Lebedev [:nl]]

Attachment: Add private method to access the treeManager which ensures that lock isn't held. v3.

Fixed the trailing space.

Comment 7

[Nick Lebedev [:nl]]

I've found an interesting thing:
method AssertNotCurrentThreadIn isn't implemented yet ([1]) but there plans to add implementation of it soon. See last comments of bug 1027818 [2]. Given this I think we either can land the patch since this method is already referenced in media classes [3] or leave the bug as is and wait for dependency. I think that second is preferable. Please let me know if you agree.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/ReentrantMonitor.h#143

[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1027818

[3] http://mxr.mozilla.org/mozilla-central/ident?i=AssertNotCurrentThreadIn

Comment 8

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

Hm, that is interesting. I'd rather land this anyway since even if it does nothing it serves as useful documentation to readers of the code.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07d4b55db7a3

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/07d4b55db7a3