Shopify / Shopify/flash-list

ViewHolderCollection render throws "index out of bounds, not enough layouts" when the render stack outlives a layout-table shrink

Open
#2,440 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
TypeScript
Stars
7.2k
Forks
393
Avg merge
1d 3h
Merged PRs (30d)
1

Description

Description

ViewHolderCollection can throw index out of bounds, not enough layouts while rendering, taking down the tree. It happens when the render stack (RenderStackManager.keyMap) still holds an entry whose index was removed from the layout table (LayoutManager.layouts): the render path reads layouts through an unguarded getLayout(index), which throws instead of returning undefined.

Same error and same getLayout family as #2291, different call site — that one is the validateItemSize measurement callback, this one is the render path.

Current behavior

RecyclerViewManager.modifyChildrenLayout() truncates the layout table on its first line, but prunes the render stack only on some of its exit paths (RecyclerViewManager.ts#L259-L281):

modifyChildrenLayout(layoutInfo, dataLength) {
  this.layoutManager?.modifyLayout(layoutInfo, dataLength);   // truncates layouts when shrinking
  if (dataLength === 0) return false;                          // render stack not pruned
  if (this.layoutManager?.requiresRepaint) { …; return true; } // render stack not pruned
  if (this.hasRenderedProgressively) {
    if (!this.isFirstPaintOnUiComplete) return false;          // render stack not pruned
    return this.recomputeEngagedIndices() !== undefined;       // prunes ONLY if the engaged range changed
  } else {
    this.renderProgressively();                                // this branch does sync the render stack
  }
  …
}

On the main path, recomputeEngagedIndices()updateScrollOffset() calls updateRenderStack() behind if (engagedIndices) (#L111-L127), and EngagedIndicesTracker.updateScrollOffset() returns undefined when the recomputed range has the same endpoints as before (EngagedIndicesTracker.ts#L177-L182). When it reports no change, RenderStackManager.sync() never runs with the new dataLength, so keys pointing at removed indices survive.

RecyclerView renders that state immediately — setRenderId((prev) => prev + 1) on a truthy return, or viewHolderCollectionRef.current?.commitLayout() on a falsy one (RecyclerView.tsx#L237-L246). ViewHolderCollection then walks the render stack and spreads getLayout(index) for each entry (ViewHolderCollection.tsx#L178-L195), which is wired to the unguarded read (RecyclerView.tsx#L589) → LayoutManager.getLayout() throws (LayoutManager.ts#L230-L233):

index out of bounds, not enough layouts

In our production reports this arrives chained under React's "There was an error during concurrent rendering but React was able to recover by instead synchronously rendering the entire root."

Expected behavior

Where the out-of-bounds index comes from

processDataUpdate() maintains this invariant explicitly — it re-syncs the render stack even when recomputeEngagedIndices() reports no change, with the comment "It's important to update render stack so that elements are assgined right keys incase items were deleted" (RecyclerViewManager.ts#L302-L311). modifyChildrenLayout() performs the same truncation but has no equivalent fallback, so callers reaching it directly — the measurement useLayoutEffect at RecyclerView.tsx#L237-L246 — can leave the render stack holding removed indices, and its requiresRepaint and !isFirstPaintOnUiComplete exits return after the truncation too. We haven't attempted a fix there; flagging it in case it's the better place to solve this.

Suggested fix

Make the render read bounds-safe, as the library already does elsewhere: StickyHeaders (3 call sites), the measurement effect's measureItemLayout call, and the public getLayout ref method all use tryGetLayout (RecyclerViewManager.ts#L145-L153), and modifyLayout filters stale indices out of incoming measurements with the comment "layoutInfo may contain stale indices from ViewHolders that were rendered before the data shrunk. Filter out any indices that are now out of bounds." The render path at RecyclerView.tsx#L589 and validateItemSize (#2291) are the two reads that throw instead.

// RecyclerView.tsx
getLayout={(index) => recyclerViewManager.tryGetLayout(index)}

// ViewHolderCollection.tsx
const layout = getLayout(index);
if (layout === undefined) {
  return null;
}

This is safe on its own: the crash requires index >= layouts.length, and after a truncation layouts.length equals the new data length — so a skipped entry always points at an item that no longer exists and nothing visible is dropped. The only cost is that the recycled key stays unused until the next render-stack sync.

Reproduction

Expo Snack or minimal reproduction link:

We don't have a Snack: the runtime trigger is timing-dependent and we can't reproduce it on demand (see Additional context). The invariant break itself, though, is deterministic and reproducible in this repo's own harness — src/__tests__/RecyclerViewManager.test.ts already constructs new RecyclerViewManager(props) directly. The shape:

  1. new RecyclerViewManager(props) with 100 items (estimatedItemSize: 20, drawDistance: 0), then updateLayoutParams({ width: 400, height: 500 }, 0).
  2. Settle it: repeatedly call modifyChildrenLayout(measurementsForCurrentStack(20), 100) until progressive render completes, set isFirstPaintOnUiComplete = true, then feed a few rounds of measurements at height 200. Items measuring much taller than estimated shrink the engaged range, so the render stack now holds keys outside it — assert max(stackIndices) > getEngagedIndices().endIndex.
  3. Call modifyChildrenLayout([], 10)without updating props. This mirrors the measurement useLayoutEffect in RecyclerView.tsx, which passes its own render-closure data?.length ?? 0.
  4. Every retained stack index at or above 10 now returns undefined from tryGetLayout (observed [10 … 25]), and modifyChildrenLayout returns false, so nothing schedules a re-render to repair it. The render path reads those same indices through getLayout(index) (ViewHolderCollection.tsx:195), which throws index out of bounds, not enough layouts.
Failing test (drop into src/__tests__/RecyclerViewManager.invariant.test.ts, run with yarn test RecyclerViewManager)
import { FlashListProps } from "../FlashListProps";
import { RecyclerViewManager } from "../recyclerview/RecyclerViewManager";

type Item = { id: number };

const makeProps = (length: number): FlashListProps<Item> =>
  ({
    data: Array.from({ length }, (_, id) => ({ id })),
    renderItem: () => null,
    keyExtractor: (item: Item) => String(item.id),
    drawDistance: 0,
    estimatedItemSize: 20,
  }) as FlashListProps<Item>;

describe("RecyclerViewManager: render stack vs layout table after modifyChildrenLayout", () => {
  const stackIndices = (manager: RecyclerViewManager<Item>) =>
    Array.from(manager.getRenderStack().values(), ({ index }) => index).sort(
      (a, b) => a - b
    );

  const measurementsForStack = (
    manager: RecyclerViewManager<Item>,
    height: number
  ) =>
    stackIndices(manager).map((index) => ({
      index,
      dimensions: { width: 400, height },
    }));

  /** Settles the manager into a state whose render stack holds keys outside the engaged range. */
  const settledManager = () => {
    const manager = new RecyclerViewManager<Item>(makeProps(100));
    manager.updateLayoutParams({ width: 400, height: 500 }, 0);
    for (let i = 0; i < 12; i++) {
      manager.modifyChildrenLayout(measurementsForStack(manager, 20), 100);
    }
    manager.isFirstPaintOnUiComplete = true;
    for (let i = 0; i < 4; i++) {
      manager.modifyChildrenLayout(measurementsForStack(manager, 200), 100);
    }
    return manager;
  };

  it("leaves out-of-bounds entries when the passed dataLength is smaller than props.data.length", () => {
    const manager = settledManager();

    // Precondition: the render stack holds keys beyond the engaged range.
    expect(Math.max(...stackIndices(manager))).toBeGreaterThan(
      manager.getEngagedIndices().endIndex
    );

    // RecyclerView's measurement useLayoutEffect calls
    // modifyChildrenLayout(layoutInfo, data?.length ?? 0) with its own `data`.
    // Here that argument (10) is smaller than props.data.length (100).
    const needsRerender = manager.modifyChildrenLayout([], 10);

    const outOfBounds = stackIndices(manager).filter(
      (index) => manager.tryGetLayout(index) === undefined
    );

    // The layout table was truncated with the ARGUMENT, but the render stack is only ever
    // pruned with props.data.length (updateRenderStack -> RenderStackManager.sync(...,
    // this.getDataLength())), so entries survive that no longer have a layout. Renders read
    // every entry through getLayout(index) and throw.
    expect({ needsRerender, outOfBounds }).toEqual({
      needsRerender: false,
      outOfBounds: [],
    });
  });

  it("prunes correctly when props and the passed dataLength agree", () => {
    const manager = settledManager();
    manager.updateProps(makeProps(10));
    manager.modifyChildrenLayout([], 10);

    expect(
      stackIndices(manager).filter(
        (index) => manager.tryGetLayout(index) === undefined
      )
    ).toEqual([]);
  });
});

First test fails on main; second passes.

Platform

  • iOS
  • Android
  • Web

Environment

React Native info output:
Paste output here

FlashList version: 2.3.0 — the same code paths are present on current main at the permalinks above.

Additional context

Roughly 116 users and 118 events over about 2.5 months in production, all on the same chat/report screen, and no user has a reliable reproduction — consistent with it requiring the engaged range to recompute unchanged while the render stack holds keys outside it.

Workaround: we carry the tryGetLayout guard above as a package patch. It removes the crash, but it doesn't repair the render-stack / layout-table disagreement, so we'd be glad to see the guard land upstream and would be happy to open the PR.

Related: #2291 (same error message, validateItemSize call site) — we patch that one locally too.

Checklist

  • I've searched existing issues and couldn't find a duplicate
  • I've provided a minimal reproduction (Expo Snack preferred)
  • I'm using the latest version of @shopify/flash-list
  • I've included all required information above

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with RecyclerViewManager.ts, RecyclerView.tsx, ViewHolderCollection.tsx, and LayoutManager.ts, then run the RecyclerViewManager test suite. Reproduce the layout-table shrink using the described 100-to-10 item scenario and verify that rendering no longer throws and stale stack entries are handled safely.

Written by the indexing model from the issue text.

Assessment

Tech stack
react-native, typescript
Domain
frontend, mobile
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.