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

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bjacob, Assigned: cjones)

Tracking

(Blocks: 1 bug)

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-, firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [found-by-refgraph][MemShrink])

Attachments

(1 attachment)

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.
(Reporter)

Comment 1

5 years ago
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
Whiteboard: [found-by-refgraph] → [found-by-refgraph][MemShrink]

Comment 2

5 years ago
Isn't AsyncPanZoomController used off-main-thread, so it can't be cycle collected.
Yes, these are non-main-thread objects.  But let me confirm that we manually break this cycle.
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.
Assignee: nobody → jones.chris.g
Blocks: 797189
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 ...)
Attachment #706187 - Flags: review?(roc)
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.
blocking-b2g: --- → tef?
Attachment #706187 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c845b323d0
(Reporter)

Comment 8

5 years ago
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?
(Reporter)

Updated

5 years ago
Blocks: 834659
(Reporter)

Comment 9

5 years ago
Filed bug 834659 to have that separate conversation.
Not blocking, but tracking, and we'll approve the patch so it can land.
blocking-b2g: tef? → -
tracking-b2g18: --- → +

Updated

5 years ago
Attachment #706187 - Flags: approval-mozilla-b2g18+
Thanks for rebasing, jst!

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d5d6ef77f2d9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-b2g18: --- → fixed
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/b7c845b323d0
Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1
status-b2g18-v1.0.1: --- → fixed
You need to log in before you can comment on or make changes to this bug.