[ISSUE-55] Fix memory leaks adding weak reference to continuation

[gerzalez-x3m]

This fix resolves the issue described in Issue 55

[chauduyphanvu]

Thanks for the investigation and the fix. Taking a look.

[gerzalez-x3m]

I forgot to add the resumeOnce logic in the callbacks. I added in the last commit.

[chauduyphanvu]

I took a look but I couldn't reproduce the original leak you reported. Regardless, from looking at your proposed fix, since it's still using an anonymous inner class (by returning object : FullScreenContentCallback() from the new methods) it'll still hold an implicit reference to the hosting Activity. I'm not sure if that'll address the potential leaks.

Why not use static inner classes or top-level classes?

[gerzalez-x3m]

Hello Chauduyphanvu! Thank you for your prompt response to the pull request.

Regardless of how the callback is created, We've identified that the crucial element lies in the application of a weak reference to the continuation object of suspendCancellableCoroutine. This design enables the object to be reclaimed by the GC when it becomes inaccessible.

In practice, this approach has proven effective, addressing the memory leak described in the reported issue. Feel free to reach out if you have any questions or if further clarification is needed.

Best regards, German.

On Tue, Jan 9, 2024 at 7:47 PM chauduyphanvu @.***> wrote:

I took a look but I couldn't reproduce the original leak you reported. Regardless, from looking at your proposed fix, since it's still using an anonymous inner class (by returning object : FullScreenContentCallback() from the new methods) it'll still hold an implicit reference to the hosting Activity. I'm not sure if that'll address the potential leaks.

Why not use static inner classes or top-level classes?

— Reply to this email directly, view it on GitHub https://github.com/ChartBoost/chartboost-mediation-android-adapter-google-bidding/pull/56#issuecomment-1883919685, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2IEJEEK3QSYZ3FBWOHPWS3YNXCHZAVCNFSM6AAAAABBR5ITHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBTHEYTSNRYGU . You are receiving this because you authored the thread.Message ID: <ChartBoost/chartboost-mediation-android-adapter-google-bidding/pull/56/c1883919685 @github.com>

[chauduyphanvu]

@bichenwang, repro is in the linked issue. I repro'd it exactly once but not afterwards so I'm not 100% confident in both the issue and the proposed fix. On paper it can happen though given how we're using anonymous inner class to register callbacks. In edge cases when the hosting Activity is gone we could have memory leaks.

[ypalav-x3m]

Hi @chauduyphanvu @bichenwang!

I've created a sample project so that you can reproduce the leak more easily: https://github.com/ypalav-x3m/chartboostmediation-admob-adapter-leak-test

