Tests that we don't leak ContentSources (#2054)

* Tests that we don't leak ContentSources

This is another one that wanted some small internal changes
to our observability APIs.

* apiDump

* Track API changes in CodeListener

redwood-treehouse-host/api/android/redwood-treehouse-host.api

@@ -5,9 +5,9 @@ public final class app/cash/redwood/treehouse/ChangeListRenderer {
 
 public class app/cash/redwood/treehouse/CodeListener {
 	public fun <init> ()V
+	public fun onCodeDetached (Lapp/cash/redwood/treehouse/TreehouseApp;Lapp/cash/redwood/treehouse/TreehouseView;Ljava/lang/Throwable;)V
 	public fun onCodeLoaded (Lapp/cash/redwood/treehouse/TreehouseApp;Lapp/cash/redwood/treehouse/TreehouseView;Z)V
 	public fun onInitialCodeLoading (Lapp/cash/redwood/treehouse/TreehouseApp;Lapp/cash/redwood/treehouse/TreehouseView;)V
-	public fun onUncaughtException (Lapp/cash/redwood/treehouse/TreehouseApp;Lapp/cash/redwood/treehouse/TreehouseView;Ljava/lang/Throwable;)V
 }
 
 public abstract interface class app/cash/redwood/treehouse/Content {

redwood-treehouse-host/api/jvm/redwood-treehouse-host.api

@@ -5,9 +5,9 @@ public final class app/cash/redwood/treehouse/ChangeListRenderer {
 
 public class app/cash/redwood/treehouse/CodeListener {
 	public fun <init> ()V
+	public fun onCodeDetached (Lapp/cash/redwood/treehouse/TreehouseApp;Lapp/cash/redwood/treehouse/TreehouseView;Ljava/lang/Throwable;)V
 	public fun onCodeLoaded (Lapp/cash/redwood/treehouse/TreehouseApp;Lapp/cash/redwood/treehouse/TreehouseView;Z)V
 	public fun onInitialCodeLoading (Lapp/cash/redwood/treehouse/TreehouseApp;Lapp/cash/redwood/treehouse/TreehouseView;)V
-	public fun onUncaughtException (Lapp/cash/redwood/treehouse/TreehouseApp;Lapp/cash/redwood/treehouse/TreehouseView;Ljava/lang/Throwable;)V
 }
 
 public abstract interface class app/cash/redwood/treehouse/Content {

redwood-treehouse-host/api/redwood-treehouse-host.klib.api

@@ -99,9 +99,9 @@ final fun <#A: app.cash.redwood.treehouse/AppService, #B: kotlin/Any> (app.cash.
 final fun <#A: kotlin/Any> (app.cash.redwood.treehouse/Content).app.cash.redwood.treehouse/bindWhenReady(app.cash.redwood.treehouse/TreehouseView<#A>): okio/Closeable // app.cash.redwood.treehouse/bindWhenReady|[email protected](app.cash.redwood.treehouse.TreehouseView<0:0>){0§<kotlin.Any>}[0]
 open class app.cash.redwood.treehouse/CodeListener { // app.cash.redwood.treehouse/CodeListener|null[0]
     constructor <init>() // app.cash.redwood.treehouse/CodeListener.<init>|<init>(){}[0]
+    open fun onCodeDetached(app.cash.redwood.treehouse/TreehouseApp<*>, app.cash.redwood.treehouse/TreehouseView<*>, kotlin/Throwable?) // app.cash.redwood.treehouse/CodeListener.onCodeDetached|onCodeDetached(app.cash.redwood.treehouse.TreehouseApp<*>;app.cash.redwood.treehouse.TreehouseView<*>;kotlin.Throwable?){}[0]
     open fun onCodeLoaded(app.cash.redwood.treehouse/TreehouseApp<*>, app.cash.redwood.treehouse/TreehouseView<*>, kotlin/Boolean) // app.cash.redwood.treehouse/CodeListener.onCodeLoaded|onCodeLoaded(app.cash.redwood.treehouse.TreehouseApp<*>;app.cash.redwood.treehouse.TreehouseView<*>;kotlin.Boolean){}[0]
     open fun onInitialCodeLoading(app.cash.redwood.treehouse/TreehouseApp<*>, app.cash.redwood.treehouse/TreehouseView<*>) // app.cash.redwood.treehouse/CodeListener.onInitialCodeLoading|onInitialCodeLoading(app.cash.redwood.treehouse.TreehouseApp<*>;app.cash.redwood.treehouse.TreehouseView<*>){}[0]
-    open fun onUncaughtException(app.cash.redwood.treehouse/TreehouseApp<*>, app.cash.redwood.treehouse/TreehouseView<*>, kotlin/Throwable) // app.cash.redwood.treehouse/CodeListener.onUncaughtException|onUncaughtException(app.cash.redwood.treehouse.TreehouseApp<*>;app.cash.redwood.treehouse.TreehouseView<*>;kotlin.Throwable){}[0]
 }
 open class app.cash.redwood.treehouse/EventListener { // app.cash.redwood.treehouse/EventListener|null[0]
     abstract fun interface Factory { // app.cash.redwood.treehouse/EventListener.Factory|null[0]

redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/LeaksTest.kt

@@ -22,7 +22,9 @@ import assertk.assertions.isEqualTo
 import assertk.assertions.isNotNull
 import com.example.redwood.testapp.testing.TextInputValue
 import kotlin.test.Test
+import kotlinx.coroutines.coroutineScope
 import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.launch
 import kotlinx.coroutines.test.runTest
 
 class LeaksTest {
@@ -121,4 +123,48 @@ class LeaksTest {
     treehouseApp.close()
     eventListenerLeakWatcher.assertNotLeaked()
   }
+
+  @Test
+  fun contentSourceNotLeaked() = runTest {
+    val tester = TreehouseTester(this)
+    val treehouseApp = tester.loadApp()
+    val view = tester.view()
+
+    // Use an inline run() to try to prevent contentSource from being retained on the stack.
+    val (content, contentSourceLeakWatcher) = run {
+      val contentSource = RetainEverythingTreehouseContentSource()
+
+      val content = treehouseApp.createContent(
+        source = contentSource,
+        codeListener = FakeCodeListener(tester.eventLog),
+      )
+
+      return@run content to LeakWatcher { contentSource }
+    }
+
+    // After we bind the content, it'll be in a retain cycle.
+    content.bind(view)
+    tester.eventLog.takeEvent("onCodeLoaded(test_app, view, initial = true)", skipOthers = true)
+    contentSourceLeakWatcher.assertObjectInReferenceCycle()
+
+    // Unbind it, and it's no longer retained.
+    content.unbind()
+    tester.eventLog.takeEvent("onCodeDetached(test_app, view, null)", skipOthers = true)
+    treehouseApp.dispatchers.awaitLaunchedTasks()
+    contentSourceLeakWatcher.assertNotLeaked()
+
+    treehouseApp.stop()
+  }
+
+  /**
+   * This is unfortunate. Some cleanup functions launch jobs on another dispatcher and we don't have
+   * a natural way to wait for those jobs to complete. So we launch empty jobs on each dispatcher,
+   * and trust that they won't start until existing queued jobs finish.
+   */
+  private suspend fun TreehouseDispatchers.awaitLaunchedTasks() {
+    coroutineScope {
+      launch(zipline) {}.join()
+      launch(ui) {}.join()
+    }
+  }
 }

redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/RetainEverythingEventListenerFactory.kt

@@ -18,7 +18,10 @@ package app.cash.redwood.treehouse
 import app.cash.zipline.Zipline
 import app.cash.zipline.ZiplineManifest
 
-/** An event listener that keeps a reference to everything it sees, for defensive leak testing. */
+/**
+ * An event listener (and factory) that keeps a reference to everything it sees, for defensive leak
+ * testing.
+ */
 class RetainEverythingEventListenerFactory(
   private val eventLog: EventLog,
 ) : EventListener(), EventListener.Factory {

redwood-treehouse-host/src/appsJvmTest/kotlin/app/cash/redwood/treehouse/RetainEverythingTreehouseContentSource.kt

@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2024 Square, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package app.cash.redwood.treehouse
+
+import com.example.redwood.testapp.treehouse.TestAppPresenter
+
+/** A content source that keeps a reference to everything it sees, for defensive leak testing. */
+class RetainEverythingTreehouseContentSource : TreehouseContentSource<TestAppPresenter> {
+  private var app: TestAppPresenter? = null
+
+  override fun get(app: TestAppPresenter): ZiplineTreehouseUi {
+    this.app = app
+    return app.launchForTester()
+  }
+}

redwood-treehouse-host/src/commonMain/kotlin/app/cash/redwood/treehouse/CodeEventPublisher.kt

@@ -25,8 +25,8 @@ internal interface CodeEventPublisher {
     initial: Boolean,
   )
 
-  fun onUncaughtException(
+  fun onCodeDetached(
     view: TreehouseView<*>,
-    exception: Throwable,
+    exception: Throwable?,
   )
 }

redwood-treehouse-host/src/commonMain/kotlin/app/cash/redwood/treehouse/CodeListener.kt

@@ -43,22 +43,25 @@ public open class CodeListener {
   }
 
   /**
-   * Invoked when the application powering [view] fails with an uncaught exception. This function
-   * should display an error UI.
+   * Invoked when the application powering [view] stops sending updates. This is triggered by:
    *
-   * Typical implementations call [TreehouseView.reset] and display an error placeholder.
-   * Development builds may show more diagnostic information than production builds.
+   *  * the UI no longer needing code to drive it, perhaps because it's detached from the screen
+   *  * the code being hot-reloaded
+   *  * the code failing with an exception
+   *
+   * If it is failing due to a failure, [exception] will be non-null and this function should
+   * display an error UI. Typical implementations call [TreehouseView.reset] and display an error
+   * placeholder. Development builds may show more diagnostic information than production builds.
    *
    * When a Treehouse app fails, its current Zipline instance is canceled so no further code will
    * execute. A new Zipline will start when new code available.
    *
-   * This condition is not permanent! If new code is loaded after an error, [onCodeLoaded] will be
-   * called.
+   * This condition is not permanent! If so, [onCodeLoaded] will be called.
    */
-  public open fun onUncaughtException(
+  public open fun onCodeDetached(
     app: TreehouseApp<*>,
     view: TreehouseView<*>,
-    exception: Throwable,
+    exception: Throwable?,
   ) {
   }
 }

redwood-treehouse-host/src/commonMain/kotlin/app/cash/redwood/treehouse/RealCodeEventPublisher.kt

@@ -27,7 +27,7 @@ internal class RealCodeEventPublisher(
     return codeListener.onCodeLoaded(app, view, initial)
   }
 
-  override fun onUncaughtException(view: TreehouseView<*>, exception: Throwable) {
-    return codeListener.onUncaughtException(app, view, exception)
+  override fun onCodeDetached(view: TreehouseView<*>, exception: Throwable?) {
+    return codeListener.onCodeDetached(app, view, exception)
   }
 }

redwood-treehouse-host/src/commonMain/kotlin/app/cash/redwood/treehouse/RealEventPublisher.kt

@@ -173,7 +173,8 @@ internal class RealEventPublisher(
     }
 
     override fun serviceLeaked(zipline: Zipline, name: String) {
-      listener!!.serviceLeaked(name)
+      // Drop serviceLeaked() calls made after close(). This can happen in practice.
+      listener?.serviceLeaked(name)
     }
 
     override fun ziplineClosed(zipline: Zipline) {

redwood-treehouse-host/src/commonMain/kotlin/app/cash/redwood/treehouse/TreehouseAppContent.kt

@@ -172,7 +172,7 @@ internal class TreehouseAppContent<A : AppService>(
     codeHost.removeListener(this)
     if (previousState.codeState is CodeState.Running) {
       val binding = previousState.codeState.viewContentCodeBinding
-      binding.cancel()
+      binding.cancel(null)
       binding.codeSession.removeListener(this)
     }
 
@@ -215,7 +215,7 @@ internal class TreehouseAppContent<A : AppService>(
     // If we replaced an old binding, cancel that old binding.
     if (previousCodeState is CodeState.Running) {
       val binding = previousCodeState.viewContentCodeBinding
-      binding.cancel()
+      binding.cancel(null)
       binding.codeSession.removeListener(this)
     }
 
@@ -246,15 +246,9 @@ internal class TreehouseAppContent<A : AppService>(
 
     // Cancel the UI binding to the canceled code.
     val binding = previousCodeState.viewContentCodeBinding
-    binding.cancel()
+    binding.cancel(exception)
     binding.codeSession.removeListener(this)
 
-    // If there's an error and a UI, show it.
-    val view = (viewState as? ViewState.Bound)?.view
-    if (exception != null && view != null) {
-      codeEventPublisher.onUncaughtException(view, exception)
-    }
-
     val nextCodeState = CodeState.Idle<A>(isInitialLaunch = false)
     stateFlow.value = State(viewState, nextCodeState)
   }
@@ -302,7 +296,7 @@ private class ViewContentCodeBinding<A : AppService>(
   val stateStore: StateStore,
   val dispatchers: TreehouseDispatchers,
   val eventPublisher: EventPublisher,
-  val contentSource: TreehouseContentSource<A>,
+  contentSource: TreehouseContentSource<A>,
   val codeEventPublisher: CodeEventPublisher,
   val stateFlow: MutableStateFlow<State<A>>,
   val codeSession: CodeSession<A>,
@@ -328,6 +322,9 @@ private class ViewContentCodeBinding<A : AppService>(
   /** Only accessed on [TreehouseDispatchers.zipline]. */
   private val serviceScope = codeSession.newServiceScope()
 
+  /** Only accessed on [TreehouseDispatchers.zipline]. Null after [cancel]. */
+  private var contentSource: TreehouseContentSource<A>? = contentSource
+
   /** Only accessed on [TreehouseDispatchers.zipline]. Null after [cancel]. */
   private var treehouseUiOrNull: ZiplineTreehouseUi? = null
 
@@ -435,7 +432,7 @@ private class ViewContentCodeBinding<A : AppService>(
   fun start() {
     bindingScope.launch(dispatchers.zipline) {
       val scopedAppService = serviceScope.apply(codeSession.appService)
-      val treehouseUi = contentSource.get(scopedAppService)
+      val treehouseUi = contentSource!!.get(scopedAppService)
       treehouseUiOrNull = treehouseUi
       eventBridge.delegate = treehouseUi
       stateSnapshot = viewOrNull?.stateSnapshotId?.let {
@@ -497,16 +494,21 @@ private class ViewContentCodeBinding<A : AppService>(
   }
 
   @OptIn(ExperimentalCoroutinesApi::class)
-  fun cancel() {
+  fun cancel(exception: Throwable?) {
     dispatchers.checkUi()
+
+    if (canceled) return
     canceled = true
+
+    viewOrNull?.let { codeEventPublisher.onCodeDetached(it, exception) }
     viewOrNull?.saveCallback = null
     viewOrNull = null
     bridgeOrNull?.close()
     bridgeOrNull = null
     eventBridge.bindingScope = null
     eventBridge.ziplineDispatcher = null
     bindingScope.launch(dispatchers.zipline, start = CoroutineStart.ATOMIC) {
+      contentSource = null
       treehouseUiOrNull = null
       eventBridge.delegate = null
       serviceScope.close()

redwood-treehouse-host/src/commonTest/kotlin/app/cash/redwood/treehouse/CodeHostTest.kt

@@ -65,6 +65,7 @@ class CodeHostTest {
 
     codeHost.stop()
     eventLog.takeEvent("codeHostUpdates1.close()")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, null)")
     eventLog.takeEvent("codeSessionA.app.uis[0].close()")
     eventLog.takeEvent("codeSessionA.stop()")
 
@@ -87,6 +88,7 @@ class CodeHostTest {
 
     codeHost.stop()
     eventLog.takeEvent("codeHostUpdates1.close()")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, null)")
     eventLog.takeEvent("codeSessionA.app.uis[0].close()")
     eventLog.takeEvent("codeSessionA.stop()")
 
@@ -108,6 +110,7 @@ class CodeHostTest {
     eventLog.takeEvent("codeSessionA.app.uis[0].start()")
     codeHost.stop()
     eventLog.takeEvent("codeHostUpdates1.close()")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, null)")
     eventLog.takeEvent("codeSessionA.app.uis[0].close()")
     eventLog.takeEvent("codeSessionA.stop()")
 
@@ -118,6 +121,7 @@ class CodeHostTest {
     eventLog.takeEvent("codeSessionB.app.uis[0].start()")
     codeHost.stop()
     eventLog.takeEvent("codeHostUpdates2.close()")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, null)")
     eventLog.takeEvent("codeSessionB.app.uis[0].close()")
     eventLog.takeEvent("codeSessionB.stop()")
 
@@ -138,7 +142,7 @@ class CodeHostTest {
     eventLog.takeEvent("codeSessionA.start()")
     eventLog.takeEvent("codeSessionA.app.uis[0].start()")
     codeSessionA.handleUncaughtException(Exception("boom!"))
-    eventLog.takeEvent("codeListener.onUncaughtException(view1, kotlin.Exception: boom!)")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, kotlin.Exception: boom!)")
     eventLog.takeEvent("codeSessionA.app.uis[0].close()")
     eventLog.takeEvent("codeSessionA.stop()")
 
@@ -150,6 +154,7 @@ class CodeHostTest {
     eventLog.takeEvent("codeSessionB.app.uis[0].start()")
     codeHost.stop()
     eventLog.takeEvent("codeHostUpdates2.close()")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, null)")
     eventLog.takeEvent("codeSessionB.app.uis[0].close()")
     eventLog.takeEvent("codeSessionB.stop()")
 
@@ -170,7 +175,7 @@ class CodeHostTest {
     eventLog.takeEvent("codeSessionA.start()")
     eventLog.takeEvent("codeSessionA.app.uis[0].start()")
     codeSessionA.handleUncaughtException(Exception("boom!"))
-    eventLog.takeEvent("codeListener.onUncaughtException(view1, kotlin.Exception: boom!)")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, kotlin.Exception: boom!)")
     eventLog.takeEvent("codeSessionA.app.uis[0].close()")
     eventLog.takeEvent("codeSessionA.stop()")
 
@@ -179,6 +184,7 @@ class CodeHostTest {
     eventLog.takeEvent("codeSessionB.app.uis[0].start()")
     codeHost.stop()
     eventLog.takeEvent("codeHostUpdates1.close()")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, null)")
     eventLog.takeEvent("codeSessionB.app.uis[0].close()")
     eventLog.takeEvent("codeSessionB.stop()")
 
@@ -199,7 +205,7 @@ class CodeHostTest {
     eventLog.takeEvent("codeSessionA.start()")
     eventLog.takeEvent("codeSessionA.app.uis[0].start()")
     codeSessionA.handleUncaughtException(Exception("boom!"))
-    eventLog.takeEvent("codeListener.onUncaughtException(view1, kotlin.Exception: boom!)")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, kotlin.Exception: boom!)")
     eventLog.takeEvent("codeSessionA.app.uis[0].close()")
     eventLog.takeEvent("codeSessionA.stop()")
 
@@ -229,6 +235,7 @@ class CodeHostTest {
     eventLog.takeEvent("codeSessionA.app.uis[0].start()")
     codeHost.stop()
     eventLog.takeEvent("codeHostUpdates1.close()")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, null)")
     eventLog.takeEvent("codeSessionA.app.uis[0].close()")
     eventLog.takeEvent("codeSessionA.stop()")
 
@@ -254,6 +261,7 @@ class CodeHostTest {
 
     codeHost.stop()
     eventLog.takeEvent("codeHostUpdates1.close()")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, null)")
     eventLog.takeEvent("codeSessionA.app.uis[0].close()")
     eventLog.takeEvent("codeSessionA.stop()")
 
@@ -276,6 +284,7 @@ class CodeHostTest {
 
     codeHost.stop()
     eventLog.takeEvent("codeHostUpdates1.close()")
+    eventLog.takeEvent("codeListener.onCodeDetached(view1, null)")
     eventLog.takeEvent("codeSessionA.app.uis[0].close()")
     eventLog.takeEvent("codeSessionA.stop()")
 

redwood-treehouse-host/src/commonTest/kotlin/app/cash/redwood/treehouse/FakeCodeEventPublisher.kt

@@ -26,9 +26,9 @@ class FakeCodeEventPublisher(
     eventLog += "codeListener.onCodeLoaded($view, initial = $initial)"
   }
 
-  override fun onUncaughtException(view: TreehouseView<*>, exception: Throwable) {
+  override fun onCodeDetached(view: TreehouseView<*>, exception: Throwable?) {
     // Canonicalize "java.lang.Exception(boom!)" to "kotlin.Exception(boom!)".
-    val exceptionString = exception.toString().replace("java.lang.", "kotlin.")
-    eventLog += "codeListener.onUncaughtException($view, $exceptionString)"
+    val exceptionString = exception?.toString()?.replace("java.lang.", "kotlin.")
+    eventLog += "codeListener.onCodeDetached($view, $exceptionString)"
   }
 }