Closed
Bug 949008
Opened 11 years ago
Closed 10 years ago
Add a GetTreeManager() private function in APZC that enforces the lock is held
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: kats, Assigned: nl, Mentored)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [lang=c++])
Attachments
(1 file, 2 obsolete files)
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.
Reporter | ||
Updated•11 years ago
|
Blocks: apz-threadsafe
Updated•11 years ago
|
Whiteboard: [TCP]
Updated•10 years ago
|
Whiteboard: [TCP]
Reporter | ||
Updated•10 years ago
|
Mentor: bugmail.mozilla
Reporter | ||
Updated•10 years ago
|
Whiteboard: [lang=c++]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nicklebedev37
Assignee | ||
Comment 1•10 years ago
|
||
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)
Reporter | ||
Comment 2•10 years ago
|
||
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-
Reporter | ||
Comment 3•10 years ago
|
||
(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•10 years ago
|
||
Done. Btw, nice catch about Bug 1063224.
Attachment #8486450 -
Attachment is obsolete: true
Attachment #8487122 -
Flags: review?(bugmail.mozilla)
Reporter | ||
Comment 5•10 years ago
|
||
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•10 years ago
|
||
Fixed the trailing space.
Attachment #8487122 -
Attachment is obsolete: true
Attachment #8487292 -
Flags: review+
Assignee | ||
Comment 7•10 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)
Reporter | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Keywords: checkin-needed
Comment 10•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•