Closed Bug 1147681 Opened 5 years ago Closed 5 years ago

APZCs can be leaked when a compositor shuts down

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files, 1 obsolete file)

We don't clear the input queue during APZCTM::ClearTree(), so if there's a wheel transaction or something lying around, it'll leak memory via a cycle from mTargetApzc -> mInputQueue.
Attached patch fix (obsolete) — Splinter Review
Attachment #8583494 - Flags: review?(bugmail.mozilla)
Comment on attachment 8583494 [details] [diff] [review]
fix

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1061,5 @@
>  APZCTreeManager::ClearTree()
>  {
>    MonitorAutoLock lock(mTreeLock);
>  
> +  mInputQueue->Clear();

We should only be calling mInputQueue->Clear() on the controller thread, to avoid race conditions. I don't think APZCTM::ClearTree is guaranteed to be called on the controller thread on all platforms. Best to wrap it in a call to APZThreadUtils::RunOnControllerThread.

Note also that we shouldn't be holding the mTreeLock when calling mInputQueue->Clear(). Probably no harm in doing it, but it's not needed either.

::: gfx/layers/apz/src/InputQueue.cpp
@@ +462,5 @@
>  
> +void
> +InputQueue::Clear()
> +{
> +  mInputBlockQueue.Clear();

Add a APZThreadUtils::AssertOnControllerThread here.

::: gfx/layers/apz/src/InputQueue.h
@@ +101,5 @@
>     */
>    WheelBlockState* GetCurrentWheelTransaction() const;
> +  /**
> +   * Clear the input queue. APZCTreeManager invokes this to ensure no
> +   * references to APZCs are alive after ClearTree().

Just something like "Remove all input blocks from the input queue." is fine here. The second sentence of this comment should move to the call site.
Attachment #8583494 - Flags: review?(bugmail.mozilla) → review-
This requires widgets to explicitly set the MessageLoop of the controller thread, which makes asserting the right thread a little easier and lets us dispatch to the controller thread from any other thread.
Attachment #8583494 - Attachment is obsolete: true
Attachment #8584047 - Flags: review?(bugmail.mozilla)
Attached patch part 2, fix leakSplinter Review
Attachment #8584049 - Flags: review?(bugmail.mozilla)
Comment on attachment 8584047 [details] [diff] [review]
part 1, refactor RunOnControllerThread

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

I remember now that Fennec won't have a MessageLoop on its controller thread, but I think we can deal with with some #ifdef'ing local to APZThreadUtils.cpp so I'm not too worried about it. This looks good, thanks!
Attachment #8584047 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8584049 [details] [diff] [review]
part 2, fix leak

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

r+ with the RunnableMethodTraits struct removed, as it's not needed
Attachment #8584049 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/c2179c027c28
https://hg.mozilla.org/mozilla-central/rev/6f42f8ee8246
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.