834470 - Leaked AsyncPanZoomController and GestureEventListener due to non-CC'd cycle

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

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)]

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]]

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]

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]

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]

Attachment: 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]

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]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c845b323d0

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

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)]

Filed bug 834659 to have that separate conversation.

Comment 10

[Johnny Stenback (:jst)]

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]

Thanks for rebasing, jst!

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d5d6ef77f2d9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1