Remove subtree removal synthesis for old hosts

CHANGELOG.md

@@ -3,6 +3,9 @@
 ## [Unreleased]
 [Unreleased]: https://github.com/cashapp/redwood/compare/0.16.0...HEAD
 
+Breaking:
+- Treehouse hosts running Redwood 0.11.0 or older are not longer actively supported. They will continue to work, but they will experience indefinite memory leaks of native widgets.
+
 New:
 - Nothing yet!
 

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

@@ -23,7 +23,7 @@ public final class app/cash/redwood/protocol/guest/DefaultGuestProtocolAdapter :
 
 public abstract class app/cash/redwood/protocol/guest/GuestProtocolAdapter : app/cash/redwood/protocol/EventSink {
 	public static final field $stable I
-	public synthetic fun <init> (Ljava/lang/String;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public fun <init> ()V
 	public abstract fun emitChanges ()V
 	public abstract fun getRoot ()Lapp/cash/redwood/widget/Widget$Children;
 	public abstract fun getWidgetSystem ()Lapp/cash/redwood/widget/WidgetSystem;

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

@@ -21,7 +21,7 @@ abstract interface app.cash.redwood.protocol.guest/ProtocolWidgetSystemFactory {
 }
 
 abstract class app.cash.redwood.protocol.guest/GuestProtocolAdapter : app.cash.redwood.protocol/EventSink { // app.cash.redwood.protocol.guest/GuestProtocolAdapter|null[0]
-    constructor <init>(app.cash.redwood.protocol/RedwoodVersion) // app.cash.redwood.protocol.guest/GuestProtocolAdapter.<init>|<init>(app.cash.redwood.protocol.RedwoodVersion){}[0]
+    constructor <init>() // app.cash.redwood.protocol.guest/GuestProtocolAdapter.<init>|<init>(){}[0]
 
     abstract val root // app.cash.redwood.protocol.guest/GuestProtocolAdapter.root|{}root[0]
         abstract fun <get-root>(): app.cash.redwood.widget/Widget.Children<kotlin/Unit> // app.cash.redwood.protocol.guest/GuestProtocolAdapter.root.<get-root>|<get-root>(){}[0]

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

@@ -41,10 +41,12 @@ import kotlinx.serialization.json.JsonPrimitive
 @OptIn(RedwoodCodegenApi::class)
 public class DefaultGuestProtocolAdapter(
   public override val json: Json = Json.Default,
+  // Just keep this parameter because we may need it in the future to vary the protocol.
+  @Suppress("unused")
   hostVersion: RedwoodVersion,
   private val widgetSystemFactory: ProtocolWidgetSystemFactory,
   private val mismatchHandler: ProtocolMismatchHandler = ProtocolMismatchHandler.Throwing,
-) : GuestProtocolAdapter(hostVersion) {
+) : GuestProtocolAdapter() {
   private var nextValue = Id.Root.value + 1
   private val widgets = mutableMapOf<Int, ProtocolWidget>()
   private val changes = mutableListOf<Change>()

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

@@ -22,7 +22,6 @@ import app.cash.redwood.protocol.ChildrenTag
 import app.cash.redwood.protocol.EventSink
 import app.cash.redwood.protocol.Id
 import app.cash.redwood.protocol.PropertyTag
-import app.cash.redwood.protocol.RedwoodVersion
 import app.cash.redwood.protocol.WidgetTag
 import app.cash.redwood.widget.Widget
 import app.cash.redwood.widget.WidgetSystem
@@ -37,20 +36,10 @@ import kotlinx.serialization.json.Json
  *
  * This interface is for generated code use only.
  */
-public abstract class GuestProtocolAdapter(
-  hostVersion: RedwoodVersion,
-) : EventSink {
+public abstract class GuestProtocolAdapter : EventSink {
   @RedwoodCodegenApi
   public abstract val json: Json
 
-  /**
-   * 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.
-   */
-  @RedwoodCodegenApi
-  public val synthesizeSubtreeRemoval: Boolean = hostVersion < RedwoodVersion("0.10.0-SNAPSHOT")
-
   /**
    * The provider of factories of widgets which record property changes and whose children changes
    * are also recorded. You **must** attach returned widgets to [root] or the children of a widget
@@ -149,22 +138,7 @@ public abstract class GuestProtocolAdapter(
   public abstract fun removeWidget(id: Id)
 
   @RedwoodCodegenApi
-  public val childrenRemover: ProtocolWidget.ChildrenVisitor = if (synthesizeSubtreeRemoval) {
-    object : ProtocolWidget.ChildrenVisitor {
-      override fun visit(
-        parent: ProtocolWidget,
-        childrenTag: ChildrenTag,
-        children: ProtocolWidgetChildren,
-      ) {
-        // This boxes Id values. Don't bother optimizing since it only serves very old hosts.
-        val childIds = children.widgets.map(ProtocolWidget::id)
-        for (childId in childIds) {
-          removeWidget(childId)
-        }
-        appendRemove(parent.id, childrenTag, 0, childIds.size, childIds)
-      }
-    }
-  } else {
+  public val childrenRemover: ProtocolWidget.ChildrenVisitor =
     object : ProtocolWidget.ChildrenVisitor {
       override fun visit(
         parent: ProtocolWidget,
@@ -176,5 +150,4 @@ public abstract class GuestProtocolAdapter(
         }
       }
     }
-  }
 }

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

@@ -37,26 +37,12 @@ public class ProtocolWidgetChildren(
   }
 
   override fun remove(index: Int, count: Int) {
-    val removedIds = if (guestAdapter.synthesizeSubtreeRemoval) {
-      // This boxes Id values. Don't bother optimizing since it only serves very old hosts.
-      buildList(count) {
-        for (i in index until index + count) {
-          val widget = _widgets[i]
-          this += widget.id
-          guestAdapter.removeWidget(widget.id)
-
-          widget.depthFirstWalk(guestAdapter.childrenRemover)
-        }
-      }
-    } else {
-      for (i in index until index + count) {
-        val widget = _widgets[i]
-        guestAdapter.removeWidget(widget.id)
-        widget.depthFirstWalk(guestAdapter.childrenRemover)
-      }
-      emptyList()
+    for (i in index until index + count) {
+      val widget = _widgets[i]
+      guestAdapter.removeWidget(widget.id)
+      widget.depthFirstWalk(guestAdapter.childrenRemover)
     }
-    guestAdapter.appendRemove(id, tag, index, count, removedIds)
+    guestAdapter.appendRemove(id, tag, index, count, emptyList())
 
     _widgets.remove(index, count)
   }

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

@@ -16,16 +16,10 @@
 package app.cash.redwood.protocol.guest
 
 import androidx.compose.runtime.BroadcastFrameClock
-import androidx.compose.runtime.Composable
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableIntStateOf
-import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.setValue
 import app.cash.redwood.compose.WidgetVersion
-import app.cash.redwood.layout.compose.Column
-import app.cash.redwood.layout.compose.Row
-import app.cash.redwood.lazylayout.compose.ExperimentalRedwoodLazyLayoutApi
-import app.cash.redwood.lazylayout.compose.LazyColumn
 import app.cash.redwood.protocol.Change
 import app.cash.redwood.protocol.ChildrenChange
 import app.cash.redwood.protocol.ChildrenTag
@@ -43,12 +37,8 @@ import app.cash.redwood.ui.Cancellable
 import app.cash.redwood.ui.OnBackPressedCallback
 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
 import com.example.redwood.testapp.compose.Button
 import com.example.redwood.testapp.compose.Button2
 import com.example.redwood.testapp.compose.TestRow
@@ -298,97 +288,6 @@ class ProtocolTest {
     )
   }
 
-  @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))),
-      )
-  }
-
-  @Test fun entireSubtreeRemovedForLazyListPlaceholders() = runTest {
-    assertThat(removeSubtree(latestVersion, LazyListParent))
-      .containsExactly(
-        ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, 1),
-      )
-  }
-
-  /**
-   * Our LazyList binding on host platforms incorrectly assumed that the placeholders children was
-   * append-only. When we fixed a host-side memory leak by traversing guest children, that
-   * introduced a crash. Special-case this by not synthesizing subtree removal for these children.
-   */
-  @Test fun entireSubtreeNotRemovedForLazyListPlaceholders() = runTest {
-    assertThat(removeSubtree(RedwoodVersion("0.9.0"), LazyListParent))
-      .containsExactly(
-        ChildrenChange.Remove(Id(2), ChildrenTag(2), 0, 1, listOf(Id(23))),
-        ChildrenChange.Remove(Id(1), ChildrenTag(1), 0, 1, listOf(Id(2))),
-        ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, 1, listOf(Id(1))),
-      )
-  }
-
-  @Test fun entireSubtreeRemovedForRefreshableLazyListPlaceholders() = runTest {
-    assertThat(removeSubtree(latestVersion, RefreshableLazyListParent))
-      .containsExactly(
-        ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, 1),
-      )
-  }
-
-  @Test fun entireSubtreeNotRemovedForRefreshableLazyListPlaceholders() = runTest {
-    assertThat(removeSubtree(RedwoodVersion("0.9.0"), RefreshableLazyListParent))
-      .containsExactly(
-        ChildrenChange.Remove(Id(2), ChildrenTag(2), 0, 1, listOf(Id(23))),
-        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,
-    parent: SubtreeParent = ColumnParent,
-  ): List<Change> {
-    val (composition, guest) = testProtocolComposition(hostVersion)
-
-    var clicks = 0
-    var remove by mutableStateOf(false)
-    composition.setContent {
-      if (!remove) {
-        Row {
-          parent.Wrap {
-            Button("Click?", onClick = { clicks++ })
-          }
-        }
-      }
-    }
-    val initialSnapshot = composition.awaitSnapshot()
-    val button = initialSnapshot.first { it is Create && it.tag.value == 4 }
-    assertThat(clicks).isEqualTo(0)
-
-    // Ensure the button is present and receiving clicks.
-    guest.sendEvent(Event(button.id, EventTag(2)))
-    assertThat(clicks).isEqualTo(1)
-
-    remove = true
-    val removeChanges = composition.awaitSnapshot()
-
-    // If the whole tree was removed, we cannot target the button anymore.
-    assertFailure { guest.sendEvent(Event(button.id, EventTag(2))) }
-      .isInstanceOf<IllegalArgumentException>()
-      .message()
-      .isEqualTo("Unknown node ID ${button.id.value} for event with tag 2")
-    assertThat(clicks).isEqualTo(1)
-
-    return removeChanges
-  }
-
   private fun TestScope.testProtocolComposition(
     hostVersion: RedwoodVersion = latestVersion,
   ): Pair<TestRedwoodComposition<List<Change>>, GuestProtocolAdapter> {
@@ -408,52 +307,4 @@ class ProtocolTest {
     }
     return composition to guestAdapter
   }
-
-  interface SubtreeParent {
-    @Composable
-    fun Wrap(content: @Composable () -> Unit)
-  }
-
-  object ColumnParent : SubtreeParent {
-    @Composable
-    override fun Wrap(content: @Composable () -> Unit) {
-      Column {
-        content()
-      }
-    }
-  }
-
-  object LazyListParent : SubtreeParent {
-    @Composable
-    override fun Wrap(content: @Composable () -> Unit) {
-      LazyColumn(
-        placeholder = {
-          Text("placeholder")
-        },
-      ) {
-        item {
-          content()
-        }
-      }
-    }
-  }
-
-  object RefreshableLazyListParent : SubtreeParent {
-    @OptIn(ExperimentalRedwoodLazyLayoutApi::class)
-    @Composable
-    override fun Wrap(content: @Composable () -> Unit) {
-      LazyColumn(
-        refreshing = false,
-        onRefresh = {
-        },
-        placeholder = {
-          Text("placeholder")
-        },
-      ) {
-        item {
-          content()
-        }
-      }
-    }
-  }
 }

redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolGuestGeneration.kt

@@ -438,14 +438,7 @@ internal fun generateProtocolWidget(
             .apply {
               for (trait in widget.traits) {
                 if (trait is ProtocolChildren) {
-                  if (workAroundLazyListPlaceholderRemoveCrash(widget, trait)) {
-                    addComment("Work around the LazyList.placeholder remove crash.")
-                    beginControlFlow("if (!guestAdapter.synthesizeSubtreeRemoval)")
-                    addStatement("%N.depthFirstWalk(this, visitor)", trait.name)
-                    endControlFlow()
-                  } else {
-                    addStatement("%N.depthFirstWalk(this, visitor)", trait.name)
-                  }
+                  addStatement("%N.depthFirstWalk(this, visitor)", trait.name)
                 }
               }
             }
@@ -456,25 +449,6 @@ internal fun generateProtocolWidget(
   }
 }
 
-private val placeholderParentTypeNames = listOf(
-  listOf("app.cash.redwood.lazylayout", "LazyList"),
-  listOf("app.cash.redwood.lazylayout", "RefreshableLazyList"),
-)
-
-/**
- * Returns true if this is the `LazyList.placeholder` trait, which had a severe bug in host code
- * by assuming `Widget.Children.remove()` is never be called. (This started crashing when we fixed
- * host-side memory leaks by removing guest-side children.)
- *
- * We work around this by not attempting to fix the host-side memory leak. This turns out to not
- * be a problem in practice anyway, because we never remove placeholders until we remove the
- * entire LazyList.
- */
-private fun workAroundLazyListPlaceholderRemoveCrash(
-  widget: ProtocolWidget,
-  trait: ProtocolWidget.ProtocolTrait,
-): Boolean = widget.type.names in placeholderParentTypeNames && trait.name == "placeholder"
-
 /*
 internal val GrowTagAndSerializer: Pair<ModifierTag, SerializationStrategy<Grow>?> =
     ModifierTag(1_000_001) to object : SerializationStrategy<Grow> {

redwood-treehouse-guest/src/jsMain/kotlin/app/cash/redwood/treehouse/ProtocolBridgeJs.kt

@@ -43,15 +43,14 @@ internal actual fun GuestProtocolAdapter(
   hostVersion: RedwoodVersion,
   widgetSystemFactory: ProtocolWidgetSystemFactory,
   mismatchHandler: ProtocolMismatchHandler,
-): GuestProtocolAdapter = FastGuestProtocolAdapter(json, hostVersion, widgetSystemFactory, mismatchHandler)
+): GuestProtocolAdapter = FastGuestProtocolAdapter(json, widgetSystemFactory, mismatchHandler)
 
 @OptIn(ExperimentalSerializationApi::class, RedwoodCodegenApi::class)
 internal class FastGuestProtocolAdapter(
   override val json: Json = Json.Default,
-  hostVersion: RedwoodVersion,
   private val widgetSystemFactory: ProtocolWidgetSystemFactory,
   private val mismatchHandler: ProtocolMismatchHandler = ProtocolMismatchHandler.Throwing,
-) : GuestProtocolAdapter(hostVersion) {
+) : GuestProtocolAdapter() {
   private var nextValue = Id.Root.value + 1
   private val widgets = JsMap<Int, ProtocolWidget>()
   private val changes = JsArray<Change>()

redwood-treehouse-guest/src/jsTest/kotlin/app/cash/redwood/treehouse/FastGuestProtocolAdapterTest.kt

@@ -137,8 +137,6 @@ class FastGuestProtocolAdapterTest {
     block: (Widget.Children<Unit>, TestSchemaWidgetSystem<Unit>) -> Unit,
   ): List<Change> {
     val guest = FastGuestProtocolAdapter(
-      // Use latest guest version as the host version to avoid any compatibility behavior.
-      hostVersion = guestRedwoodVersion,
       widgetSystemFactory = TestSchemaProtocolWidgetSystemFactory,
       json = json,
       mismatchHandler = ProtocolMismatchHandler.Throwing,