Pushwoosh leaking my activity

Question

I am using Pushwoosh inside my application to receive push notifications. I am using the latest version of Pushwoosh library 3.1.14.

I have a screen structure like this.

Login Activity -> Main Activity with multiple tabs.

So I am implementing my pushwoosh related logic inside MainActivity. And I want to unregister from push on Logout, and go back to Login Activity.

My code is given below. I have filtered out all other sections unrelated to Pushwoosh. To be frank, this code is exactly similar to the code in Pushwoosh documentation here. The only difference is in the onLogout() method where I try to unregister from pushwoosh and go back to LoginActivity.

TabbarActivity.java

@Override
protected void onCreate(Bundle savedInstanceState) {
   super.onCreate(savedInstanceState);

   //Pushwoosh Registration
   registerReceivers();
   PushManager pushManager = PushManager.getInstance(this);
   pushManager.setNotificationFactory(new PushNotificationFactory());
   try {
       pushManager.onStartup(this);
   } catch(Exception e) {}

   //Register for push!
   pushManager.registerForPushNotifications();
   checkMessage(getIntent());
} 


@Override 
protected void onNewIntent(Intent intent) {
    super.onNewIntent(intent);    
    setIntent(intent);    
    checkMessage(intent);
}

@Override
protected void onResume() {    
    super.onResume();    
    registerReceivers();
}

@Override
protected void onPause() {    
    super.onPause();    
    unregisterReceivers();
}

BroadcastReceiver mBroadcastReceiver = new BaseRegistrationReceiver() {    
   @Override    
   public void onRegisterActionReceive(Context context, Intent intent) {        
       checkMessage(intent);    
   }
};

private BroadcastReceiver mReceiver = new BasePushMessageReceiver() {
   @Override    protected void onMessageReceive(Intent intent) {
     //JSON_DATA_KEY contains JSON payload of push notification.    
   }
};

public void registerReceivers() {
  IntentFilter intentFilter = new IntentFilter(
    getPackageName() + ".action.PUSH_MESSAGE_RECEIVE");
  registerReceiver(mReceiver, intentFilter, 
    getPackageName() +".permission.C2D_MESSAGE", null);    
  registerReceiver(mBroadcastReceiver, new IntentFilter(
    getPackageName() + "." + PushManager.REGISTER_BROAD_CAST_ACTION));
}

public void unregisterReceivers() {
  try {
    unregisterReceiver(mReceiver);    
  } catch (Exception e) {
    e.printStackTrace();    
  }

  try {        
    unregisterReceiver(mBroadcastReceiver);
  } catch (Exception e) {
    e.printStackTrace();   
  }
}

private void checkMessage(Intent intent) {
  if (null != intent) {
    if (intent.hasExtra(PushManager.REGISTER_EVENT)) {
       uploadPushTokenToServer(PushManager.getPushToken(this));
    }
    resetIntentValues();    
  }
}

private void resetIntentValues() {
  Intent mainAppIntent = getIntent();
  if (mainAppIntent.hasExtra(PushManager.PUSH_RECEIVE_EVENT)) {
    mainAppIntent.removeExtra(PushManager.PUSH_RECEIVE_EVENT);    
  } else if (mainAppIntent.hasExtra(PushManager.REGISTER_EVENT)) {
    mainAppIntent.removeExtra(PushManager.REGISTER_EVENT);
  } else if (mainAppIntent.hasExtra(PushManager.UNREGISTER_EVENT)) {
    mainAppIntent.removeExtra(PushManager.UNREGISTER_EVENT);
  } else if (mainAppIntent.hasExtra(PushManager.REGISTER_ERROR_EVENT)) {
    mainAppIntent.removeExtra(PushManager.REGISTER_ERROR_EVENT);
  } else if (mainAppIntent.hasExtra(PushManager.UNREGISTER_ERROR_EVENT)) {
    mainAppIntent.removeExtra(PushManager.UNREGISTER_ERROR_EVENT);
  }
  setIntent(mainAppIntent);
}

//Finally on logout
private void onLogout() {
   //other cleanup

   //pushwoosh
   PushManager.getInstance(this).unregisterForPushNotifications();

   //goback to login activity
}

