Closed
Bug 1257934
Opened 8 years ago
Closed 8 years ago
LeakCanary: (PrefsHelper$PrefHandlerBase) GeckoPreferences has leaked 246KB
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: sebastian, Assigned: jchen)
References
Details
Attachments
(3 files)
> * org.mozilla.gecko.preferences.GeckoPreferences has leaked:
> * GC ROOT org.mozilla.gecko.preferences.GeckoPreferences$15.this$0 (anonymous class extends org.mozilla.gecko.PrefsHelper$PrefHandlerBase)
> * leaks org.mozilla.gecko.preferences.GeckoPreferences instance
Reporter | ||
Comment 1•8 years ago
|
||
Reporter | ||
Updated•8 years ago
|
Summary: LeakCanary: GeckoPreferences has leaked 246KB → LeakCanary: (PrefsHelper$PrefHandlerBase) GeckoPreferences has leaked 246KB
Assignee | ||
Comment 2•8 years ago
|
||
The pref handler class in GeckoPreferences doesn't need a reference back to GeckoPreferences, so it's better to make it a static class rather than a (non-static) anonymous inner class, in order to avoid leaking the GeckoPreferences instance inadvertently. To avoid confusion, the patch also renames the class to "PrefCallbacks", because GeckoPreferences already has an unrelated interface named "PrefHandler".
Attachment #8733080 -
Flags: review?(s.kaspari)
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → nchen
Reporter | ||
Comment 3•8 years ago
|
||
Comment on attachment 8733080 [details] [diff] [review] Make the GeckoPreferences pref handler a static class (v1) Review of attachment 8733080 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java @@ +1453,5 @@ > } > > // Initialize preferences by requesting the preference values from Gecko > + private static class PrefCallbacks extends PrefsHelper.PrefHandlerBase { > + private final PreferenceGroup screen; That's much nicer but I guess with this strong PreferenceGroup reference we still indirectly reference GeckoPreferences too? @@ +1558,2 @@ > final String[] prefNames = prefs.toArray(new String[prefs.size()]); > PrefsHelper.addObserver(prefNames, prefHandler); I wonder if the observer survived from here on and therefore GeckoPreferences could not be cleared: PrefsHelper -> (static) prefHandler (-> now: PreferenceGroup) -> GeckoPreferences.
Attachment #8733080 -
Flags: review?(s.kaspari) → review+
Assignee | ||
Comment 4•8 years ago
|
||
I took a look at the memory dumps and I didn't see a leak in the observer. When the preferences screen is shown, we do hold onto a reference to PrefHandler. But when preferences are dismissed, I see PrefHandler is garbage collected as well. Also, in my build, ProGuard automatically stripped away the PrefHandler's reference to GeckoPreferences because it's an unused reference. Did you turn off ProGuard in your build?
Flags: needinfo?(s.kaspari)
Reporter | ||
Comment 5•8 years ago
|
||
This report is from a normal local build - I assume this does not run proguard? But still this doesn't happen all the time. I only got this report once. So it's a really rare thing - maybe even just a race between LeakCanary's timers and the GC?
Flags: needinfo?(s.kaspari)
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/81ba35b38dc1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•