react / react/react-native

Fabric: schedulerDidFinishTransaction picks oldest (not newest) pending transaction as merge target, corrupting mount order

Open Beginner friendly
#58,175 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Needs: Author Feedback Needs: Repro
Dominant language
C++
Stars
127k
Forks
25.3k
Avg merge
1d 23h
Merged PRs (30d)
4

Description

Description

FabricUIManagerBinding::schedulerDidFinishTransaction (Android, ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp) selects which pending mounting transaction an incoming transaction should merge into using a forward search:

auto pendingTransaction = std::find_if(
    pendingTransactions_.begin(),
    pendingTransactions_.end(),
    [&](const auto& transaction) {
      return transaction.getSurfaceId() == mountingTransaction->getSurfaceId();
    });

if (pendingTransaction != pendingTransactions_.end() &&
    pendingTransaction->canMergeWith(*mountingTransaction)) {
  pendingTransaction->mergeWith(std::move(*mountingTransaction));
} else {
  pendingTransactions_.push_back(std::move(*mountingTransaction));
}

std::find_if over begin()/end() returns the oldest queued transaction for a surface. Whenever a surface has more than one pending transaction at once — which can legitimately happen any time MountingTransaction::canMergeWith refuses a merge between two adjacent transactions — this picks the wrong merge target.

Repro scenario

Given pending transactions T1, T2 for the same surface (already queued separately because canMergeWith refused to combine them), and an incoming T3:

  • T3 was diffed by the renderer against shadow-tree state that already includes T2.
  • The forward search finds T1 first and merges T3 into it, producing [T1+T3, T2], which executes as T1 → T3 → T2.
  • But T3 was never diffed against a tree without T2 in it — running it before T2 desyncs the native view tree from the shadow tree that produced the diff.

This manifests as native-tree/shadow-tree divergence at mount-apply time: an insert lands at an index the real parent doesn't have (IndexOutOfBoundsException in addViewAt), or a remove resolves a stale parent tag that's no longer the expected ViewGroup (IllegalStateException in removeViewAt).

Expected behavior

A new transaction should only ever merge into the most recently queued pending transaction for its surface — the only one whose diffed-against tree state actually reflects everything already queued. That means the merge-target lookup should search from the back (rbegin()/rend()) instead of the front (begin()/end()).

Fix

Three-token change:

auto pendingTransaction = std::find_if(
    pendingTransactions_.rbegin(),
    pendingTransactions_.rend(),
    [&](const auto& transaction) {
      return transaction.getSurfaceId() == mountingTransaction->getSurfaceId();
    });

if (pendingTransaction != pendingTransactions_.rend() &&
    pendingTransaction->canMergeWith(*mountingTransaction)) {
  ...

Environment

React Native 0.86.0, Android (Fabric), observed downstream in a fork carrying an additional local patch that makes canMergeWith refuse certain merges (a DeleteCreate tag-pairing guard), which is what surfaces multi-entry pendingTransactions_ queues in practice. The underlying merge-target selection bug is present in schedulerDidFinishTransaction upstream regardless of that local patch — any code path that causes canMergeWith to refuse a merge (including future upstream guards) would trigger the same divergence.

Reproducibility

No deterministic manual repro is provided here — the bug is timing/queue-state dependent. The mechanism is provable deterministically with a unit test against MountingTransaction directly (construct 3 transactions, assert execution order), without needing device timing.

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 in ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp and inspect schedulerDidFinishTransaction alongside MountingTransaction::canMergeWith. Construct three transactions in a deterministic unit test, verify the execution order for multiple pending transactions, and confirm the merge target is the most recently queued transaction.

Written by the indexing model from the issue text.

Assessment

Tech stack
android, cpp, react-native
Domain
mobile, mobile-dev
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.