From 9e72e7bdb6213a421cd029df23e686fcad5816f2 Mon Sep 17 00:00:00 2001 From: Jake Wharton Date: Wed, 20 Nov 2024 16:37:01 -0500 Subject: [PATCH] Remove subtree removal synthesis for old hosts This is inhibiting the ability to easily support Compose capabilities like movableContentOf. --- CHANGELOG.md | 3 + .../api/redwood-protocol-guest.api | 2 +- .../api/redwood-protocol-guest.klib.api | 2 +- .../guest/DefaultGuestProtocolAdapter.kt | 4 +- .../protocol/guest/GuestProtocolAdapter.kt | 31 +--- .../protocol/guest/ProtocolWidgetChildren.kt | 24 +-- .../redwood/protocol/guest/ProtocolTest.kt | 149 ------------------ .../codegen/protocolGuestGeneration.kt | 28 +--- .../redwood/treehouse/ProtocolBridgeJs.kt | 5 +- .../treehouse/FastGuestProtocolAdapterTest.kt | 2 - 10 files changed, 18 insertions(+), 232 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55f95ef256..b31d0c442e 100644 --- a/CHANGELOG.md +++ b/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! diff --git a/redwood-protocol-guest/api/redwood-protocol-guest.api b/redwood-protocol-guest/api/redwood-protocol-guest.api index d75fd6c3a6..08a54248ac 100644 --- a/redwood-protocol-guest/api/redwood-protocol-guest.api +++ b/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 (Ljava/lang/String;Lkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun ()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; diff --git a/redwood-protocol-guest/api/redwood-protocol-guest.klib.api b/redwood-protocol-guest/api/redwood-protocol-guest.klib.api index 8a23618f20..0855765847 100644 --- a/redwood-protocol-guest/api/redwood-protocol-guest.klib.api +++ b/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 (app.cash.redwood.protocol/RedwoodVersion) // app.cash.redwood.protocol.guest/GuestProtocolAdapter.|(app.cash.redwood.protocol.RedwoodVersion){}[0] + constructor () // app.cash.redwood.protocol.guest/GuestProtocolAdapter.|(){}[0] abstract val root // app.cash.redwood.protocol.guest/GuestProtocolAdapter.root|{}root[0] abstract fun (): app.cash.redwood.widget/Widget.Children // app.cash.redwood.protocol.guest/GuestProtocolAdapter.root.|(){}[0] diff --git a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/DefaultGuestProtocolAdapter.kt b/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/DefaultGuestProtocolAdapter.kt index 8a38ef7614..52636b25ab 100644 --- a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/DefaultGuestProtocolAdapter.kt +++ b/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() private val changes = mutableListOf() diff --git a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/GuestProtocolAdapter.kt b/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/GuestProtocolAdapter.kt index 50c7c44997..ca0a2066f0 100644 --- a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/GuestProtocolAdapter.kt +++ b/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( } } } - } } diff --git a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidgetChildren.kt b/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidgetChildren.kt index 1f4e53e99d..1fac07fd6b 100644 --- a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidgetChildren.kt +++ b/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) } diff --git a/redwood-protocol-guest/src/commonTest/kotlin/app/cash/redwood/protocol/guest/ProtocolTest.kt b/redwood-protocol-guest/src/commonTest/kotlin/app/cash/redwood/protocol/guest/ProtocolTest.kt index a1e034a7ff..9075886a76 100644 --- a/redwood-protocol-guest/src/commonTest/kotlin/app/cash/redwood/protocol/guest/ProtocolTest.kt +++ b/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 { - 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() - .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>, 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() - } - } - } - } } diff --git a/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolGuestGeneration.kt b/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolGuestGeneration.kt index 59bf289cc2..01c9043a3f 100644 --- a/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolGuestGeneration.kt +++ b/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(1_000_001) to object : SerializationStrategy { diff --git a/redwood-treehouse-guest/src/jsMain/kotlin/app/cash/redwood/treehouse/ProtocolBridgeJs.kt b/redwood-treehouse-guest/src/jsMain/kotlin/app/cash/redwood/treehouse/ProtocolBridgeJs.kt index 534ef21797..f1d04592ba 100644 --- a/redwood-treehouse-guest/src/jsMain/kotlin/app/cash/redwood/treehouse/ProtocolBridgeJs.kt +++ b/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() private val changes = JsArray() diff --git a/redwood-treehouse-guest/src/jsTest/kotlin/app/cash/redwood/treehouse/FastGuestProtocolAdapterTest.kt b/redwood-treehouse-guest/src/jsTest/kotlin/app/cash/redwood/treehouse/FastGuestProtocolAdapterTest.kt index 6c7cf955e4..c476fce179 100644 --- a/redwood-treehouse-guest/src/jsTest/kotlin/app/cash/redwood/treehouse/FastGuestProtocolAdapterTest.kt +++ b/redwood-treehouse-guest/src/jsTest/kotlin/app/cash/redwood/treehouse/FastGuestProtocolAdapterTest.kt @@ -137,8 +137,6 @@ class FastGuestProtocolAdapterTest { block: (Widget.Children, TestSchemaWidgetSystem) -> Unit, ): List { 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,