Fabric: schedulerDidFinishTransaction picks oldest (not newest) pending transaction as merge target, corrupting mount order
Chưa có ai nhận issue này.
Đánh giá
- Độ khó
- 2/5
- Thời gian dự kiến
- 1-3 giờ
- Mức phù hợp với người mới
- 78/100
- Loại issue
- Lỗi
- Độ rõ ràng
- Đặc tả rõ ràng
- Mức độ hoạt động
- Sôi nổi
- Công nghệ
- android, cpp, react-native
- Lĩnh vực
- mobile, mobile-dev
Hướng nghiên cứu
Bắt đầu trong ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp và kiểm tra schedulerDidFinishTransaction cùng với MountingTransaction::canMergeWith. Tạo ba transaction trong một unit test xác định, xác minh thứ tự thực thi của nhiều transaction đang chờ xử lý và xác nhận rằng transaction đích của thao tác merge là transaction được đưa vào hàng đợi gần đây nhất.
Do mô hình lập chỉ mục viết ra từ nội dung của issue.
Mô tả
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:
T3was diffed by the renderer against shadow-tree state that already includesT2.- The forward search finds
T1first and mergesT3into it, producing[T1+T3, T2], which executes asT1 → T3 → T2. - But
T3was never diffed against a tree withoutT2in it — running it beforeT2desyncs 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 Delete↔Create 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.
- Ngôn ngữ chính
- C++
- Star
- 127k
- Fork
- 25.3k
- Chỉ số merge pull request
- Không có pull request nào được merge trong 30 ngày
Hướng dẫn đóng góp
Bắt đầu từ đâu
- Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
- Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
- Fork repository và làm thay đổi trên một nhánh.
- Mở pull request có tham chiếu số hiệu của issue.
Issue khác của react/react-native
-
Needs: Author Feedback Needs: Repro
Độ khó 1/5 Dưới một giờ Mức phù hợp với người mới 92/100
react/react-native#58621 · 1 bình luận ·
-
Needs: Author Feedback Needs: Repro
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 85/100
react/react-native#58610 · 1 bình luận ·
-
Needs: Triage :mag:
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 82/100
react/react-native#58565 · 1 bình luận · 2 reaction ·
-
Needs: Author Feedback Needs: Repro
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 88/100
react/react-native#58555 · 4 bình luận · 1 reaction ·
-
Needs: Attention Needs: Repro
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 85/100
react/react-native#58526 · 2 bình luận ·
Tất cả issue của react/react-native
Issue tương tự
-
enhancement
Độ khó 1/5 Dưới một giờ Mức phù hợp với người mới 88/100
QuantStack/git2cpp#187 ·
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 86/100
-
Độ khó 1/5 Dưới một giờ Mức phù hợp với người mới 90/100
AXERA-TECH/ax-llm#77 ·
-
Độ khó 1/5 Dưới một giờ Mức phù hợp với người mới 90/100
games-on-whales/wolf#509 ·
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 82/100