Describe what happened

RumViewScope prevents destroyed activities from being garbage collected, causing an increase in memory pressure.

This leak was hard to find: the destroyed activities were not being garbage collected, however every time myself or LeakCanary would look at a heap dump, they were always weakly reachable. I ended up tweaking the shortest path algorithm that LeakCanary uses to include weak references that aren't set by the Android Framework.

I eventually figured out that the culprit is the way the SDK abuses weak references and revives the references by frequently invoking WeakReference.get() and recreating temporary strong local references to the destroyed activities.

Leak Trace:

┬───
│ GC Root: Global variable in native code
│
├─ dalvik.system.PathClassLoader instance
│    Leaking: NO (GlobalRumMonitor↓ is not leaking and A ClassLoader is never leaking)
│    ↓ ClassLoader.runtimeInternalObjects
├─ java.lang.Object[] array
│    Leaking: NO (GlobalRumMonitor↓ is not leaking)
│    ↓ Object[23579]
├─ com.datadog.android.rum.GlobalRumMonitor class
│    Leaking: NO (a class is never leaking)
│    ↓ static GlobalRumMonitor.registeredMonitors
│                              ~~~~~~~~~~~~~~~~~~
├─ java.util.LinkedHashMap instance
│    ↓ LinkedHashMap[instance @378137616 of com.datadog.android.core.DatadogCore]
│                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
├─ com.datadog.android.rum.internal.monitor.DatadogRumMonitor instance
│    ↓ DatadogRumMonitor.rootScope
│                        ~~~~~~~~~
├─ com.datadog.android.rum.internal.domain.scope.RumApplicationScope instance
│    ↓ RumApplicationScope.childScopes
│                          ~~~~~~~~~~~
├─ java.util.ArrayList instance
│    ↓ ArrayList[0]
│               ~~~
├─ com.datadog.android.rum.internal.domain.scope.RumSessionScope instance
│    ↓ RumSessionScope.childScope
│                      ~~~~~~~~~~
├─ com.datadog.android.rum.internal.domain.scope.RumViewManagerScope instance
│    ↓ RumViewManagerScope.childrenScopes
│                          ~~~~~~~~~~~~~~
├─ java.util.ArrayList instance
│    ↓ ArrayList[1]
│               ~~~
├─ com.datadog.android.rum.internal.domain.scope.RumViewScope instance
│    ↓ RumViewScope.keyRef
│                   ~~~~~~
├─ java.lang.ref.WeakReference instance
│    referent instance of com.squareup.featureflags.devdrawer.LdFeaturesOverrideActivity with mDestroyed = true
│    ↓ Reference.referent
│                ~~~~~~~~
╰→ com.squareup.featureflags.devdrawer.LdFeaturesOverrideActivity instance
     Leaking: YES (Activity#mDestroyed is true)
     Retaining 690.4 kB in 14512 objects

Root Cause

The core bug is in RumViewScope#onStopView (source) :

    private fun onStopView(
        event: RumRawEvent.StopView,
        writer: DataWriter<Any>
    ) {
        delegateEventToChildren(event, writer)
        val startedKey = keyRef.get()

Every time a RumRawEvent.StopView event is sent, all RumViewScope instances that are children of RumViewManagerScope will invoke onStopView() and call keyRef.get() to do an equals check to see if the stop view is for themselves. Unfortunately this has the side effect of creating a strong local reference to the activity, preventing it from being garbage collected in the next GC run.

The short term fix is simple: check if stopped is false before invoking val startedKey = keyRef.get() and immediately returning if stopped is true.

Now, of course, one might wonder: "why would we send RumRawEvent.StopView events to a RumViewScope associated to a view that's already stopped, shouldn't it be removed from its RumViewManagerScope parent?

A RumViewScope is removed from its RumViewManagerScope parent when the RumViewScope returns null in handleEvent, which happens only if isViewComplete() returns true. I have a heap dump showing a RumViewScope instance where stopped is true, activeResourceScopes is empty but pendingResourceCount = 2 (and the other pending fields counts set to 0), which makes isViewComplete() return false. I'm not sure exactly how this happens. DatadogRumMonitor.executorService has an empty queue so this isn't about pending events.

Unfortunately, I'm only able to look at heap dumps long after the issue has happened. I have tried to reproduce locally and debug through it, and I didn't manage to reproduce the exact scenario above, however I did get a leak when I went offline and the number of events increased so much (more errors) that the single thread executor in DatadogRumMonitor.executorService was lagging behind and its queue was filling up, so much that the StopView event took a really long time to get delivered and in the meantime we kept leaking the activity by calling get() on the weak reference.

Here's an example heap dump where I found this issue. I stripped all primitive arrays of their data and filled them with 0s to avoid sharing any secrets.

2023-12-13_12-20-26_265-e767bc81-376b-47e6-acc9-92a1a41eb297-stripped.hprof.zip

Proper fixes

The current implementation around keys is strange. The key has a type of Any, the framework holds a weak reference to it but calls get() often, then makes an equals check against the event key. This implies a requirement that the keys implement equals properly. That requirement most definitely doesn't work for activities and fragments, as it's very possible that developers override equals for their own need, leading to unexpected behavior on the datadog side.

On top of that, events are delivered async through a single threaded executor service. So events are always late. Sometimes they're a little bit late, but if the executor queue fills up they can be very late. When they're very late, they're held in memory for longer, and the event key is as well. So it's really not appropriate for the keys to be activities and fragments, they should instead be small objects that represent an identity but that we don't mind keeping in memory for longer.