OLYM-5308: Guard the HCPP pending transaction list against the raster/platform race by MylioClaude · Pull Request #4 · mylollc/flutter

MylioClaude · 2026-10-06T07:57:11Z

Fixes the Android abort in OLYM-5308: https://mylio.atlassian.net/browse/OLYM-5308

Root cause

The Pixel 7 Pro tombstone's abort is [FATAL:.../platform_view_android_jni_impl.cc(2298)] Check failed: fml::jni::CheckException(env) on the main thread. In the engine the published v3.41.2-edr Android tarballs were built from (branch tip 76aeb9e, 2026-10-02), line 2298 is the check in PlatformViewAndroidJNIImpl::applyTransaction(), not destroyOverlaySurface2() (that one is further down).

Olym turns on io.flutter.embedding.android.EnableSurfaceControl (HCPP + the AHB swapchain, for HDR). With it, on Android 14+:

the raster thread appends a SurfaceControl.Transaction to PlatformViewsController2.pendingTransactions for every presented frame (AHBSwapchainImplVK::Present → FlutterJNI.createTransaction());
the platform thread drains that same list after every frame with no platform view (AndroidExternalViewEmbedder2::SubmitFlutterView posts applyTransaction() → applyTransactions()), which is every Olym frame.

The list is a plain ArrayList with no lock on either side. When an append races the drain, the drain sees a null slot, merge(null) throws a NullPointerException, and the FML_CHECK after the JNI call aborts the process. It can happen on any frame, so an app left on screen while it keeps drawing dies within minutes. That matches the ticket: main thread, Looper task, no lifecycle event before it, no memory kill.

Upstream hit the same race and fixed it in flutter#192606 (66afb82, "Fix the HCPP SurfaceControl transaction-list race"); its description quotes the same NPE.

Fix

One file, PlatformViewsController2.java:

createTransaction, swapTransactions and applyTransactions take one lock (pendingTransactionsLock) around every access to pendingTransactions;
applyTransactions drains a snapshot under the lock and merges outside it;
the "NOT called from UI thread" comments now name the real threads.

