Memory leak in AppboySlideupManager

[MichaelEvans]

Looks like you're leaking sInstance. 👎

[briancaw]

Thanks for reporting @MichaelEvans - do you have steps to reproduce?

[briancaw]

@MichaelEvans Just pinging on this incase the opening and closing of the issue messed up the signal.

[MichaelEvans]

Sorry, totally missed notifications for this. Still seeing some in 1.8.0.

Try this 😄 https://github.com/square/leakcanary

[briancaw]

Hi @MichaelEvans we can reproduce and we'll have a fix out in an upcoming release. We'll update on timing as the release approaches. Thanks very much for reporting.

[briancaw]

Hi Again @MichaelEvans

It appears that this is a false alarm. AppboySlideupManager is a singleton static instance that persists throughout the life of the process it's in. Therefore, it should not be garbage collected.

When looking at what LeakCanary's RefWatcher does:

Use a RefWatcher to watch references that should be GCed:

...

RefWatcher.watch() creates a KeyedWeakReference to the watched object. Later, in a background thread, it checks if the reference has been cleared and if not it triggers a GC. If the reference is still not cleared, it then dumps the heap into a .hprof file stored on the app file system.

So because our static singleton AppboySlideupManager is always supposed to be around, it's never going to be cleared by a GC. In fact, LeakCanary seems to throw that same error if we added a RefWatcher to any static object.

Please let me know if there's more information regarding this leak or if you agree with this analysis and we can close this. Thanks!

[briancaw]

Hi @MichaelEvans have you had a chance to take a look at this? Do you agree with this assessment? Please let us know as it'd be good to close this out if it's not an issue or keep investigating if it is.

[MichaelEvans]

@briancaw I think the problem is that you need to disassociate this from the Activity. By keeping this reference, the Activity can never be GC'd. If you want to keep it around always, maybe using the Application context is a better idea?

[yishai-glide]

don't think application context is good here since they create views from
the manager

On Wed, Aug 12, 2015 at 7:44 PM, Michael Evans notifications@github.com
wrote:

@briancaw https://github.com/briancaw I think the problem is that you
need to disassociate this from the Activity. By keeping this reference, the
Activity can never be GC'd. If you want to keep it around always, maybe
using the Application context is a better idea?

—
Reply to this email directly or view it on GitHub
#40 (comment)
.

Have a great day.

Yishai Levenglick
Android-dev team
Glide Talk Ltd.

[briancaw]

That's right @yishai-glide we do need the activity. @MichaelEvans we change the activity we point to with mActivity on every onResume/onPause (per integration instructions calling register/unregisterInAppMessageManager in those methods) so it's only pointing to the current activity, so the previous one should only be kept around for a very short period of time. Are you seeing activities being persisted in memory for long periods of time as a result of this? We have not seen that behavior in our testing.

Incidentally, after this initial investigation we did update our code to null out mActivity in unregisterInAppMessageManager, but more for code cleanliness and to make the functionality more explicit. That'll be in the next release.

[yishai-glide]

maybe use a WeakReference?

On Wed, Aug 12, 2015 at 7:53 PM, briancaw notifications@github.com wrote:

That's right @yishai-glide https://github.com/yishai-glide we do need
the activity. @MichaelEvans https://github.com/MichaelEvans we change
the activity we point to with mActivity on every onResume/onPause (per
integration instructions calling register/unregisterInAppMessageManager in
those methods) so it's only pointing to the current activity, so the
previous one should only be kept around for a very short period of time.
Are you seeing activities being persisted in memory for long periods of
time as a result of this?

Incidentally, after this initial investigation we did update our code to
null out mActivity in unregisterInAppMessageManager, but more for code
cleanliness and to make the functionality more explicit. That'll be in the
next release.

—
Reply to this email directly or view it on GitHub
#40 (comment)
.

Have a great day.

Yishai Levenglick
Android-dev team
Glide Talk Ltd.

[samdozor]

@briancaw nulling out mActivity in unregisterInAppMessageManager sounds like the perfect solution to me.

[briancaw]

We've implemented the nulling out of mActivity in unregisterInAppMessageManager with our recent 1.9.0 release. Thanks for the input and feedback everyone.