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)

All
Android
defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: sebastian, Assigned: jchen)

References

Details

Attachments

(3 files)

Attached file leak-report.txt
> * 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
Summary: LeakCanary: GeckoPreferences has leaked 246KB → LeakCanary: (PrefsHelper$PrefHandlerBase) GeckoPreferences has leaked 246KB
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)
Assignee: nobody → nchen
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+
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)
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)
https://hg.mozilla.org/mozilla-central/rev/81ba35b38dc1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.