Just to be throughout, the leak trace is this one:

  ┬───
  │ GC Root: Global variable in native code
  │
  ├─ com.google.android.gms.dynamic.ObjectWrapper instance
  │    Leaking: UNKNOWN
  │    Retaining 528 B in 1 objects
  │    ↓ ObjectWrapper.a
  │                    ~
  ├─ com.google.android.gms.ads.nonagon.ad.event.o instance
  │    Leaking: UNKNOWN
  │    Retaining 473 B in 14 objects
  │    ↓ dz.b
  │         ~
  ├─ java.util.HashMap instance
  │    Leaking: UNKNOWN
  │    Retaining 388 B in 12 objects
  │    ↓ HashMap[key()]
  │             ~~~~~~~
  ├─ com.google.android.gms.ads.nonagon.ad.event.dz instance
  │    Leaking: UNKNOWN
  │    Retaining 236 B in 8 objects
  │    ↓ dz.b
  │         ~
  ├─ java.util.HashMap instance
  │    Leaking: UNKNOWN
  │    Retaining 224 B in 7 objects
  │    ↓ HashMap[key()]
  │             ~~~~~~~
  ├─ com.google.android.gms.ads.nonagon.shim.z instance
  │    Leaking: UNKNOWN
  │    Retaining 433.8 kB in 3658 objects
  │    ↓ z.b
  │        ~
  ├─ java.util.concurrent.atomic.AtomicReference instance
  │    Leaking: UNKNOWN
  │    Retaining 556 B in 3 objects
  │    ↓ AtomicReference.value
  │                      ~~~~~
  ├─ com.google.android.gms.ads.internal.client.bq instance
  │    Leaking: UNKNOWN
  │    Retaining 544 B in 2 objects
  │    ↓ ajj.a
  │          ~
  ├─ com.google.android.gms.ads.internal.client.zzbb instance
  │    Leaking: UNKNOWN
  │    Retaining 528 B in 1 objects
  │    ↓ zzbb.zza
  │           ~~~
  ├─ com.chartboost.mediation.googlebiddingadapter.GoogleBiddingAdapter$showInterstitialAd$2$1$1$1 instance
  │    Leaking: UNKNOWN
  │    Retaining 214.2 kB in 2344 objects
  │    Anonymous subclass of com.google.android.gms.ads.FullScreenContentCallback
  │    ↓ GoogleBiddingAdapter$showInterstitialAd$2$1$1$1.$continuation
  │                                                      ~~~~~~~~~~~~~
  ├─ kotlinx.coroutines.CancellableContinuationImpl instance
  │    Leaking: UNKNOWN
  │    Retaining 214.2 kB in 2342 objects
  │    ↓ CancellableContinuationImpl.delegate
  │                                  ~~~~~~~~
  ├─ kotlinx.coroutines.internal.DispatchedContinuation instance
  │    Leaking: UNKNOWN
  │    Retaining 204 B in 5 objects
  │    ↓ DispatchedContinuation.continuation
  │                             ~~~~~~~~~~~~
  ├─ com.chartboost.mediation.googlebiddingadapter.GoogleBiddingAdapter$showInterstitialAd$1 instance
  │    Leaking: UNKNOWN
  │    Retaining 156 B in 4 objects
  │    Anonymous subclass of kotlin.coroutines.jvm.internal.ContinuationImpl
  │    L$1 instance of com.x3mads.test.chartboostmediationtestapp.InterstitialActivity with mDestroyed = true
  │    ↓ GoogleBiddingAdapter$showInterstitialAd$1.L$1
  │                                                ~~~
  ╰→ com.x3mads.test.chartboostmediationtestapp.InterstitialActivity instance
  ​     Leaking: YES (ObjectWatcher was watching this because com.x3mads.test.chartboostmediationtestapp.
  ​     InterstitialActivity received Activity#onDestroy() callback and Activity#mDestroyed is true)
  ​     Retaining 213.3 kB in 2312 objects
  ​     key = 7590d7a8-cc01-47f4-9a84-90dbb3ec6051
  ​     watchDurationMillis = 7252
  ​     retainedDurationMillis = 2252
  ​     mApplication instance of android.app.Application
  ​     mBase instance of androidx.appcompat.view.ContextThemeWrapper

You can reproduce it by following these steps:

1. Initialize the Helium SDK on the Main Activity
2. Navigate to the Interstitial Activity
3. Load, Show and Dismiss an AdMob interstitial using the Helium SDK
4. Go back to the Main Activity, so that the Interstitial Activity gets destroyed

By that point you can run the Android Studio Profiler or use the included Leak Canary library to dump the heap and check for any leaks; you should get the same leak trace as the one above.

Why does the leak happen?

1. First, the Google Mobile Ads SDK is retaining the com.google.android.gms.ads.FullScreenContentCallback for more than it should; but this seems to be out of our control
2. The FullScreenContentCallback in turn leaks the continuation callback of the suspendCancellableCoroutine function, used to resume the coroutine after the ad gets dismissed
3. And finally, since the continuation callback was built in a lexical scope where there was a strong reference to the activity (the context parameter of the showInterstitialAd method, used also to call the .show method), it ends up leaking the destroyed Activity that was used to show the ad

The solution we found then, is to wrap the continuation callback with a WeakReference, so that it gets retained only while the coroutine is expecting a response from it. However, feel free to experiment with any alternative solutions you might find.

I hope this explanation helped!

[chauduyphanvu]

@ypalav-x3m, thanks for the repro. We've checked it out and will prepare a fix from https://github.com/ChartBoost/chartboost-mediation-android-adapter-google-bidding/pull/59.