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

RESOLVED FIXED in mozilla35

Status

()

Core
Panning and Zooming
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: nl, Mentored)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

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.
Blocks: 988858
Whiteboard: [TCP]
Whiteboard: [TCP]
Mentor: bugmail.mozilla@staktrace.com
Whiteboard: [lang=c++]
(Assignee)

Updated

4 years ago
Assignee: nobody → nicklebedev37
(Assignee)

Comment 1

4 years ago
Created attachment 8486450 [details] [diff] [review]
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
Attachment #8486450 - Flags: review?(bugmail.mozilla)
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 ...;
}
...
Attachment #8486450 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Comment 4

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

Done. Btw, nice catch about Bug 1063224.
Attachment #8486450 - Attachment is obsolete: true
Attachment #8487122 - Flags: review?(bugmail.mozilla)
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
Attachment #8487122 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 8487292 [details] [diff] [review]
Add private method to access the treeManager which ensures that lock isn't held. v3.

Fixed the trailing space.
Attachment #8487122 - Attachment is obsolete: true
Attachment #8487292 - Flags: review+
(Assignee)

Comment 7

4 years ago
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
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Updated

4 years ago
Depends on: 1027772
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.
Flags: needinfo?(bugmail.mozilla)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07d4b55db7a3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.