APZCs can be leaked when a compositor shuts down

RESOLVED FIXED in Firefox 39



5 years ago
4 years ago


(Reporter: dvander, Assigned: dvander)


Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)



(2 attachments, 1 obsolete attachment)

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.
Posted patch fix (obsolete) — Splinter Review
Attachment #8583494 - Flags: review?(bugmail.mozilla)
Comment on attachment 8583494 [details] [diff] [review]

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)
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+
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.