Synthesize depth-first removes for old hosts (#1936)

This works around a since-fixed memory leak.

CHANGELOG.md

@@ -35,7 +35,7 @@ Fixed:
 - Fix the backgroundColor for `UIViewLazyList` to be transparent. This matches the behavior of the other `LazyList` platform implementations.
 - Fix `TreehouseUIView` to size itself according to the size of its subview.
 - In `UIViewLazyList`, adding `beginUpdates`/`endUpdates` calls to `insertRows`/`deleteRows`, and wrapping changes in `UIView.performWithoutAnimation` blocks.
-- Fix memory leak in 'protocol-guest' and 'protocol-host' where child nodes beneath a removed node were incorrectly retained in an internal map indefinitely.
+- Fix memory leak in 'protocol-guest' and 'protocol-host' where child nodes beneath a removed node were incorrectly retained in an internal map indefinitely. The guest protocol code has been updated to work around this memory leak when deployed to old hosts by sending individual remove operations for each node in the subtree.
 - Ensure that Zipline services are not closed prematurely when disposing a Treehouse UI.
 - In `UIViewLazyList`, don't remove subviews from hierarchy during `prepareForReuse` call
 

redwood-protocol-guest/api/android/redwood-protocol-guest.api

@@ -25,7 +25,7 @@ public final class app/cash/redwood/protocol/guest/ProtocolRedwoodCompositionKt
 
 public final class app/cash/redwood/protocol/guest/ProtocolState {
 	public static final field $stable I
-	public fun <init> ()V
+	public synthetic fun <init> (Ljava/lang/String;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
 	public final fun addWidget (Lapp/cash/redwood/protocol/guest/ProtocolWidget;)V
 	public final fun append (Lapp/cash/redwood/protocol/Change;)V
 	public final fun getChangesOrNull ()Ljava/util/List;
@@ -35,23 +35,23 @@ public final class app/cash/redwood/protocol/guest/ProtocolState {
 }
 
 public abstract interface class app/cash/redwood/protocol/guest/ProtocolWidget : app/cash/redwood/widget/Widget {
+	public abstract fun depthFirstWalk (Lkotlin/jvm/functions/Function3;)V
 	public abstract fun getId-0HhLjSo ()I
 	public abstract fun getTag-BlhN7y0 ()I
 	public synthetic fun getValue ()Ljava/lang/Object;
 	public fun getValue ()Lkotlin/Unit;
 	public abstract fun sendEvent (Lapp/cash/redwood/protocol/Event;)V
-	public abstract fun visitIds (Lkotlin/jvm/functions/Function1;)V
 }
 
 public final class app/cash/redwood/protocol/guest/ProtocolWidgetChildren : app/cash/redwood/widget/Widget$Children {
 	public static final field $stable I
 	public synthetic fun <init> (IILapp/cash/redwood/protocol/guest/ProtocolState;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public final fun depthFirstWalk (Lapp/cash/redwood/protocol/guest/ProtocolWidget;Lkotlin/jvm/functions/Function3;)V
 	public fun getWidgets ()Ljava/util/List;
 	public fun insert (ILapp/cash/redwood/widget/Widget;)V
 	public fun move (III)V
 	public fun onModifierUpdated (ILapp/cash/redwood/widget/Widget;)V
 	public fun remove (II)V
-	public final fun visitIds (Lkotlin/jvm/functions/Function1;)V
 }
 
 public final class app/cash/redwood/protocol/guest/VersionKt {

redwood-protocol-guest/api/jvm/redwood-protocol-guest.api

@@ -25,7 +25,7 @@ public final class app/cash/redwood/protocol/guest/ProtocolRedwoodCompositionKt
 
 public final class app/cash/redwood/protocol/guest/ProtocolState {
 	public static final field $stable I
-	public fun <init> ()V
+	public synthetic fun <init> (Ljava/lang/String;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
 	public final fun addWidget (Lapp/cash/redwood/protocol/guest/ProtocolWidget;)V
 	public final fun append (Lapp/cash/redwood/protocol/Change;)V
 	public final fun getChangesOrNull ()Ljava/util/List;
@@ -35,23 +35,23 @@ public final class app/cash/redwood/protocol/guest/ProtocolState {
 }
 
 public abstract interface class app/cash/redwood/protocol/guest/ProtocolWidget : app/cash/redwood/widget/Widget {
+	public abstract fun depthFirstWalk (Lkotlin/jvm/functions/Function3;)V
 	public abstract fun getId-0HhLjSo ()I
 	public abstract fun getTag-BlhN7y0 ()I
 	public synthetic fun getValue ()Ljava/lang/Object;
 	public fun getValue ()Lkotlin/Unit;
 	public abstract fun sendEvent (Lapp/cash/redwood/protocol/Event;)V
-	public abstract fun visitIds (Lkotlin/jvm/functions/Function1;)V
 }
 
 public final class app/cash/redwood/protocol/guest/ProtocolWidgetChildren : app/cash/redwood/widget/Widget$Children {
 	public static final field $stable I
 	public synthetic fun <init> (IILapp/cash/redwood/protocol/guest/ProtocolState;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public final fun depthFirstWalk (Lapp/cash/redwood/protocol/guest/ProtocolWidget;Lkotlin/jvm/functions/Function3;)V
 	public fun getWidgets ()Ljava/util/List;
 	public fun insert (ILapp/cash/redwood/widget/Widget;)V
 	public fun move (III)V
 	public fun onModifierUpdated (ILapp/cash/redwood/widget/Widget;)V
 	public fun remove (II)V
-	public final fun visitIds (Lkotlin/jvm/functions/Function1;)V
 }
 
 public final class app/cash/redwood/protocol/guest/VersionKt {

redwood-protocol-guest/api/redwood-protocol-guest.klib.api

@@ -25,8 +25,8 @@ abstract interface app.cash.redwood.protocol.guest/ProtocolMismatchHandler { //
     }
 }
 abstract interface app.cash.redwood.protocol.guest/ProtocolWidget : app.cash.redwood.widget/Widget<kotlin/Unit> { // app.cash.redwood.protocol.guest/ProtocolWidget|null[0]
+    abstract fun depthFirstWalk(kotlin/Function3<app.cash.redwood.protocol.guest/ProtocolWidget, app.cash.redwood.protocol/ChildrenTag, app.cash.redwood.protocol.guest/ProtocolWidgetChildren, kotlin/Unit>) // app.cash.redwood.protocol.guest/ProtocolWidget.depthFirstWalk|depthFirstWalk(kotlin.Function3<app.cash.redwood.protocol.guest.ProtocolWidget,app.cash.redwood.protocol.ChildrenTag,app.cash.redwood.protocol.guest.ProtocolWidgetChildren,kotlin.Unit>){}[0]
     abstract fun sendEvent(app.cash.redwood.protocol/Event) // app.cash.redwood.protocol.guest/ProtocolWidget.sendEvent|sendEvent(app.cash.redwood.protocol.Event){}[0]
-    abstract fun visitIds(kotlin/Function1<app.cash.redwood.protocol/Id, kotlin/Unit>) // app.cash.redwood.protocol.guest/ProtocolWidget.visitIds|visitIds(kotlin.Function1<app.cash.redwood.protocol.Id,kotlin.Unit>){}[0]
     abstract val id // app.cash.redwood.protocol.guest/ProtocolWidget.id|{}id[0]
         abstract fun <get-id>(): app.cash.redwood.protocol/Id // app.cash.redwood.protocol.guest/ProtocolWidget.id.<get-id>|<get-id>(){}[0]
     abstract val tag // app.cash.redwood.protocol.guest/ProtocolWidget.tag|{}tag[0]
@@ -35,7 +35,7 @@ abstract interface app.cash.redwood.protocol.guest/ProtocolWidget : app.cash.red
         open fun <get-value>() // app.cash.redwood.protocol.guest/ProtocolWidget.value.<get-value>|<get-value>(){}[0]
 }
 final class app.cash.redwood.protocol.guest/ProtocolState { // app.cash.redwood.protocol.guest/ProtocolState|null[0]
-    constructor <init>() // app.cash.redwood.protocol.guest/ProtocolState.<init>|<init>(){}[0]
+    constructor <init>(app.cash.redwood.protocol/RedwoodVersion) // app.cash.redwood.protocol.guest/ProtocolState.<init>|<init>(app.cash.redwood.protocol.RedwoodVersion){}[0]
     final fun addWidget(app.cash.redwood.protocol.guest/ProtocolWidget) // app.cash.redwood.protocol.guest/ProtocolState.addWidget|addWidget(app.cash.redwood.protocol.guest.ProtocolWidget){}[0]
     final fun append(app.cash.redwood.protocol/Change) // app.cash.redwood.protocol.guest/ProtocolState.append|append(app.cash.redwood.protocol.Change){}[0]
     final fun getChangesOrNull(): kotlin.collections/List<app.cash.redwood.protocol/Change>? // app.cash.redwood.protocol.guest/ProtocolState.getChangesOrNull|getChangesOrNull(){}[0]
@@ -45,11 +45,11 @@ final class app.cash.redwood.protocol.guest/ProtocolState { // app.cash.redwood.
 }
 final class app.cash.redwood.protocol.guest/ProtocolWidgetChildren : app.cash.redwood.widget/Widget.Children<kotlin/Unit> { // app.cash.redwood.protocol.guest/ProtocolWidgetChildren|null[0]
     constructor <init>(app.cash.redwood.protocol/Id, app.cash.redwood.protocol/ChildrenTag, app.cash.redwood.protocol.guest/ProtocolState) // app.cash.redwood.protocol.guest/ProtocolWidgetChildren.<init>|<init>(app.cash.redwood.protocol.Id;app.cash.redwood.protocol.ChildrenTag;app.cash.redwood.protocol.guest.ProtocolState){}[0]
+    final fun depthFirstWalk(app.cash.redwood.protocol.guest/ProtocolWidget, kotlin/Function3<app.cash.redwood.protocol.guest/ProtocolWidget, app.cash.redwood.protocol/ChildrenTag, app.cash.redwood.protocol.guest/ProtocolWidgetChildren, kotlin/Unit>) // app.cash.redwood.protocol.guest/ProtocolWidgetChildren.depthFirstWalk|depthFirstWalk(app.cash.redwood.protocol.guest.ProtocolWidget;kotlin.Function3<app.cash.redwood.protocol.guest.ProtocolWidget,app.cash.redwood.protocol.ChildrenTag,app.cash.redwood.protocol.guest.ProtocolWidgetChildren,kotlin.Unit>){}[0]
     final fun insert(kotlin/Int, app.cash.redwood.widget/Widget<kotlin/Unit>) // app.cash.redwood.protocol.guest/ProtocolWidgetChildren.insert|insert(kotlin.Int;app.cash.redwood.widget.Widget<kotlin.Unit>){}[0]
     final fun move(kotlin/Int, kotlin/Int, kotlin/Int) // app.cash.redwood.protocol.guest/ProtocolWidgetChildren.move|move(kotlin.Int;kotlin.Int;kotlin.Int){}[0]
     final fun onModifierUpdated(kotlin/Int, app.cash.redwood.widget/Widget<kotlin/Unit>) // app.cash.redwood.protocol.guest/ProtocolWidgetChildren.onModifierUpdated|onModifierUpdated(kotlin.Int;app.cash.redwood.widget.Widget<kotlin.Unit>){}[0]
     final fun remove(kotlin/Int, kotlin/Int) // app.cash.redwood.protocol.guest/ProtocolWidgetChildren.remove|remove(kotlin.Int;kotlin.Int){}[0]
-    final fun visitIds(kotlin/Function1<app.cash.redwood.protocol/Id, kotlin/Unit>) // app.cash.redwood.protocol.guest/ProtocolWidgetChildren.visitIds|visitIds(kotlin.Function1<app.cash.redwood.protocol.Id,kotlin.Unit>){}[0]
     final val widgets // app.cash.redwood.protocol.guest/ProtocolWidgetChildren.widgets|{}widgets[0]
         final fun <get-widgets>(): kotlin.collections/List<app.cash.redwood.protocol.guest/ProtocolWidget> // app.cash.redwood.protocol.guest/ProtocolWidgetChildren.widgets.<get-widgets>|<get-widgets>(){}[0]
 }

redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolState.kt

@@ -18,14 +18,24 @@ package app.cash.redwood.protocol.guest
 import app.cash.redwood.RedwoodCodegenApi
 import app.cash.redwood.protocol.Change
 import app.cash.redwood.protocol.Id
+import app.cash.redwood.protocol.RedwoodVersion
 
 /** @suppress For generated code use only. */
 @RedwoodCodegenApi
-public class ProtocolState {
+public class ProtocolState(
+  hostVersion: RedwoodVersion,
+) {
   private var nextValue = Id.Root.value + 1
   private val widgets = PlatformMap<Int, ProtocolWidget>()
   private var changes = PlatformList<Change>()
 
+  /**
+   * Host versions prior to 0.10.0 contained a bug where they did not recursively remove widgets
+   * from the protocol map which leaked any child views of a removed node. We can work around this
+   * on the guest side by synthesizing removes for every node in the subtree.
+   */
+  internal val synthesizeSubtreeRemoval = hostVersion < RedwoodVersion("0.10.0-SNAPSHOT")
+
   public fun nextId(): Id {
     val value = nextValue
     nextValue = value + 1

redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidget.kt

@@ -16,6 +16,7 @@
 package app.cash.redwood.protocol.guest
 
 import app.cash.redwood.RedwoodCodegenApi
+import app.cash.redwood.protocol.ChildrenTag
 import app.cash.redwood.protocol.Event
 import app.cash.redwood.protocol.Id
 import app.cash.redwood.protocol.WidgetTag
@@ -36,6 +37,37 @@ public interface ProtocolWidget : Widget<Unit> {
 
   public fun sendEvent(event: Event)
 
-  /** Recursively visit IDs in this widget's tree, starting with this widget's [id]. */
-  public fun visitIds(block: (Id) -> Unit)
+  /**
+   * Perform a depth-first walk of this widget's children hierarchy.
+   *
+   * For example, given the hierarchy:
+   * ```kotlin
+   * Split(
+   *   left = {
+   *     Row {
+   *       Text(..)
+   *       Button(..)
+   *     }
+   *   },
+   *   right = {
+   *     Column {
+   *       Button(..)
+   *       Text(..)
+   *     }
+   *   }
+   * }
+   * ```
+   * You will see the following argument values passed to [block] if invoked on the `Split`:
+   * 1. parent: `Row`, childrenTag: 1, children: `[Text+Button]`
+   * 2. parent: `Split`, childrenTag: 1, children: `[Row]`
+   * 3. parent: `Column`, childrenTag: 1, children: `[Button+Text]`
+   * 4. parent: `Split`, childrenTag: 2, children: `[Column]`
+   */
+  public fun depthFirstWalk(
+    block: (
+      parent: ProtocolWidget,
+      childrenTag: ChildrenTag,
+      children: ProtocolWidgetChildren,
+    ) -> Unit,
+  )
 }

redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidgetChildren.kt

@@ -39,15 +39,36 @@ public class ProtocolWidgetChildren(
   }
 
   override fun remove(index: Int, count: Int) {
-    val removedIds = ArrayList<Id>(count)
-    for (i in index until index + count) {
-      val widget = _widgets[i]
-      widget.visitIds(state::removeWidget)
-      removedIds += widget.id
+    if (state.synthesizeSubtreeRemoval) {
+      val removedIds = ArrayList<Id>(count)
+      for (i in index until index + count) {
+        val widget = _widgets[i]
+        removedIds += widget.id
+        state.removeWidget(widget.id)
+
+        widget.depthFirstWalk { parent, childrenTag, children ->
+          val childIds = children.widgets.map(ProtocolWidget::id)
+          for (childId in childIds) {
+            state.removeWidget(childId)
+          }
+          state.append(ChildrenChange.Remove(parent.id, childrenTag, 0, childIds.size, childIds))
+        }
+      }
+      state.append(ChildrenChange.Remove(id, tag, index, count, removedIds))
+    } else {
+      for (i in index until index + count) {
+        val widget = _widgets[i]
+        state.removeWidget(widget.id)
+        widget.depthFirstWalk { _, _, children ->
+          for (childWidget in children.widgets) {
+            state.removeWidget(childWidget.id)
+          }
+        }
+      }
+      state.append(ChildrenChange.Remove(id, tag, index, count))
     }
 
     _widgets.remove(index, count)
-    state.append(ChildrenChange.Remove(id, tag, index, count, removedIds))
   }
 
   override fun move(fromIndex: Int, toIndex: Int, count: Int) {
@@ -58,9 +79,13 @@ public class ProtocolWidgetChildren(
   override fun onModifierUpdated(index: Int, widget: Widget<Unit>) {
   }
 
-  public fun visitIds(block: (Id) -> Unit) {
-    for (i in _widgets.indices) {
-      _widgets[i].visitIds(block)
+  public fun depthFirstWalk(
+    parent: ProtocolWidget,
+    block: (ProtocolWidget, ChildrenTag, ProtocolWidgetChildren) -> Unit,
+  ) {
+    for (widget in widgets) {
+      widget.depthFirstWalk(block)
     }
+    block(parent, tag, this)
   }
 }

redwood-protocol-guest/src/commonTest/kotlin/app/cash/redwood/protocol/guest/ProtocolTest.kt

@@ -32,6 +32,7 @@ import app.cash.redwood.protocol.Id
 import app.cash.redwood.protocol.ModifierChange
 import app.cash.redwood.protocol.PropertyChange
 import app.cash.redwood.protocol.PropertyTag
+import app.cash.redwood.protocol.RedwoodVersion
 import app.cash.redwood.protocol.WidgetTag
 import app.cash.redwood.testing.TestRedwoodComposition
 import app.cash.redwood.ui.Cancellable
@@ -40,6 +41,7 @@ import app.cash.redwood.ui.OnBackPressedDispatcher
 import app.cash.redwood.ui.UiConfiguration
 import assertk.assertFailure
 import assertk.assertThat
+import assertk.assertions.containsExactly
 import assertk.assertions.isEqualTo
 import assertk.assertions.isInstanceOf
 import assertk.assertions.message
@@ -58,15 +60,13 @@ import kotlinx.coroutines.test.runTest
 import kotlinx.serialization.json.JsonPrimitive
 
 class ProtocolTest {
-  private val bridge = TestSchemaProtocolBridge.create(
-    // Use latest guest version as the host version to avoid any compatibility behavior.
-    hostVersion = guestRedwoodVersion,
-  )
+  // Use latest guest version as the host version to avoid any compatibility behavior.
+  private val latestVersion = guestRedwoodVersion
 
   @Test fun widgetVersionPropagated() = runTest {
     val composition = ProtocolRedwoodComposition(
       scope = this + BroadcastFrameClock(),
-      bridge = bridge,
+      bridge = TestSchemaProtocolBridge.create(latestVersion),
       changesSink = ::error,
       widgetVersion = 22U,
       onBackPressedDispatcher = object : OnBackPressedDispatcher {
@@ -90,7 +90,7 @@ class ProtocolTest {
   }
 
   @Test fun protocolChangeOrder() = runTest {
-    val composition = testProtocolComposition()
+    val (composition) = testProtocolComposition()
 
     composition.setContent {
       TestRow {
@@ -128,7 +128,7 @@ class ProtocolTest {
   }
 
   @Test fun protocolAlwaysSendsInitialLambdaPresence() = runTest {
-    val composition = testProtocolComposition()
+    val (composition) = testProtocolComposition()
     composition.setContent {
       Button("hi", onClick = null)
       Button("hi", onClick = {})
@@ -166,7 +166,7 @@ class ProtocolTest {
   }
 
   @Test fun protocolSkipsNullableLambdaChangeOfSamePresence() = runTest {
-    val composition = testProtocolComposition()
+    val (composition, bridge) = testProtocolComposition()
 
     var state by mutableStateOf(0)
     composition.setContent {
@@ -241,7 +241,7 @@ class ProtocolTest {
   }
 
   @Test fun protocolSkipsNonNullLambdaChange() = runTest {
-    val composition = testProtocolComposition()
+    val (composition, bridge) = testProtocolComposition()
 
     var state by mutableStateOf(0)
     composition.setContent {
@@ -285,8 +285,24 @@ class ProtocolTest {
     )
   }
 
-  @Test fun entireSubtreeRemoved() = runTest {
-    val composition = testProtocolComposition()
+  @Test fun entireSubtreeRemovedLatest() = runTest {
+    assertThat(removeSubtree(latestVersion))
+      .containsExactly(
+        ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, 1),
+      )
+  }
+
+  @Test fun entireSubtreeRemovedOldHostSynthesizesDepthFirstRemoval() = runTest {
+    assertThat(removeSubtree(RedwoodVersion("0.9.0")))
+      .containsExactly(
+        ChildrenChange.Remove(Id(2), ChildrenTag(1), 0, 1, listOf(Id(3))),
+        ChildrenChange.Remove(Id(1), ChildrenTag(1), 0, 1, listOf(Id(2))),
+        ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, 1, listOf(Id(1))),
+      )
+  }
+
+  private suspend fun TestScope.removeSubtree(hostVersion: RedwoodVersion): List<Change> {
+    val (composition, bridge) = testProtocolComposition(hostVersion)
 
     var clicks = 0
     var remove by mutableStateOf(false)
@@ -307,17 +323,22 @@ class ProtocolTest {
     assertThat(clicks).isEqualTo(1)
 
     remove = true
-    composition.awaitSnapshot()
+    val removeChanges = composition.awaitSnapshot()
 
     // If the whole tree was removed, we cannot target the button anymore.
     assertFailure { bridge.sendEvent(Event(Id(3), EventTag(2))) }
       .isInstanceOf<IllegalArgumentException>()
       .message()
       .isEqualTo("Unknown node ID 3 for event with tag 2")
     assertThat(clicks).isEqualTo(1)
+
+    return removeChanges
   }
 
-  private fun TestScope.testProtocolComposition(): TestRedwoodComposition<List<Change>> {
+  private fun TestScope.testProtocolComposition(
+    hostVersion: RedwoodVersion = latestVersion,
+  ): Pair<TestRedwoodComposition<List<Change>>, ProtocolBridge> {
+    val bridge = TestSchemaProtocolBridge.create(hostVersion)
     val composition = TestRedwoodComposition(
       scope = backgroundScope,
       widgetSystem = bridge.widgetSystem,
@@ -328,6 +349,6 @@ class ProtocolTest {
     backgroundScope.coroutineContext.job.invokeOnCompletion {
       composition.cancel()
     }
-    return composition
+    return composition to bridge
   }
 }

redwood-protocol/api/redwood-protocol.api

@@ -77,6 +77,7 @@ public final class app/cash/redwood/protocol/ChildrenChange$Move$Companion {
 
 public final class app/cash/redwood/protocol/ChildrenChange$Remove : app/cash/redwood/protocol/ChildrenChange {
 	public static final field Companion Lapp/cash/redwood/protocol/ChildrenChange$Remove$Companion;
+	public synthetic fun <init> (IIIILjava/util/List;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
 	public synthetic fun <init> (IIIILjava/util/List;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
 	public fun equals (Ljava/lang/Object;)Z
 	public final fun getCount ()I