Combating Memory Leaks

I know I've said that using a Loader would not introduce any memory leaks. That's only half the truth though, because it depends on how the Loader is instantiated. The typical case of :

public class FragmentListDevices extends 
    Fragment implements LoaderManager.
            LoaderCallbacks<Pair<Device,String>> {
 ...
 ...
 ...
 @Override
    public Loader<Pair<Device,String>>
     onCreateLoader(int i, final Bundle args) {
        return new AsyncTaskLoader<Pair<Device,String>>
                   (getContext()) {
            private Pair<Device,String> cached;
            @Override
            public Pair<Device,String> loadInBackground() {
                try {
                    if (RetrofitSendJSONobj.getMenuValues().
                        getJsonWithMenuData()==null) {
                            RetrofitSendJSONobj.
                               getMenuValuesJSONSync();
                    }
                    Device device = RetrofitSendJSONobj.
                                            sendValuesSync();
                    
                    String scrolling =
                        args.getString("scrolling");

                   Pair<Device,String> pair = new 
                        Pair<>(device,scrolling);
                   
                   return pair;
                } catch (Exception e) {
                    e.printStackTrace();
                    throw(e);
                }
            }
            @Override
            protected void onStartLoading() {
                super.onStartLoading();
                if (cached == null) {
                   forceLoad();
                }
                else {
                    super.deliverResult(cached);
                }
            }
            @Override
            public void deliverResult(
                      Pair<Device, String> data) {
                cached=data;
                super.deliverResult(data);
            }
        };
    }
 ...
 ...
 ...
}

has Android Studio warn that

"This Loader class should be static or leaks might occur (anonymous android.support.v4.content.AsyncTaskLoader)"

The issue here is that AsyncTaskLoader is declared as an inner class of FragmentListDevices and as such it has the bad habit of keeping an implicit reference to its outer/parent class, making for the number one cause of memory leaks.

There's this very good Making loading data lifecycle aware article on that matter by Ian Lake, Android Framework Developer at Google, which suggests that AsyncTaskLoader should be declared as a static inner class, which would forgo keeping that reference:

"You might have noticed the static keyword when declaring the JsonAsyncTaskLoader. It is incredibly important that your Loader does not contain any reference to any containing Activity or Fragment and that includes the implicit reference created by non-static inner classes. Obviously, if you’re not declaring your Loader as an inner class, you won’t need the static keyword."

If you go the static way however be warned that you'll loose all the flexibility of easily accessing the outer class's member fields because those can't be accessed inside the enfirced static context.In this case that's field RetrofitSendJSONobj and derived Context (through getContext).

So in order to go static, AsyncTaskLoader should be refactored to accept those values as arguments to its constructor.But if you descend down that way, then why not opt for the much cleaner approach of moving AsyncTaskLoader to its own file/namespace therefore totally disengaging it from its parent?