I am getting pushes from server without any issue. The only problem I face is after I logout and go back to LoginActivity, TabbarActivity remains in memory, which in turns hold on to many other fragments and views. I tried debugging using MAT and this is what it shows up.

Class Name                                                                      | Ref. Objects | Shallow Heap | Ref. Shallow Heap | Retained Heap
--------------------------------------------------------------------------------------------------------------------------------------------------
com.pushwoosh.internal.request.RequestManager$1 @ 0x12f89ce0  Thread-1737 Thread|            1 |           88 |               360 |           536
'- val$context in.myproject.activities.TabbarActivity         @ 0x12d8ac40      |            1 |          360 |               360 |        18,520
--------------------------------------------------------------------------------------------------------------------------------------------------

I have also cross checked the same with LeakCanary tool, that also indicates that Pushwoosh is holding on to my activity.

So my question is, how can I cleanup pushwoosh to avoid my activity getting leaked?

Answer 1

The docs you've referenced, from looking at them briefly, are giving an example and these examples are not always the best way to implement the api into a fully functional app with other activities. I understand by using getInstance, you are attempting to use a singleton, but suspect this is not being managed well.

I would take control of the instance of PushManager that is being used for the duration of your app.

The problem may be the multiple instance of PushManager being created both from the scope and creating multiple instances of pushManager within the class, and possibly within the life of the program. This will cause leaks.

I would be making pushManager a class variable, rather than using PushManager.getInstance twice and consider creating a static instance of the PushManager to be used for the duration of the app, much like using a single database instance throughout an app.

On a class level:

PushManager pushManager;

and initializing in oncreate

pushManager = PushManager.getInstance(this);


//Finally on logout
private void onLogout() {
    //other cleanup

    //pushwoosh
    // Here the first instance is left dangling.
    // PushManager.getInstance(this).unregisterForPushNotifications();

    pushManager..unregisterForPushNotifications();

    //goback to login activity
}

This way you've cleaned up the resources for the one instance of pushmanager.

To use an app wide static PushManager:

static PushManager pushManager;

initialised as:

pushManager = new PushManager(this.getApplicationContext());

When is a Singleton not a Singleton?

Answer 2

@CommonsWare's comment is spot on. Looking at the (decompiled) source code of the Pushwoosh SDK, PushManager.onStartUp() forwards the supplied context straight to its RequestManager, which on its turn hands it over to a Thread that basically keeps running infinitely. Meaning it'll hang onto your activity instance long after it has become invalid.

Note how this is exactly what MAT is trying to tell you (and LeakCanary too probably).

In other words, internally a strong reference will be held to whatever you pass into onStartUp(), for the lifetime of your app. As such, make sure the context you supply has an appropriately scoped life cycle. In other words: the only correct option here is the application context.

You may want to file a bug report with Pushwoosh and inform them that the issue is:

public void onStartup(Context context) throws Exception {
    Context applicationContext = context.getApplicationContext();
    this.pushRegistrar.checkDevice(applicationContext);
    sendAppOpen(context); // <--- ISSUE
    ...
}

I can only guess someone had a bad day at the office and forgot to change the problematic line into sendAppOpen(applicationContext).

No promises that all leaks will be history after this change (I didn't dig too deep into the source), but it should at least solve the immediate issue at hand.

Also, and this can't be stressed enough, in general, if you don't know (or control) the life time of a component, use the application context. If an activity is really needed, the method signature will/should indicate so. If it's just asking for a context, play it safe. (And yes, of course there are plenty of exceptions to this rule of thumb, but generally it's easier to track that down issues as a result of that rather than those of memory leaks).

Answer 3

Well, I got a reply from Pushwoosh saying that they have fixed the issue. I downloaded their latest SDK and voila, the leak was gone. And it seems user @MH was spot on about the culprit code.

This is from decompiled source code of new SDK,

public void onStartup(Context context) throws Exception {
    Context applicationContext = context.getApplicationContext();
    this.pushRegistrar.checkDevice(applicationContext);
    sendAppOpen(applicationContext); // <--- NO ISSUE
    ...
}