1015331 - Task object leaked during fling handoff

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Bug 965871, which implemented overscroll handoff for flings, introduced a bug where the Task object queued here [1] and run here [2] is never freed (Task objects are freed by the event loop if they are posted to one, but otherwise have to be freed manually).

This was caught during the work on bug 998025 (see https://bugzilla.mozilla.org/show_bug.cgi?id=998025#c79, point 1) and is fixed by the patches there, but bug 965871 landed for b2g 1.4, so we should uplift the fix there.

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp#1417
[2] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp#1730

Comment 1

[Botond Ballo [:botond]]

Attachment: bug1015331.patch

Comment 2

[Botond Ballo [:botond]]

Comment on attachment 8427898 [details] [diff] [review]
bug1015331.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 965871 (APZ fling handoff).

User impact if declined: 
  Platforms that use APZ (notably, B2G 1.4) will suffer a small memory leak
  every time a fling is handed off from a child element to a parent element.

Testing completed (on m-c, etc.): 
  Locally and on Try.

Risk to taking this patch (and alternatives if risky): 
  Very low.

String or IDL/UUID changes made by this patch:
  None.

Comment 3

[Botond Ballo [:botond]]

Green try:
  https://tbpl.mozilla.org/?tree=Try&rev=4ec7d365b86c

Landed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a9b3c891a2

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4a9b3c891a2

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

This is neither blocking B2G, nor tracking for Firefox 30, so this late in the Beta cycle it's not clear the risk of taking this is worth the reward.  Will approve for Aurora.

Comment 6

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

FWIW I think this should at least go into B2G 1.4, if not Firefox 30. The leak may be small but it will accumulate rapidly and be significant for a long-running process.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Needs a branch patch for Aurora uplift.

Comment 8

[Preeti Raghunath(:Preeti)]

How much memory leak are we observing?

Need impact from memory leak perspective.

Comment 9

[Botond Ballo [:botond]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #7)
> Needs a branch patch for Aurora uplift.

Turns out this depends, for a clean uplift, on bug 1000633, which was uplifted to B2G 1.4 but not aurora. I nominated it for aurora uplift. This patch should then apply cleanly.

Comment 10

[Botond Ballo [:botond]]

(In reply to Preeti Raghunath(:Preeti) from comment #8)
> How much memory leak are we observing?
> 
> Need impact from memory leak perspective.

The Task object itself which leaks on every fling handoff is, by code inspection, < 50 bytes in size, including memory allocation overhead.

However, the Task object also keeps a reference to the AsyncPanZoomController object, keeping it alive. This is potentially a significantly larger leak - I'm not sure how to quantify it - but it also happens less often: once per scrollable element that experiences fling handoff, rather than once per fling handoff.

Comment 11

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

(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Preeti Raghunath(:Preeti) from comment #8)
> > How much memory leak are we observing?
> > 
> > Need impact from memory leak perspective.
> 
> The Task object itself which leaks on every fling handoff is, by code
> inspection, < 50 bytes in size, including memory allocation overhead.

50 bytes, but on almost every scroll action.

> However, the Task object also keeps a reference to the
> AsyncPanZoomController object, keeping it alive. This is potentially a
> significantly larger leak - I'm not sure how to quantify it - but it also
> happens less often: once per scrollable element that experiences fling
> handoff, rather than once per fling handoff.

So pretty much any APZC instance that gets flung gets leaked?

Comment 12

[Preeti Raghunath(:Preeti)]

(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Preeti Raghunath(:Preeti) from comment #8)
> > How much memory leak are we observing?
> > 
> > Need impact from memory leak perspective.
> 
> The Task object itself which leaks on every fling handoff is, by code
> inspection, < 50 bytes in size, including memory allocation overhead.
> 
> However, the Task object also keeps a reference to the
> AsyncPanZoomController object, keeping it alive. This is potentially a
> significantly larger leak - I'm not sure how to quantify it - but it also
> happens less often: once per scrollable element that experiences fling
> handoff, rather than once per fling handoff.

Got it. Thanks for the analysis. Not comfortable leaving this leak issue to become significant impact. Hence taking it in 1.4.

Comment 13

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> (In reply to Botond Ballo [:botond] from comment #10)
> > (In reply to Preeti Raghunath(:Preeti) from comment #8)
> > > How much memory leak are we observing?
> > > 
> > > Need impact from memory leak perspective.
> > 
> > The Task object itself which leaks on every fling handoff is, by code
> > inspection, < 50 bytes in size, including memory allocation overhead.
> 
> 50 bytes, but on almost every scroll action.

Only flings that are handed off incur the leak. Basically these are flings that take you to the end of the element's scroll range in the direction of the fling.

> > However, the Task object also keeps a reference to the
> > AsyncPanZoomController object, keeping it alive. This is potentially a
> > significantly larger leak - I'm not sure how to quantify it - but it also
> > happens less often: once per scrollable element that experiences fling
> > handoff, rather than once per fling handoff.
> 
> So pretty much any APZC instance that gets flung gets leaked?

Any APZC instance that gets flung with the fling reaching the end of the scroll range.

Comment 14

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

In practice while working on fling acceleration and such I've noticed that most flings are handed off because of the way gaia apps are architected with nested scrollable elements. For example IIRC in the settings app almost any scroll will hand off a horizontal component from the inner APZ (which doesn't scroll horizontally) to the outer APZ (which doesn't scroll on either axis).

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d74b6d5766d4

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d03d09ae8b6f