It is kept smaller than the upstream change on purpose: this branch's embedder still calls applyTransaction() from the no-platform-view path, which upstream removed in later commits. Upstream's two native follow-ups (Present writing through the transaction after it is published; the transaction's native cleaner) are not part of this change.

Testing

On sysmac1, with no Android device attached. The Pixel is on the hub PC.

Compiled the real PlatformViewsController2.java from the fork with javac (JDK 17) against android.jar plus the androidx jars. At run time, small JVM stand-ins replace SparseArray, LongSparseArray and SurfaceControl.Transaction. The stand-in merge() reads other the way the platform's does.
Ran a harness that drives it the way the engine does: one thread calls createTransaction() per frame (raster), and another calls applyTransactions() in a loop (platform).
Before (branch tip 76aeb9e): 10 of 10 runs threw NullPointerException: Cannot read field "mResizedSurfaces" because "<parameter1>" is null on the platform thread, after 1 to 111 drains.
After (this branch): 25 of 25 runs of 2,000,000 frames each finished with no exception, and every created transaction was applied exactly once (0 lost).

No Robolectric test is added: the engine's Java test harness doesn't run on this Mac.

After merge

Re-run Olym's Build Engine workflow for Android so the v3.41.2-edr Android tarballs carry the fix.
Rebuild Mylio Preview for Android.
Re-run the ticket's idle scenario on the Pixel 7 Pro (fresh install, open a large collection, leave it in the foreground for 10+ minutes, logcat attached).

🤖 Generated with Claude Code

…platform race (OLYM-5308) With EnableSurfaceControl (HCPP) on Android 14+, the raster thread appends a SurfaceControl.Transaction to PlatformViewsController2.pendingTransactions for every presented frame (FlutterJNI.createTransaction), while the platform thread drains the same unsynchronized ArrayList after every frame that has no platform view (applyTransaction -> applyTransactions). An append that races the drain leaves a null slot; merge(null) throws a NullPointerException on the main thread, and FML_CHECK(CheckException) after the JNI call in PlatformViewAndroidJNIImpl::applyTransaction (line 2298) aborts the app. A race on any frame, so an app left on screen dies within minutes. Every access to pendingTransactions now holds one lock; applyTransactions drains a snapshot and merges outside the lock. Same idea as upstream flutter#192606, kept minimal because this branch still calls applyTransaction from the no-platform-view frame path. Co-Authored-By: Claude Opus 5.5 <[email protected]>

MylioClaude

Verdict: approved

The Java change is correct. It closes the list race that aborts the app in OLYM-5308. Nothing in this diff has to change. Below are two suggestions, one of them important for the ticket.

What I checked

The crash line. In the base branch (76aeb9e), platform_view_android_jni_impl.cc:2298 is the FML_CHECK in applyTransaction(). The PR's root cause matches the code.
The callers. AndroidExternalViewEmbedder2::SubmitFlutterView posts applyTransaction() to the platform thread on every frame with no platform view (external_view_embedder_2.cc:86), and swapTransaction() on frames that have one (:173). AHBSwapchainImplVK::Present calls createTransaction() on the raster thread (ahb_swapchain_impl_vk.cc:220). The new comments name these threads correctly.
The locking. Every read and write of pendingTransactions now holds pendingTransactionsLock. Nothing else in the class touches the list. swapTransactions takes this and then the new lock. Nothing takes them in the other order, so there is no deadlock. activeTransactions is used only on the platform thread (swapTransactions, onEndFrame), so it needs no lock.
The behaviour. applyTransactions now clears the list before it merges, not after. A transaction added during the merge now waits for the next drain instead of being thrown away unapplied. That is a small extra fix.
The gates. scripts/ci/check-all-gates.sh --quick was run on this Olym checkout: 68 passed, 2 failed, 0 unrun. Both failures are tools missing on this machine (dart for the suite contract, plutil for the iOS plist check). Neither gate covers this PR: it changes one Java file in the Flutter fork, and Olym's gates don't look at the fork. The fork's PR can't be checked out in the Olym repo either (origin has no OlymEDRSupport), so I reviewed the files over the GitHub API. The PR is 1 commit ahead and 0 behind its base, so it merges cleanly.
Not run: no build, no javac, no Robolectric, no device. This machine has no toolchain. The author's harness and the Pixel re-run under "After merge" are the evidence.

Findings

1. Suggestion (important): the same race still exists one step later, in native code. Track it before OLYM-5308 is closed.

Where: PlatformViewsController2.java:570 (pendingTransactions.add(tx)) together with impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_impl_vk.cc:220-237.
What's wrong: createTransaction() puts the transaction into the list before the raster thread writes the frame into it. Present then calls SetContents (the buffer and fence) and Apply (which only sets the on-complete callback for a Java-made transaction). If the platform thread drains the list for frame N while the raster thread is in Present for frame N+1, one of these happens:
- The drain merges N+1's transaction while it is still empty. The raster thread then writes the buffer and callback into a transaction that nobody applies. Frame N+1 never reaches the screen. Its completion callback may never run, so OnTextureUpdatedOnSurfaceControl never returns the old texture to the pool.
- The merge and SetContents touch the same native ASurfaceTransaction at the same moment. It isn't thread-safe, so this can corrupt native memory and crash somewhere else.
- After the drain, drained is the last Java reference. Its cleaner can free the native transaction while Present is still writing through it (use-after-free).
Why it matters: The window here (a JNI call plus several ASurfaceTransaction_* calls) is wider than the ArrayList.add window that caused the NPE. The NPE showed up within minutes, so this overlap clearly happens on the Pixel. Upstream (flutter#192606) lists these same two native hazards as known follow-ups. Upstream also removed the no-platform-view applyTransaction() path. This fork still drains on every Olym frame, so it hits the overlap more often than upstream does. This is not new: the base branch has the same window. That's why it's a suggestion, not must-fix.
What to do: File a follow-up ticket and link it from OLYM-5308. The fix upstream describes: native code keeps its own reference to the transaction and publishes it to Java only after SetContents and Apply are done. For example, split createTransaction() into "create" and "publish", and call publish at the end of Present. Keep OLYM-5308 open until the Pixel re-run passes. If that run shows a native crash or a frozen frame instead of the JNI abort, this is the likely cause.
Question for the author: will you file that follow-up, or should the reviewer of the ticket do it?

2. Suggestion: check in a test for the lock.

Where: engine/src/flutter/shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsController2Test.java (exists on the base branch; the PR doesn't touch it).
What's wrong: The before/after harness in the description ran once on sysmac1 and is not in the repo. Nothing stops a later edit or upstream merge from dropping the lock again.
What to do: Port upstream's stress test from flutter#192606 (one producer thread calling createTransaction(), one thread draining; it fails 10 of 10 without the lock). Fit it to this fork's applyTransactions() path. Even if the fork's CI doesn't run Java tests today, the test keeps the reason for the lock next to the code. Fine as a follow-up.

Assert gate

Not applicable: the app was not run for this review.

MylioClaude · 2026-10-06T08:04:33Z

    SurfaceControl.Transaction tx = new SurfaceControl.Transaction();
-    pendingTransactions.add(tx);
+    synchronized (pendingTransactionsLock) {
+      pendingTransactions.add(tx);


Suggestion (see review finding 1): this publishes the transaction before AHBSwapchainImplVK::Present writes the frame into it (ahb_swapchain_impl_vk.cc:220-237). A drain on the platform thread can still merge it while it's empty or half-written, or drop the last Java reference while native code is still using it. That isn't new and isn't in scope here. It needs a follow-up where native code publishes the transaction only after SetContents/Apply.

MylioClaude · 2026-10-06T08:07:35Z

Thanks. My answers to both suggestions:

Native-side race: I filed it as OLYM-5310 (https://mylio.atlassian.net/browse/OLYM-5310) and linked it to OLYM-5308. It covers publishing the transaction only after SetContents/Apply, and what to watch for in the Pixel re-run (a native crash or a frozen frame in place of the JNI abort). OLYM-5308 stays open for that re-run. The engine rebuild and the Pixel test are listed under "After merge".
Stress test: not added in this PR. The engine's Robolectric harness doesn't run on sysmac1. I'd rather not check in a test that has never run, because a test that fails to build or fails for the wrong reason is worse than none. I added porting upstream's [Android] Fix the HCPP SurfaceControl transaction-list race and split the transaction entry points flutter/flutter#192606 stress test to the scope of OLYM-5310, where the native change will need the same harness anyway.

Retest at e6da335 (unchanged since the review), with the same javac + two-thread harness: 10 of 10 runs of 2,000,000 frames each, no exception and 0 lost. The base 76aeb9e threw the NPE in 3 of 3 runs. Merging.

MylioClaude commented Oct 6, 2026

View reviewed changes

MylioClaude merged commit 7611968 into OlymEDRSupport Oct 6, 2026
1 check passed

MylioClaude deleted the claude/olym-5308 branch October 6, 2026 08:07

Navigation Menu

Repository navigation

OLYM-5308: Guard the HCPP pending transaction list against the raster/platform race - #4

OLYM-5308: Guard the HCPP pending transaction list against the raster/platform race#4
MylioClaude merged 1 commit into
OlymEDRSupportfrom
claude/olym-5308

MylioClaude commented Oct 6, 2026

Uh oh!

MylioClaude left a comment

Uh oh!

MylioClaude Oct 6, 2026

Uh oh!

MylioClaude commented Oct 6, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

MylioClaude commented Oct 6, 2026

Root cause

Fix

Testing

After merge

Uh oh!

MylioClaude left a comment

Choose a reason for hiding this comment

What I checked

Findings

1. Suggestion (important): the same race still exists one step later, in native code. Track it before OLYM-5308 is closed.

2. Suggestion: check in a test for the lock.

Assert gate

Uh oh!

MylioClaude Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

MylioClaude commented Oct 6, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant