RumViewScope creates memory leaks about dd-sdk-android HOT 11 CLOSED

Thanks for the quick reply @mariusc83

I don't think this necessarily has to do with the use of a background thread, though the executor used here probably increases the delays.

Here's how I'd think about it:

• In a Java based runtime, a memory leak is a programming error that prevents the GC from collecting an object that is no longer needed.
• Activities and fragments should become unreachable immediately after their onDestroy() method is invoked. Preventing them from being garbage collected, even if it's only temporary, is still a memory leak. E.g. here if we were switching activities fast enough we'd be retaining quite a lot of memory.
• This leak is particularly interesting in that the duration of the leak isn't well defined. It could be very short or very long, as it depends on a specific set of conditions: 1) RumViewScope staying as a child of RumViewManagerScope after the activity is destroyed 2) Any StopView event being sent close enough to a GC that the GC sees the java local reference when running and concludes that the object is reachable. Unfortunately, the more you use an app, the more the GC runs but also the more screens go in and out and the more we get StopView events.

I think long term I'd suggest something like: 1) Stop using the actual activity & fragment instances as keys, create unique identifiers for those instead (I don't know if you have any code expecting actual activity / fragment instances as keys though?). then 2) Stop using a weak ref, just keep a strong reference to the key, and make it clear that key should be an identifier not a live UI object.

from dd-sdk-android.

Hi @pyricau, thank you for all the details provided regarding this bug, tremendous work there.

This is indeed a very interesting discovery, so if I understand correctly because the onStopView is being called in a different Thread by using the keyRef.get() in there we are impeding that activity or fragment to be recycled in the GC cycle that could run in parallel. Is that correct ? Please correct me if I am wrong but this doesn't mean that the activity or fragment will always be leaked, is just that this is not going to be GC fast enough ?

In any case we are going to fix this on our next version probably by just applying the quick fix first but on the long term I totally agree with you that we should find a better equivalent for those keys.

from dd-sdk-android.

Could we call WeakReference.refersTo instead? The docs say "Prefer this to a comparison with the result of get"

from dd-sdk-android.

@pyricau pointed out that refersTo is only available on 33+. :( I'll close my PR.

If there's interest I can do something similar with System.identityHashCode. i.e. I could avoid calling WeakReference.get() except when the identity hash codes matched. This would be the case when they were the same object, but in such cases the object would definitely still be alive, so there would be no leak problem created. It would be very unlikely to happen spuriously since identity hash codes are 32 bits.

I'm happy to prepare a PR if you're interested in such a fix. Let me know.

from dd-sdk-android.

@tcmulcahy thank you for all the suggestions, we discussed it internally and we will take the time to prepare a more robust solution there. We will handle this on our end. As @pyricau was pointing out and I also agree with, the best approach would be to not have those activities/fragments as keys in there and totally remove the WeakRef. As that code there is quite complex and has a lot of implications we will need more time to assess and properly fix this in a PR.

On the other hand in the meantime we will add the quick fix for our next sdk version by not accessing the reference if the view was already stopped.

Let me know if this is ok with you @pyricau @tcmulcahy.

from dd-sdk-android.

Sounds good! I had a PR with the quick fix at #1763 but happy to close it.

from dd-sdk-android.

from dd-sdk-android.

@pyricau no need to close it, the PR is actually good, I just approved it and we will merge it soon.

from dd-sdk-android.

The issue referenced is fixed in #1779, but we will keep this ticket open for the remaining work to be done.

from dd-sdk-android.

The fix for this issue is now available in version 2.4.0.

from dd-sdk-android.

Hi @pyricau! We've made changes to our code and no more hold Activities inside the RUM processing pipeline. This change was released in version 2.5.0.

I am closing this issue, don't hesitate to re-open it if needed.

from dd-sdk-android.