So a new class, FragmentListDevicesLoader which extends AsyncTaskLoader was introduced, turning the initial

 
@Override
    public Loader<Pair<Device,String>>
         onCreateLoader(int i, final Bundle args) {
           return new AsyncTaskLoader<Pair<Device,String>>(
                             getContext()) {
               private Pair<Device,String> cached;
to:
 
 @Override
    public Loader<Pair<Device, String>> 
            onCreateLoader(int i, final Bundle args) {
                  return new FragmentListDevicesLoader(
                   getContext(), args, RetrofitSendJSONobj);
    }

However there's yet another situation casting doubt over the potentiality of memory leaks.What about the Context we pass into the constructor? Isn't that going to keep an explicit this time reference to the context derived from the parent activity/fragment class? In that case WeakReference can be used which can be garbage collected when the need arises:

WeakReference<Context> ctx = new WeakReference<Context>(
                            getContext());
@Override
    public Loader<Pair<Device, String>> 
           onCreateLoader(int i, final Bundle args) {
       return new FragmentListDevicesLoader(
               ctx.get(), args, RetrofitSendJSONobj);
    }

It certainly looked plausible but I've reached out to Ian Lake just to make sure :

"Should context in

public JsonAsyncTaskLoader(Context context) {

super(context);

}

be turned into a WeakReference<Context>?"

His reply :

"That isn’t necessary. Per the note in the loadInBackground method, AsyncTaskLoader does not store a reference to the Activity context itself, but a reference to the application context."

nicely settled the matter.

The percussions of the implicit reference rule do not stop here however.There's many more places to trip up without even noticing.For example the event handlers attached to the Views are after all anonymous inner classes themselves:

Anonymous inner classes as Event Handlers

The same for Lambdas that act in place of anonymous classes

Lambdas as anonymous inner classes

or Retrofit and Picasso callbacks

Retrofit callbacks

Picasso callbacks

So many places to trip up.If you look closer at those pics you'll see me trying to access the outer parent class from within the potentially troublesome inner class using the notation Outer.this. as in FragmetListDevices.this., FragmentComparable.this. . Digging deeper, lets examine the part where I dynamically set up 6 Checkboxes inside the Filter screen.

 
 for (int i = 0; i < sensorsListJsonarray.length(); i++) {
        JSONObject object2 = 
                  sensorsListJsonarray.getJSONObject(i);
        CheckBox checkBox = new CheckBox(
               FragmentFilterDevices.this.getContext());
        checkBox.setId(object2.getInt("LookUpId"));
        checkBox.setText(
           object2.getString("LookUpDescription"));
        checkBox.setWidth(400);
                    
        if (mValues.getValuesLookUp()!=null) {
                 mValues.getValuesLookUp().forEach(x -> {
                     if (x != null && checkBox.getId() == x) {
                             checkBox.setChecked(true);
                             valuesLookUpChecked.add(x);
                                    }
                                }
                        );
          }
                    
         checkBox.setOnCheckedChangeListener(
              (buttonView, isChecked)->{
                       int id = buttonView.getId();
                        if (buttonView.isChecked() && 
                          !(valuesLookUpChecked.contains(id)))
                        {
                             valuesLookUpChecked.add(
                                 buttonView.getId());
                         }
                         else if (!buttonView.isChecked() && 
                           (valuesLookUpChecked.contains(id)))
                        {
                             valuesLookUpChecked.remove
                            (valuesLookUpChecked.indexOf(id));
                         }
                    });
                    sensorsLayout.addView(checkBox);
                }

Filter screen - The Checkboxes

The question therefore is, is rotating the device going to create a memory leak due to the Checkboxes' lambdas acting as event handlers? Android Studio's Memory profiler seems to agree.In The following screen we see that at each rotation 6 new lambda instances are created, thus 6 screen rotations equal 6x6=36 lambda instances.These keep implicit references to parent FragmentFilterDevices not letting it to get GC'd, hence the number of its instances in memory keeps adding up.

Lambdas memory leak

In this occasion, the usual way of releasing memory is through keeping file/global scope references in member fields to those lambdas, in order to null them out upon FragmentFilterDevices.onDestroyView.But this wasn't optimal as in most cases I had the lambdas defined in local scopes therefore unable to keep global pointers around.

Since laziness is a virtue, I forced myself to figure out a way of cleaning up auto-magically.What if there was a way to use the Visitor pattern to traverse every node (View) inside the fragment, CheckBoxes or otherwise, in order to destroy them? This would in effect null out the lambdas recursively.

Transferring the convention of the Utility class from C# but adapting it to Java's intricacies, namely that you can't have top level static classes, class TreeCleanUp was born

import android.view.View;
import android.view.ViewGroup;
import android.widget.AdapterView;
import com.google.android.apps.common.testing.ui.
                    espresso.util.TreeIterables;
public class TreeCleanUp {
    public static class TreeCleanUpNested {
        public static void TreeCleanUpNestedViews(
                     View rootView){
            for (View v : TreeIterables.
                     FirstViewTraversal(rootView)) {
                if ((v instanceof ViewGroup) &&
                     !(v instanceof AdapterView)) {
                    ((ViewGroup) v).removeAllViews();
                }
            }
        }
    }
}

TreeCleanUp in essence wraps the handy TreeIterables class of the Espresso UI testing library which in turn has methods for traversing the ViewGroups of the fragment.

So whenever FragmentFilterDevices.onDestroyView takes place, TreeCleanUp makes a call to TreeIterables to go through all the fragment's ViewGroups in order to destroy all their child views first before finally destroy them as well (with depthFirstViewTraversal)

@Override
    public void onDestroyView() {
        super.onDestroyView();
        TreeCleanUp.TreeCleanUpNested.
          TreeCleanUpNestedViews(rootView);
    }

Performing the same screen rotating experiment has the Profiler telling of success.

Lambdas memory leak solved

However there's a case that it's not that clear cut.Take a look at the following lambda which doesn't act as a replacement for an anonymous class :

Does it act as a closure that wraps up on its enclosing scope? I couldn't find an authoritative answer but the Profiler hinted that a memory leak was in fact introduced. Now, I of course have no saying on when the Garbage Collector kicks in but by nulling everything out I at least indicate to it their availability, hoping for a much quicker recovery.