Memory leak in Fragment

Question

I am using the LeakCanary library to monitor memory leaks in my app. I received this memory leak and not sure how to track down what is causing it.

05-09 09:32:14.731  28497-31220/? D/LeakCanary﹕ In com.etiennelawlor.minesweeper:0.0.21:21.
    * com.etiennelawlor.minesweeper.fragments.MinesweeperFragment has leaked:
    * GC ROOT com.google.android.gms.games.internal.GamesClientImpl$PopupLocationInfoBinderCallbacks.zzahO
    * references com.google.android.gms.games.internal.PopupManager$PopupManagerHCMR1.zzajo
    * references com.google.android.gms.games.internal.GamesClientImpl.mContext
    * references com.etiennelawlor.minesweeper.activities.MinesweeperActivity.mFragments
    * references android.app.FragmentManagerImpl.mAdded
    * references java.util.ArrayList.array
    * references array java.lang.Object[].[0]
    * leaks com.etiennelawlor.minesweeper.fragments.MinesweeperFragment instance
    * Reference Key: 2f367393-6dfd-4797-8d85-7ac52c431d07
    * Device: LGE google Nexus 5 hammerhead
    * Android Version: 5.1 API: 22
    * Durations: watch=5015ms, gc=141ms, heap dump=1978ms, analysis=23484ms

This is my repo : https://github.com/lawloretienne/Minesweeper

This seems to be an elusive one. I set up an Interface to communicate between a Fragment and an Activity. I set this mCoordinator Interface variable up in onAttach() then I realized I was not nulling it out in onDetach(). I fixed that issue but still am getting a memory leak. Any ideas?

Update

I disabled the Fragment leak watching, and I still get a notification about the activity leaking with the following leak trace :

05-09 17:07:33.074  12934-14824/? D/LeakCanary﹕ In com.etiennelawlor.minesweeper:0.0.21:21.
    * com.etiennelawlor.minesweeper.activities.MinesweeperActivity has leaked:
    * GC ROOT com.google.android.gms.games.internal.GamesClientImpl$PopupLocationInfoBinderCallbacks.zzahO
    * references com.google.android.gms.games.internal.PopupManager$PopupManagerHCMR1.zzajo
    * references com.google.android.gms.games.internal.GamesClientImpl.mContext
    * leaks com.etiennelawlor.minesweeper.activities.MinesweeperActivity instance
    * Reference Key: f4d06830-0e16-43a2-9750-7e2cb77ae24d
    * Device: LGE google Nexus 5 hammerhead
    * Android Version: 5.1 API: 22
    * Durations: watch=5016ms, gc=164ms, heap dump=3430ms, analysis=39535ms

Answer 1

The documentation indicates that it is safe to call connect() even if the state is "connected" or "connecting". It also indicates that you can safely call disconnect() no matter what the connection state is. Therefore, I would remove the "if" statements around the calls to connect() and disconnect(). However, I doubt that that will make this "leak" go away.

It is clear that GamesClientImpl is storing a reference to your Activity as a Context. I imagine this is occuring in the construction of the GoogleApiClient that happens when you call GoogleApiClient.Builder.build(). It seems like a bug to me that the instance of GoogleApiClient is still around after your Activity has finished. However, if you are supposed to call connect() in onStart() and disconnect() in onStop() that seems to imply that you can reuse the connection (since onStart() and onStop() can be called repeatedly). For this to work, the GoogleApiClient must keep the reference to your Context even after you've called disconnect().

You could try using the global application context instead of your Activity context when creating the GoogleApiClient, as the global application context lives forever (until the process is killed). This should make your "leak" go away:

// Create the Google Api Client with access to Plus and Games
mGoogleApiClient = new GoogleApiClient.Builder(getApplicationContext())
    .addConnectionCallbacks(this)
    .addOnConnectionFailedListener(this)
    .addApi(Plus.API).addScope(Plus.SCOPE_PLUS_LOGIN)
    .addApi(Games.API).addScope(Games.SCOPE_GAMES)
    .build();

Answer 2

05-27 13:15:04.478  24415-25236/com.package D/LeakCanary﹕ In com.package:0.0.52-dev:202.
* com.package.launcher.LauncherActivity has leaked:
* GC ROOT com.google.android.gms.ads.internal.request.q.a
* references com.google.android.gms.ads.internal.request.m.d
* references com.google.android.gms.ads.internal.request.c.a
* references com.google.android.gms.ads.internal.j.b
* references com.google.android.gms.ads.internal.aa.f
* references com.google.android.gms.ads.internal.ab.mParent
* references com.google.android.gms.ads.doubleclick.PublisherAdView.mParent
* references android.widget.FrameLayout.mContext
* leaks com.package.launcher.LauncherActivity instance
* Reference Key: 9ba3c5ea-2888-4677-9cfa-ebf38444c994
* Device: LGE google Nexus 5 hammerhead
* Android Version: 5.1.1 API: 22
* Durations: watch=5128ms, gc=150ms, heap dump=5149ms, analysis=29741ms

I was using gms ads library and there was a similar leak. So I fixed the above case by handling it onDestroyView() of my fragment.

@Override
public void onDestroyView() {

    if (mAdView != null) {
        ViewParent parent = mAdView.getParent();
        if (parent != null && parent instanceof ViewGroup) {
            ((ViewGroup) parent).removeView(mAdView);
        }
    }
    super.onDestroyView();
}

So here I'm basically removing my PublisherAdView from it's parent on onDestroyView().

Also, note that I had to use Application Context when I created the PublisherAdView otherwise I would get the following leak:

05-27 13:59:23.684  10041-11496/com.package D/LeakCanary﹕ In com.package:0.0.52-dev:202.
* com.package.launcher.LauncherActivity has leaked:
* GC ROOT com.google.android.gms.ads.internal.request.q.a
* references com.google.android.gms.ads.internal.request.m.b
* leaks com.package.launcher.LauncherActivity instance
* Reference Key: 5acaa61a-ea04-430a-b405-b734216e7e80
* Device: LGE google Nexus 5 hammerhead
* Android Version: 5.1.1 API: 22
* Durations: watch=7275ms, gc=138ms, heap dump=5260ms, analysis=22447ms

Not sure if it's going to solve the question above directly but hope it helps.

Answer 3

You shouldn't rely on onDestroy() callback execution, in some cases it might not be called. More solid solution is to put your register/unregister code inside onResume()/onPause().

Same goes (for another reason, of course) for Fragment's onDetach(), move your sensible code to onStop() or onPause().

Answer 4

In my case, I had the following code:

googleApiClient = new GoogleApiClient.Builder(activity.getApplicationContext())
            .addConnectionCallbacks(this)
            .addOnConnectionFailedListener(this)
            .addApi(Games.API).addScope(Games.SCOPE_GAMES)
            .build();

The issue were the calls addConnectionCallbacks(this) and addConnectionCallbacks(this). The class referenced by this was keeping a reference to an activity, and as GoogleApiClient won't let go of the references to the connection callbacks/listener, this was resulting in a memory leak.

My solution was to register/unregister the callbacks as the googleApiClient connects/disconnects:

public void connect() {
    mGoogleApiClient.registerConnectionCallbacks(this);
    mGoogleApiClient.registerConnectionFailedListener(this);
    mGoogleApiClient.connect();
}

public void disconnect() {
    if (mGoogleApiClient.isConnected()) {
        mGoogleApiClient.disconnect();
        mGoogleApiClient.unregisterConnectionCallbacks(this);
        mGoogleApiClient.unregisterConnectionFailedListener(this);
    }
}