Last Comment Bug 834470 - Leaked AsyncPanZoomController and GestureEventListener due to non-CC'd cycle
: Leaked AsyncPanZoomController and GestureEventListener due to non-CC'd cycle
Status: RESOLVED FIXED
[found-by-refgraph][MemShrink]
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: ---
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
:
Mentors:
Depends on:
Blocks: slim-fast 834659
  Show dependency treegraph
 
Reported: 2013-01-24 14:40 PST by Benoit Jacob [:bjacob] (mostly away)
Modified: 2013-02-13 10:43 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-
wontfix
fixed
+
fixed
fixed


Attachments
Release AsyncPanZoomController and friends (3.92 KB, patch)
2013-01-24 17:40 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: review+
jst: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2013-01-24 14:40:39 PST
This was found by refgraph,

  https://github.com/bjacob/mozilla-central/wiki

There is a cycle of nsRefPtr's between AsyncPanZoomController and GestureEventListener that is not declared to the CC. The pointers are a nsRefPtr<GestureEventListener> and a nsRefPtr<AsyncPanZoomController>. Neither is traversed by the CC.

This seems like a real leak: after a few minutes of usage of B2G/Otoro, I have two AsyncPanZoomController's and two GestureEventListener's, forming two such cycles; none of these cycles is referenced by any strong reference; and one of these two cycles is not even referenced anymore by any pointer at all from any other heap block, so it is almost certainly leaked.
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2013-01-24 14:47:28 PST
Here's the refgraph summary for these cycles:



The not-yet-leaked one:

Cycle 33 has 2 vertices.

Is NOT traversed by the CC!

This cycle is NOT strong-referenced by outside!
Types involved: mozilla::layers::AsyncPanZoomController / mozilla::layers::GestureEventListener

type name: mozilla::layers::AsyncPanZoomController
address: 0x40421820
size: 1072 bytes
cycleIndex: 33
Cycle edges:
    nsRefPtr<mozilla::layers::GestureEventListener>  to mozilla::layers::GestureEventListener at 0x440da860
        NOT TRAVERSED BY CC
backRefCount: 0
backWeakRefCount: 1


type name: mozilla::layers::GestureEventListener
address: 0x440da860
size: 120 bytes
cycleIndex: 33
Cycle edges:
    nsRefPtr<mozilla::layers::AsyncPanZoomController>  to mozilla::layers::AsyncPanZoomController at 0x40421820
        NOT TRAVERSED BY CC
backRefCount: 0
backWeakRefCount: 0




The leaked one:

Cycle 34 has 2 vertices.

Is NOT traversed by the CC!

This cycle is NOT referenced at all by outside heap blocks!
Almost certainly leaked!
Types involved: mozilla::layers::AsyncPanZoomController / mozilla::layers::GestureEventListener

type name: mozilla::layers::AsyncPanZoomController
address: 0x4807c020
size: 1072 bytes
cycleIndex: 34
Cycle edges:
    nsRefPtr<mozilla::layers::GestureEventListener>  to mozilla::layers::GestureEventListener at 0x4ab132c0
        NOT TRAVERSED BY CC
backRefCount: 0
backWeakRefCount: 0


type name: mozilla::layers::GestureEventListener
address: 0x4ab132c0
size: 120 bytes
cycleIndex: 34
Cycle edges:
    nsRefPtr<mozilla::layers::AsyncPanZoomController>  to mozilla::layers::AsyncPanZoomController at 0x4807c020
        NOT TRAVERSED BY CC
backRefCount: 0
backWeakRefCount: 0
Comment 2 Olli Pettay [:smaug] 2013-01-24 15:00:34 PST
Isn't AsyncPanZoomController used off-main-thread, so it can't be cycle collected.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-01-24 15:04:26 PST
Yes, these are non-main-thread objects.  But let me confirm that we manually break this cycle.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-01-24 16:56:31 PST
It looks like we're not manually breaking that cycle currently :(.  This could result in growing b2g memory usage, but it would require many many thousands of tabs opened to see MBs getting away from us.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-01-24 17:40:07 PST
Created attachment 706187 [details] [diff] [review]
Release AsyncPanZoomController and friends

There was another Axis <-> APZC cycle to break, but that one is trivial since APZC bounds the lifetime of Axis.  (Yay conversions from Java ...)
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-01-24 17:41:25 PST
Use of the browser or any async-pan-zoomed apps can cause the b2g process to permanently "leak" memory, which eventually results in severe instability.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-01-24 19:59:28 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c845b323d0
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2013-01-25 04:21:13 PST
While we're here: shouldn't refcounting on APZC be dropped? It doesn't seem like refcounting is actually how the APZC's lifetime is managed. Havingf unused refcounting seems dangerous, because if now someone arrived here, saw APZC being refcounted, and made code holding a nsRefPtr<APZC>, then the APZC would get destroyed as soon as that nsRefPtr<APZC> would go away (because nothing else addref'd it), which would be surprising, right?
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2013-01-25 04:26:47 PST
Filed bug 834659 to have that separate conversation.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2013-01-25 08:47:30 PST
Not blocking, but tracking, and we'll approve the patch so it can land.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-01-25 11:46:57 PST
Thanks for rebasing, jst!

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d5d6ef77f2d9
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-01-26 16:54:45 PST
https://hg.mozilla.org/mozilla-central/rev/b7c845b323d0
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-13 10:43:19 PST
Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1

Note You need to log in before you can comment on or make changes to this bug.