search

telenordigital / connect-android-sdk

Android SDK for CONNECT ID
https://telenordigital.github.io/id-docs.telenordigital.com/
Other
16 stars 14 forks source link

Fix SmsBroadcastReceiver leaking activity #156

Closed simonnorberg closed 5 years ago

simonnorberg commented 5 years ago

We get an activity leak when we login with ChromeCustomTab in SDK 1.9.5. Possibly related to #145

com.telenor.app.mytelenor.ui.LoginActivity has leaked: thread Thread.!()! (named 'Answers Events Handler1') ↳ static ExecutorUtils$1$1.!($class$dexCache)! ↳ DexCache.!(runtimeInternalObjects)! ↳ array Object[].!([1376])! ↳ static ConnectSdk.!(smsBroadcastReceiver)! ↳ SmsBroadcastReceiver.!(smsHandler)! ↳ ConnectSdk$2.!(val$activity)! (anonymous implementation of com.telenor.connect.sms.SmsHandler) ↳ LoginActivity

It looks like smsBroadcastReceiver is only unregistered if an SMS is received? https://github.com/telenordigital/connect-android-sdk/blob/master/connect/src/com/telenor/connect/ConnectSdk.java#L152-L165

I'm not sure it is the best solution but unregistering/clearing smsBroadcastReceiver after successful login seems to fix the leak.

jorunfa commented 5 years ago

I think you are completely right that I/we missed that scenario. I like the way you're doing it here, but I think you have to surround the thing in a try/catch-ignore here, in case the user did get an SMS and it was unregistered already

try {
    unregisterReceiver(broadcastReceiver);
} catch (IllegalArgumentException ignore) {}

And if you wrap the change in a method called safeUnregisterAndRemoveBroadcastReceiver I would like to use that method for the other unregister as well (in the BroadcastReceiver).

Might it be that the revert did nothing about the memory leak, then? I suggest I merge your change when it is ready; I revert the revert; and then I can make an alpha release which you could test?

simonnorberg commented 5 years ago

I added the method safeUnregisterAndRemoveBroadcastReceiver and it works for me but I'm unable to test the receivedSms callback.

The weak reference in Custom tabs login button is still needed. That was a different leak trace.

jorunfa commented 5 years ago

Will try to get this released before the weekend.

simonnorberg commented 5 years ago

Thanks!

jorunfa commented 5 years ago

https://github.com/telenordigital/connect-android-sdk/releases/tag/v1.9.6

jorunfa commented 5 years ago

There is also an updated instant verification release (if you didn't already test the other one): https://github.com/telenordigital/connect-android-sdk/releases/tag/v1.11.2-beta

simonnorberg commented 5 years ago

Awesome!

jorunfa commented 5 years ago

@simonnorberg: Did you get a chance to test the beta, by any chance? If you can log in once without any hiccups then it's probably fine.

If you don't have the time, then that is something I can relate to 😏.

simonnorberg commented 5 years ago

@jorunfa I've tried 1.11.3-beta now and I can login without any problems.

jorunfa commented 5 years ago

Thanks for checking out!