Skip to content

Commit

Permalink
Fix a host crash in LazyListUpdateProcessor (#2177)
Browse files Browse the repository at this point in the history
I introduced a bug in the mechanism that attempts to replace
animated inserts and removes with update-in-place transitions.

The crash occurs when the before window shrinks while the total
number of elements is growing. The previous code naively
assumed the before window shrinking would always be a scroll,
but it can coincide with a content change.

Closes: #2172
  • Loading branch information
squarejesse authored Jul 10, 2024
1 parent 7db3477 commit de44817
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Changed:
Fixed:
- Using a `data object` for a widget of modifier no longer causes schema parsing to crash.
- Ensuring `LazyList`'s `itemsBefore` and `itemsAfter` properties are always within `[0, itemCount]`, to prevent `IndexOutOfBoundsException` crashes.
- Don't crash in `LazyList` when a scroll and content change occur in the same update.
- Updating a flex container's margin now works correctly for Yoga-based layouts.

Upgraded:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ public abstract class LazyListUpdateProcessor<V : Any, W : Any> {
itemsAfter.addRange(0, itemsBefore, splitIndex, count)
itemsBefore.removeRange(splitIndex, splitIndex + count)
} else if (splitIndex > itemsBefore.size) {
val count = splitIndex - itemsBefore.size
val count = minOf(splitIndex - itemsBefore.size, itemsAfter.size)
itemsBefore.addRange(itemsBefore.size, itemsAfter, 0, count)
itemsAfter.removeRange(0, count)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,89 @@ class LazyListUpdateProcessorTest {
assertThat(processor.toString()).isEqualTo("[2...] . . . . [...13]")
}

/**
* We had a bug where we attempted to move the split between `itemsBefore` and `itemsAfter` beyond
* the end of `itemsAfter`. This bug would occurs when the following held:
*
* * The items before count is shrinking
* * The insert size is greater than all unloaded items (itemsBefore + itemsAfter)
*
* https://github.com/cashapp/redwood/issues/2172
*/
@Test
fun shiftItemsFromAfterToBeforeWhenThereAreNotEnoughToShift() {
processor.itemsBefore(3)
processor.itemsAfter(3)
processor.onEndChanges()
processor.scrollTo(0, 6)
assertThat(processor.toString()).isEqualTo(". . . . . .")

processor.itemsBefore(2)
processor.items.insert(0, "C")
processor.items.insert(1, "D")
processor.items.insert(2, "E")
processor.items.insert(3, "F")
processor.items.insert(4, "G")
processor.items.insert(5, "H")
processor.items.insert(6, "I")
processor.onEndChanges()
processor.scrollTo(0, 12)
assertThat(processor.toString()).isEqualTo(". . C D E Fv2 G H I . . .")
}

@Test
fun shiftItemsFromAfterToBeforeWhenAfterIsEmpty() {
processor.itemsBefore(1)
processor.itemsAfter(0)
processor.onEndChanges()
processor.scrollTo(0, 1)
assertThat(processor.toString()).isEqualTo(".")

processor.itemsBefore(0)
processor.items.insert(0, "A")
processor.items.insert(1, "B")
processor.onEndChanges()
processor.scrollTo(0, 2)
assertThat(processor.toString()).isEqualTo("A B")
}

@Test
fun shiftItemsFromBeforeToAfterWhenThereAreNotEnoughToShift() {
processor.itemsBefore(3)
processor.itemsAfter(3)
processor.onEndChanges()
processor.scrollTo(0, 6)
assertThat(processor.toString()).isEqualTo(". . . . . .")

processor.items.insert(0, "C")
processor.items.insert(1, "D")
processor.items.insert(2, "E")
processor.items.insert(3, "F")
processor.items.insert(4, "G")
processor.items.insert(5, "H")
processor.items.insert(6, "I")
processor.itemsAfter(2)
processor.onEndChanges()
processor.scrollTo(0, 12)
assertThat(processor.toString()).isEqualTo(". . . Cv2 D E F G H I . .")
}

@Test
fun shiftItemsFromBeforeToAfterWhenBeforeIsEmpty() {
processor.itemsBefore(0)
processor.itemsAfter(1)
processor.onEndChanges()
processor.scrollTo(0, 1)
assertThat(processor.toString()).isEqualTo(".")

processor.itemsAfter(0)
processor.items.insert(0, "A")
processor.items.insert(1, "B")
processor.onEndChanges()
processor.scrollTo(0, 2)
assertThat(processor.toString()).isEqualTo("Av2 B")
}

private fun Widget.Children<StringContent>.insert(index: Int, value: String) {
val content = StringContent(value)
val widget = object : Widget<StringContent> {
Expand Down

0 comments on commit de44817

Please sign in to comment.