1147681 - APZCs can be leaked when a compositor shuts down

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Comment 2

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

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.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, refactor RunOnControllerThread

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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, fix leak

Comment 5

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

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!

Comment 6

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

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

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c2179c027c28
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6f42f8ee8246

w/ comment #6 addressed

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2179c027c28
https://hg.mozilla.org/mozilla-central/rev/6f42f8ee8246