search

OneSignal / OneSignal-Android-SDK

OneSignal is a free push notification service for mobile apps. This plugin makes it easy to integrate your native Android or Amazon app with OneSignal. https://onesignal.com
Other
604 stars 368 forks source link

ActivityLifecycleHandler#curActivity leaks Android Activities #342

Closed runningcode closed 5 years ago

runningcode commented 6 years ago

ActivityLifecycleHandler#curActivity keeps static references to android activities. This should probably be a WeakReference in order to avoid memory leaks.

This also breaks instant run for any consumers of OneSignal.

Could be related to #258

Here is this same error in lint:

screen shot 2017-10-23 at 14 27 14
runningcode commented 6 years ago

Any updates on this?

jkasten2 commented 6 years ago

@runningcode The current code shouldn't be creating a leak of the Activity. curActivity will be set to null when the Activity is stopped, paused, or destroyed. Code of where this is done linked below. https://github.com/OneSignal/OneSignal-Android-SDK/blob/3.6.5/OneSignalSDK/onesignal/src/main/java/com/onesignal/ActivityLifecycleHandler.java#L79 https://github.com/OneSignal/OneSignal-Android-SDK/blob/3.6.5/OneSignalSDK/onesignal/src/main/java/com/onesignal/ActivityLifecycleHandler.java#L90 https://github.com/OneSignal/OneSignal-Android-SDK/blob/3.6.5/OneSignalSDK/onesignal/src/main/java/com/onesignal/ActivityLifecycleHandler.java#L101

The static analysis that the lint tool doesn't account for the logic above. However even in this case it should still be converted to a weak reference as the implementation is fragile to leaking with future code changes.

Let us know if you are detecting a leak at runtime and I can give this issue a higher priority.

MuhammadMuzammilQadri commented 6 years ago

Yes we are detecting a leak at runtime. Here is the stack trace from leak canary:

In com.telenor.capp.farmsupplies:2.0.1:2.

jkasten2 commented 6 years ago

@MuhammadMuzammilQadri Thanks for reporting with the full state. I looks like com.onesignal.ActivityLifecycleHandler.curActivity does have a static reference to com.telenor.capp.farmsupplies.ui.main.MainActivity.mFragments but I don't see any other Activities so this shouldn't be a production issue.

Was your app in the background or foreground at this time?

runningcode commented 6 years ago

What do you mean by "you don't see any other activities"? This isn't the full heap. This is just the shortest path to GC root. By default LeakCanary triggers a warning like this if an Activity is destroyed but there is still a reference to it in the heap. In this case this Activity should have been garbage collected but OneSignal is keeping a reference to it and preventing it from being garbage collected. One way to fix it would simply be to replace the strong reference to the activity with a WeakReference.

jkasten2 commented 6 years ago

@runningcode I see, didn't know if it printed all Activities. OneSignal shouldn't be leaving any references behind as it does hook into the onActivityDestroyed and clears it's reference.

From looking at state log above again it seems like the Activity is in a in focus state;

Instance of com.telenor.capp.farmsupplies.ui.main.MainActivity
...
| isOnPauseCalled = false
...
| mCreated = true
...
| mReallyStopped = false
| mRequestedPermissionsFromFragment = false
| mResumed = true
| mRetaining = false
| mStopped = false

I believe LeakCanary might be just looking for static references in this case then. Let me know if you think this isn't the case.

I agree that WeakReference is the correct path forward here. All Context references on Android should be referenced this way with the only exception being the ApplicationContext and only if it doesn't create issues with "Instant Run". This issue has been lower priority with us we haven't been able to reproduce a leak in our testing. We have also reviewed the code were not able to find a case were it would leak. For safely our plan is still to switch to WeakReference here.

rgomezp commented 5 years ago

Closing issue. Note: request still under consideration