Fix memory leak if request is started on detached view. (#518)

* Fix memory leak if request is started on detached view.

* Fix tests.

* Docs.

* Add regression test.

coil-base/src/androidTest/AndroidManifest.xml

@@ -10,7 +10,7 @@
 
     <application android:usesCleartextTraffic="true">
         <activity
-            android:name="coil.fetch.ResourceUriFetcherTest$TestActivity"
+            android:name="coil.util.TestActivity"
             android:theme="@style/Theme.TestActivity"/>
 
         <provider

coil-base/src/androidTest/java/coil/EventListenerTest.kt

@@ -3,8 +3,9 @@ package coil
 import android.content.ContentResolver.SCHEME_ANDROID_RESOURCE
 import android.content.Context
 import android.graphics.Bitmap
-import android.widget.ImageView
+import androidx.lifecycle.Lifecycle
 import androidx.test.core.app.ApplicationProvider
+import androidx.test.ext.junit.rules.activityScenarioRule
 import coil.annotation.ExperimentalCoilApi
 import coil.base.test.R
 import coil.bitmap.BitmapPool
@@ -21,10 +22,13 @@ import coil.size.Size
 import coil.transform.Transformation
 import coil.transition.Transition
 import coil.transition.TransitionTarget
+import coil.util.TestActivity
+import coil.util.activity
 import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.suspendCancellableCoroutine
 import org.junit.Before
+import org.junit.Rule
 import org.junit.Test
 import java.util.concurrent.atomic.AtomicInteger
 import kotlin.coroutines.resume
@@ -36,9 +40,13 @@ class EventListenerTest {
 
     private lateinit var context: Context
 
+    @get:Rule
+    val activityRule = activityScenarioRule<TestActivity>()
+
     @Before
     fun before() {
         context = ApplicationProvider.getApplicationContext()
+        activityRule.scenario.moveToState(Lifecycle.State.RESUMED)
     }
 
     @Test
@@ -202,7 +210,7 @@ class EventListenerTest {
     ) = suspendCancellableCoroutine<Unit> { continuation ->
         val request = ImageRequest.Builder(context)
             .size(100, 100)
-            .target(ImageView(context))
+            .target(activityRule.scenario.activity.imageView)
             .listener(
                 onSuccess = { _, _ -> continuation.resume(Unit) },
                 onError = { _, throwable -> continuation.resumeWithException(throwable) },

coil-base/src/androidTest/java/coil/RealImageLoaderTest.kt

@@ -12,7 +12,9 @@ import android.graphics.drawable.BitmapDrawable
 import android.graphics.drawable.ColorDrawable
 import android.widget.ImageView
 import androidx.core.net.toUri
+import androidx.lifecycle.Lifecycle
 import androidx.test.core.app.ApplicationProvider
+import androidx.test.ext.junit.rules.activityScenarioRule
 import coil.annotation.ExperimentalCoilApi
 import coil.base.test.R
 import coil.bitmap.BitmapPool
@@ -34,7 +36,9 @@ import coil.request.SuccessResult
 import coil.size.PixelSize
 import coil.size.Precision
 import coil.size.Size
+import coil.util.TestActivity
 import coil.util.Utils
+import coil.util.activity
 import coil.util.createMockWebServer
 import coil.util.decodeBitmapAsset
 import coil.util.getDrawableCompat
@@ -53,6 +57,7 @@ import okio.sink
 import okio.source
 import org.junit.After
 import org.junit.Before
+import org.junit.Rule
 import org.junit.Test
 import java.io.File
 import kotlin.coroutines.resume
@@ -71,6 +76,9 @@ class RealImageLoaderTest {
     private lateinit var strongMemoryCache: StrongMemoryCache
     private lateinit var imageLoader: RealImageLoader
 
+    @get:Rule
+    val activityRule = activityScenarioRule<TestActivity>()
+
     @Before
     fun before() {
         context = ApplicationProvider.getApplicationContext()
@@ -93,6 +101,7 @@ class RealImageLoaderTest {
             launchInterceptorChainOnMainThread = true,
             logger = null
         )
+        activityRule.scenario.moveToState(Lifecycle.State.RESUMED)
     }
 
     @After
@@ -421,7 +430,7 @@ class RealImageLoaderTest {
     }
 
     private fun testEnqueue(data: Any, expectedSize: PixelSize = PixelSize(80, 100)) {
-        val imageView = ImageView(context)
+        val imageView = activityRule.scenario.activity.imageView
         imageView.scaleType = ImageView.ScaleType.FIT_CENTER
 
         assertNull(imageView.drawable)

coil-base/src/androidTest/java/coil/fetch/ResourceUriFetcherTest.kt

@@ -4,12 +4,9 @@ import android.content.ContentResolver.SCHEME_ANDROID_RESOURCE
 import android.content.Context
 import android.graphics.drawable.BitmapDrawable
 import android.os.Build.VERSION.SDK_INT
-import androidx.appcompat.app.AppCompatActivity
 import androidx.core.graphics.drawable.toBitmap
 import androidx.core.net.toUri
-import androidx.lifecycle.Lifecycle
 import androidx.test.core.app.ApplicationProvider
-import androidx.test.core.app.launchActivity
 import coil.base.test.R
 import coil.bitmap.BitmapPool
 import coil.decode.DrawableDecoderService
@@ -20,6 +17,7 @@ import coil.size.PixelSize
 import coil.util.createOptions
 import coil.util.getDrawableCompat
 import coil.util.isSimilarTo
+import coil.util.withTestActivity
 import kotlinx.coroutines.runBlocking
 import org.junit.Assume.assumeTrue
 import org.junit.Before
@@ -113,21 +111,13 @@ class ResourceUriFetcherTest {
 
     /** Regression test: https://github.com/coil-kt/coil/issues/469 */
     @Test
-    fun colorAttributeIsApplied() {
-        val scenario = launchActivity<TestActivity>()
-        scenario.moveToState(Lifecycle.State.RESUMED)
-        scenario.onActivity { activity ->
-            runBlocking {
-                val result = runBlocking {
-                    val uri = ResourceIntMapper(context).map(R.drawable.ic_tinted_vector)
-                    fetcher.fetch(pool, uri, OriginalSize, createOptions(activity))
-                }
-                val expected = activity.getDrawableCompat(R.drawable.ic_tinted_vector).toBitmap()
-                val actual = (result as DrawableResult).drawable.toBitmap()
-                assertTrue(actual.isSimilarTo(expected))
-            }
+    fun colorAttributeIsApplied() = withTestActivity { activity ->
+        val result = runBlocking {
+            val uri = ResourceIntMapper(context).map(R.drawable.ic_tinted_vector)
+            fetcher.fetch(pool, uri, OriginalSize, createOptions(activity))
         }
+        val expected = activity.getDrawableCompat(R.drawable.ic_tinted_vector).toBitmap()
+        val actual = (result as DrawableResult).drawable.toBitmap()
+        assertTrue(actual.isSimilarTo(expected))
     }
-
-    class TestActivity : AppCompatActivity()
 }

coil-base/src/androidTest/java/coil/request/DisposableTest.kt

@@ -5,14 +5,19 @@ import android.content.Context
 import android.graphics.Bitmap
 import android.graphics.drawable.Drawable
 import android.widget.ImageView
+import androidx.lifecycle.Lifecycle
 import androidx.test.core.app.ApplicationProvider
+import androidx.test.ext.junit.rules.activityScenarioRule
 import coil.ImageLoader
 import coil.annotation.ExperimentalCoilApi
 import coil.bitmap.BitmapPool
 import coil.fetch.AssetUriFetcher.Companion.ASSET_FILE_PATH_ROOT
 import coil.size.Size
 import coil.transform.Transformation
 import coil.util.CoilUtils
+import coil.util.TestActivity
+import coil.util.activity
+import coil.util.isAttachedToWindowCompat
 import coil.util.requestManager
 import coil.util.runBlockingTest
 import kotlinx.coroutines.ExperimentalCoroutinesApi
@@ -21,6 +26,7 @@ import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.first
 import org.junit.After
 import org.junit.Before
+import org.junit.Rule
 import org.junit.Test
 import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
@@ -33,12 +39,16 @@ class DisposableTest {
     private lateinit var context: Context
     private lateinit var imageLoader: ImageLoader
 
+    @get:Rule
+    val activityRule = activityScenarioRule<TestActivity>()
+
     @Before
     fun before() {
         context = ApplicationProvider.getApplicationContext()
         imageLoader = ImageLoader.Builder(context)
             .memoryCachePolicy(CachePolicy.DISABLED)
             .build()
+        activityRule.scenario.moveToState(Lifecycle.State.RESUMED)
     }
 
     @After
@@ -83,7 +93,7 @@ class DisposableTest {
 
     @Test
     fun viewTargetDisposable_dispose() = runBlockingTest {
-        val imageView = ImageView(context)
+        val imageView = activityRule.scenario.activity.imageView
         val request = ImageRequest.Builder(context)
             .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg")
             // Set a fixed size so we don't suspend indefinitely waiting for the view to be measured.
@@ -102,7 +112,7 @@ class DisposableTest {
     @Test
     fun viewTargetDisposable_await() = runBlockingTest {
         val transformation = GateTransformation()
-        val imageView = ImageView(context)
+        val imageView = activityRule.scenario.activity.imageView
         val request = ImageRequest.Builder(context)
             .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg")
             // Set a fixed size so we don't suspend indefinitely waiting for the view to be measured.
@@ -122,7 +132,7 @@ class DisposableTest {
     @Test
     fun viewTargetDisposable_restart() = runBlockingTest {
         val transformation = GateTransformation()
-        val imageView = ImageView(context)
+        val imageView = activityRule.scenario.activity.imageView
         val request = ImageRequest.Builder(context)
             .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg")
             // Set a fixed size so we don't suspend indefinitely waiting for the view to be measured.
@@ -151,7 +161,7 @@ class DisposableTest {
 
     @Test
     fun viewTargetDisposable_replace() = runBlockingTest {
-        val imageView = ImageView(context)
+        val imageView = activityRule.scenario.activity.imageView
 
         fun launchNewRequest(): Disposable {
             val request = ImageRequest.Builder(context)
@@ -178,7 +188,7 @@ class DisposableTest {
 
     @Test
     fun viewTargetDisposable_clear() = runBlockingTest {
-        val imageView = ImageView(context)
+        val imageView = activityRule.scenario.activity.imageView
         val request = ImageRequest.Builder(context)
             .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg")
             // Set a fixed size so we don't suspend indefinitely waiting for the view to be measured.
@@ -193,6 +203,24 @@ class DisposableTest {
         assertTrue(disposable.isDisposed)
     }
 
+    @Test
+    fun viewTargetDisposable_detachedViewIsImmediatelyCancelled() = runBlockingTest {
+        val imageView = ImageView(context)
+
+        assertFalse(imageView.isAttachedToWindowCompat)
+
+        val request = ImageRequest.Builder(context)
+            .data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/normal.jpg")
+            // Set a fixed size so we don't suspend indefinitely waiting for the view to be measured.
+            .size(100, 100)
+            .target(imageView)
+            .build()
+        val disposable = imageLoader.enqueue(request)
+
+        assertFalse(disposable.isDisposed)
+        assertTrue(imageView.requestManager.currentRequestJob!!.isCancelled)
+    }
+
     /**
      * Prevent completing the [ImageRequest] until [open] is called.
      * This is to avoid our test assertions racing the image request.
@@ -201,7 +229,7 @@ class DisposableTest {
 
         private val isOpen = MutableStateFlow(false)
 
-        override fun key() = GateTransformation::class.java.name
+        override fun key(): String = GateTransformation::class.java.name
 
         override suspend fun transform(pool: BitmapPool, input: Bitmap, size: Size): Bitmap {
             // Suspend until the gate is open.

coil-base/src/androidTest/java/coil/util/AndroidTestFunctions.kt

@@ -0,0 +1,34 @@
+package coil.util
+
+import android.app.Activity
+import androidx.lifecycle.Lifecycle
+import androidx.test.core.app.ActivityScenario
+import androidx.test.core.app.launchActivity
+import androidx.test.ext.junit.rules.ActivityScenarioRule
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.runBlocking
+
+/** Launch [TestActivity] and invoke [block]. */
+fun withTestActivity(block: (TestActivity) -> Unit) {
+    launchActivity<TestActivity>().use { scenario ->
+        scenario.moveToState(Lifecycle.State.RESUMED)
+        scenario.onActivity(block)
+    }
+}
+
+/**
+ * Get a reference to the [ActivityScenario]'s [Activity].
+ *
+ * NOTE: [ActivityScenario.onActivity] explicitly recommends against holding a
+ * reference to the [Activity] outside of its scope. However, it should be safe
+ * as long we use [ActivityScenarioRule].
+ */
+val <T : Activity> ActivityScenario<T>.activity: T
+    get() {
+        lateinit var activity: T
+        runBlocking(Dispatchers.Main.immediate) {
+            // onActivity is executed synchronously when called from the main thread.
+            onActivity { activity = it }
+        }
+        return activity
+    }

coil-base/src/androidTest/java/coil/util/TestActivity.kt

@@ -0,0 +1,10 @@
+package coil.util
+
+import android.widget.ImageView
+import androidx.appcompat.app.AppCompatActivity
+import coil.base.test.R
+
+class TestActivity : AppCompatActivity(R.layout.ic_test_activity) {
+
+    val imageView: ImageView by lazy { findViewById(R.id.image) }
+}

coil-base/src/androidTest/res/drawable/ic_tinted_vector.xml

@@ -7,6 +7,6 @@
     android:tint="?colorPrimary"
     android:tintMode="src_in">
     <path
-        android:fillColor="#00FF00"
+        android:fillColor="#FF0000"
         android:pathData="M21,19V5c0,-1.1 -0.9,-2 -2,-2H5c-1.1,0 -2,0.9 -2,2v14c0,1.1 0.9,2 2,2h14c1.1,0 2,-0.9 2,-2zM8.5,13.5l2.5,3.01L14.5,12l4.5,6H5l3.5,-4.5z"/>
 </vector>

coil-base/src/androidTest/res/layout/ic_test_activity.xml

@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="utf-8"?>
+<FrameLayout
+    xmlns:android="http://schemas.android.com/apk/res/android"
+    android:layout_width="match_parent"
+    android:layout_height="match_parent">
+
+    <ImageView
+        android:id="@+id/image"
+        android:layout_width="100px"
+        android:layout_height="100px"
+        android:layout_gravity="center" />
+</FrameLayout>

coil-base/src/androidTest/res/values/styles.xml

@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <resources>
     <style name="Theme.TestActivity" parent="@style/Theme.MaterialComponents.Light">
-        <item name="colorPrimary">#FF0000</item>
+        <item name="colorPrimary">#FFFFFF</item>
     </style>
 </resources>

coil-base/src/main/java/coil/memory/DelegateService.kt

@@ -14,6 +14,7 @@ import coil.target.ViewTarget
 import coil.util.Logger
 import coil.util.Utils.REQUEST_TYPE_ENQUEUE
 import coil.util.Utils.REQUEST_TYPE_EXECUTE
+import coil.util.isAttachedToWindowCompat
 import coil.util.requestManager
 import kotlinx.coroutines.Job
 
@@ -55,12 +56,17 @@ internal class DelegateService(
     ): RequestDelegate {
         val lifecycle = request.lifecycle
         val delegate: RequestDelegate
-        when (request.target) {
+        when (val target = request.target) {
             is ViewTarget<*> -> {
                 delegate = ViewTargetRequestDelegate(imageLoader, request, targetDelegate, job)
                 lifecycle.addObserver(delegate)
-                if (request.target is LifecycleObserver) lifecycle.addObserver(request.target)
-                request.target.view.requestManager.setCurrentRequest(delegate)
+                if (target is LifecycleObserver) lifecycle.addObserver(target)
+                target.view.requestManager.setCurrentRequest(delegate)
+
+                // Call onViewDetachedFromWindow immediately if the view is already detached.
+                if (!target.view.isAttachedToWindowCompat) {
+                    target.view.requestManager.onViewDetachedFromWindow(target.view)
+                }
             }
             else -> {
                 delegate = BaseRequestDelegate(lifecycle, job)

coil-base/src/test/resources/robolectric.properties

@@ -1 +1 @@
-sdk=29
+sdk=28

coil-singleton/src/test/resources/robolectric.properties

@@ -1 +1 @@
-sdk=29
+sdk=28