Skip to content

[Android] Fix use-after-free of SurfaceControl.Transaction in HCPP - #192969

Merged
auto-submit[bot] merged 12 commits into
flutter:masterfrom
gmackall:hcpp-transaction-lifetime
Oct 12, 2026
Merged

[Android] Fix use-after-free of SurfaceControl.Transaction in HCPP#192969
auto-submit[bot] merged 12 commits into
flutter:masterfrom
gmackall:hcpp-transaction-lifetime

Conversation

@gmackall

@gmackall gmackall commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes a native SIGABRT in SurfaceComposerClient::Transaction that we were seeing with HCPP under heavy platform view churn.

createTransaction() was adding the transaction to pendingRasterTransactions before handing it back to native, but AHBSwapchainImplVK::Present() keeps writing into it after that (setBuffer, setOnComplete). So the platform thread could merge or close it mid-write. Separately, nothing was holding a reference to the Java object (ASurfaceTransaction_fromJava doesn't take one), so it could also just get GC'd out from under us.

This splits creation from publication. Native now gets an unpublished transaction, holds a global ref to it while writing, and calls publishTransaction from SurfaceTransaction::Apply() after its last write. If publishing fails for whatever reason (FlutterJNI is gone, Java throws), we apply the transaction natively instead of dropping it, so the completion callback still fires and we don't leak the buffer. I don't think those paths are reachable today, but they're easy to just handle.

On a stress test that cycles ~2,900 SurfaceViews per run on a low-core emulator, this went from 24 aborts in 100 runs to 0 in 75.

It's a little awkward that the NDK only goes one direction here: there's ASurfaceTransaction_fromJava, but no toJava. If we could create the transaction natively and hand it to Java at the end, most of this wouldn't be needed, might be feedback that is worth filing.

@mackall-work
[Android] Fix use-after-free of SurfaceControl.Transaction in HCPP
f7d5451 
PlatformViewsController2.createTransaction() published the newly allocated
transaction into pendingRasterTransactions before returning it to the native
raster-thread caller. The caller then keeps writing into that same transaction
(ASurfaceTransaction_setBuffer, ASurfaceTransaction_setOnComplete), so the
platform thread could merge, apply, or close it mid-write. Nothing held a JNI
reference to the Java object either, so it could also be collected out from
under the raw ASurfaceTransaction* the engine had cached.

Split creation from publication: createUnpublishedTransaction() hands back an
unpublished transaction, a JNI global ref keeps the Java object alive for the
duration of the native writes, and SurfaceTransaction::Apply() invokes a submit
callback that calls back into submitTransaction() once the last write is done.
@gmackall gmackall added the CICD Run CI/CD label Sep 18, 2026
@github-actions github-actions Bot added platform-android Android applications specifically engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests team-android Owned by Android platform team labels Sep 18, 2026
gmackall and others added 9 commits September 17, 2026 20:02
@gmackall
[Android] Fix ScopedJavaGlobalRef::Reset() call and clang-format
61c2a0e
@mackall-work
[Android] Add tests for HCPP transaction publication and lifetime
475353e 
- PlatformViewsController2Test: a raster transaction created through
  FlutterJNI.createTransaction() is not merged into a frame until it has been
  submitted through FlutterJNI.submitTransaction().
- ToolkitAndroidTest: SurfaceTransaction::Apply() on a borrowed transaction
  releases the native wrapper and hands the transaction back through the
  submit callback instead of applying it natively, and the completion callback
  registered on it still fires once the owner applies it.
- PlatformViewAndroidJNIImplTest: createTransactionWithSubmitCallback() holds a
  global ref to the Java transaction until the submit callback runs, and passes
  that same transaction to FlutterJNI.submitTransaction().

MockJNIEnv gains CallVoidMethod(V) and weak global ref mocks for the JNI test.
@gmackall
Merge branch 'master' into hcpp-transaction-lifetime
9415f71
@gmackall
Merge branch 'master' into hcpp-transaction-lifetime
91f3c87
@gmackall
Merge branch 'master' into hcpp-transaction-lifetime
62b2733
@mackall-work
Address review feedback: single JNI createTransaction entry point
5cbfe27 
- Collapse createTransaction()/createTransactionWithSubmitCallback() into a
  single pure-virtual createTransaction(std::function<void()>*), removing the
  publish-immediately path and the unused borrowed SurfaceTransaction ctor.
- Remove PlatformViewsController2.createTransaction() test helper; tests now
  create + submit explicitly.
- Build flutter_shell_native_unittests with TESTING and use
  GetMutableProcTable() instead of const_cast.
@mackall-work
Rename submit -> publish for raster transaction handoff
0955ea0 
"Submit" read as "apply now" next to SurfaceTransaction::Apply and
ASurfaceTransaction_apply, and the docs already described the operation as
publishing. Rename so create/publish read as a pair:

- PlatformViewsController2/FlutterJNI.submitTransaction -> publishTransaction
- FlutterJNI.createTransaction -> createUnpublishedTransaction, matching the
  controller method it forwards to
- C++ submit_callback -> publish_callback (SurfaceTransaction, JNI interface,
  call sites, and tests)

No behavior change.
@mackall-work
Take the publish callback out-param by reference
98c3cfd 
The callback is required, but as a pointer it was only guarded by an
FML_DCHECK, so a null argument would be dereferenced in release builds.
A reference makes the requirement a compile-time one and matches the style
guide's convention for non-optional output parameters.

No behavior change.
@mackall-work
Never drop a raster transaction that fails to publish
fdaebd1 
At Flutter's scale, "should not happen" paths do happen, so degrade instead
of dropping frames, leaking buffers, or aborting:

- If FlutterJNI has been collected, or publishTransaction throws, apply the
  transaction natively. This keeps the completion callback registered by
  SurfaceTransaction::Apply from being dropped, which would otherwise leak
  the buffer it holds. Previously the first case silently dropped the
  transaction and the second aborted via FML_CHECK.
- Check and clear exceptions from createUnpublishedTransaction before
  inspecting the result, so a throw falls back to a natively owned
  transaction instead of leaving an exception pending on the raster thread.
- Guard against the publish callback running twice.
- Give FlutterJNI.publishTransaction's exception a diagnosable message.

Adds a test for the collected-FlutterJNI path.
@gmackall
gmackall marked this pull request as ready for review October 7, 2026 18:57
@gmackall
gmackall requested a review from a team as a code owner October 7, 2026 18:57
@gmackall
gmackall requested review from reidbaker and removed request for a team October 7, 2026 18:57
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Oct 7, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the Android surface transaction lifecycle to use unpublished transactions, ensuring native writes on the raster thread complete before the transaction is published back to Java. The review feedback suggests conforming to the Google C++ Style Guide by passing the output callback parameter as a pointer rather than a non-const reference across the JNI interface, implementation, mocks, and unit tests, while also ensuring the pointer is null-checked before assignment.

Comment thread engine/src/flutter/shell/platform/android/jni/platform_view_android_jni.h Resolved
Comment thread engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.h Resolved
Comment thread engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc Resolved
Comment thread engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc Resolved
Comment thread engine/src/flutter/shell/platform/android/android_surface_vk_impeller.cc Resolved
Comment thread engine/src/flutter/shell/platform/android/jni/jni_mock.h Resolved
Comment thread engine/src/flutter/shell/platform/android/android_shell_holder_unittests.cc Resolved
Comment thread engine/src/flutter/shell/platform/android/platform_view_android_jni_impl_unittests.cc Resolved
Comment thread engine/src/flutter/shell/platform/android/platform_view_android_jni_impl_unittests.cc Resolved
gmackall pushed a commit to gmackall/flutter that referenced this pull request Oct 7, 2026
@mackall-work
Add PV-16..PV-19, PV-1 history, flutter#192969 status; order open ite…
29f9cdd 
…ms by priority
reidbaker
reidbaker previously approved these changes Oct 9, 2026
View reviewed changes

@reidbaker reidbaker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not comfortable reviewing the c code so this approval has little weight but the parts I understand make sense.

Comment thread .../src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewsController2.java
* has finished writing into it.
*/
// Called from the raster thread through FlutterJNI.
@RequiresApi(API_LEVELS.API_34)

@reidbaker reidbaker Oct 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document why it is safe to assume that we will always be on api 34 or higher.

@gmackall gmackall Oct 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@gmackall
gmackall requested a review from reidbaker October 9, 2026 22:41
@gmackall gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 12, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Oct 12, 2026
Merged via the queue into flutter:master with commit 91d9b9c Oct 12, 2026
22 of 23 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 12, 2026
This was referenced Oct 12, 2026
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
MylioScott pushed a commit to mylollc/flutter that referenced this pull request Oct 14, 2026
@gmackall @mackall-work @MylioScott
[Android] Fix use-after-free of SurfaceControl.Transaction in HCPP (f…
67132bd 
…lutter#192969)

Fixes a native `SIGABRT` in `SurfaceComposerClient::Transaction` seen
with HCPP under heavy platform view churn.

`createTransaction()` added the transaction to `pendingRasterTransactions`
before handing it back to native, but `AHBSwapchainImplVK::Present()`
keeps writing into it after that (`setBuffer`, `setOnComplete`), so the
platform thread could merge or close it mid-write. Separately, nothing
held a reference to the Java object (`ASurfaceTransaction_fromJava` does
not take one), so it could also be GC'd out from under native code.

This splits creation from publication. Native now gets an unpublished
transaction, holds a global ref to it while writing, and calls
`publishTransaction` from `SurfaceTransaction::Apply()` after its last
write. If publishing fails (`FlutterJNI` is gone, Java throws), the
transaction is applied natively instead of dropped, so the completion
callback still fires and the buffer is not leaked.

On a stress test cycling ~2,900 `SurfaceView`s per run on a low-core
emulator, this went from 24 aborts in 100 runs to 0 in 75.

(cherry picked from commit 91d9b9c)
Co-authored-by: Gray Mackall <mackall@google.com>
Cherry-pick note: the two SurfaceTransaction constructor definitions were
placed by hand; on this branch they sit below an anonymous namespace that
the Android EDR commit added to surface_transaction.cc.
gmackall pushed a commit to gmackall/flutter that referenced this pull request Oct 14, 2026
@mackall-work
[Android] Fix use-after-free of SurfaceControl.Transaction in HCPP (f…
a467664 
…lutter#192969)

Cherry-pick 91d9b9c onto hcpp-backdrop-blur-adpf.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 14, 2026
Closed
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Oct 14, 2026
@engine-flutter-autoroll
Manual roll Flutter from d03768eef055 to 4249ccbc18a3 (158 revisions) (…
975717a 
…#13220)

Manual roll Flutter from d03768eef055 to 4249ccbc18a3 (158 revisions)

Manual roll requested by bmparr@google.com

flutter/flutter@d03768e...4249ccb

2026-10-12 269567208+reidbaker-agent@users.noreply.github.com [AGP 9.1.0 Migration #8] Unify add-to-app module wiring on the variant API (flutter/flutter#193717)
2026-10-12 154381524+flutteractionsbot@users.noreply.github.com [master] Update Flutter DEPS to Dart 5933cb486b4aef2f8cf6b9f89a5651c0d5b0955e (flutter/flutter#193982)
2026-10-12 34871572+gmackall@users.noreply.github.com [Android] Fix use-after-free of SurfaceControl.Transaction in HCPP (flutter/flutter#192969)
2026-10-12 97480502+b-luk@users.noreply.github.com [windows] Add OpenGL compositor tests against ANGLE at D3D11 feature level 10_0 (flutter/flutter#194114)
2026-10-12 evanwall@buffalo.edu Fix HDR image packing for ios (flutter/flutter#192526)
2026-10-12 katelovett@google.com Bump customer tests to pick up material_ui and cupertino_ui (flutter/flutter#194236)
2026-10-12 32538273+ValentinVignal@users.noreply.github.com Remove no-shuffle from text_field_test.dart (flutter/flutter#193550)
2026-10-12 zhongliu88889@gmail.com [web] Clip semantics DOM text labels to stop stealing taps (flutter/flutter#194011)
2026-10-12 engine-flutter-autoroll@skia.org Roll Skia from 9b6ceba4728d to 17f6986c226c (6 revisions) (flutter/flutter#194232)
2026-10-12 kevmoo@users.noreply.github.com [web] Skip non-focusable containers in Focusable.focusAsRouteDefault (flutter/flutter#194138)
2026-10-12 Rusino@users.noreply.github.com [web_paragraph] Honor kTextHeightNone in TextStyle and ParagraphStyle (flutter/flutter#193733)
2026-10-12 engine-flutter-autoroll@skia.org Roll Skia from c88b49d1d6c7 to 9b6ceba4728d (4 revisions) (flutter/flutter#194226)
2026-10-12 15619084+vashworth@users.noreply.github.com Migrate remaining test to macOS 26 (flutter/flutter#193825)
2026-10-12 engine-flutter-autoroll@skia.org Roll Dart SDK from 047ca322aa9c to 221363557163 (1 revision) (flutter/flutter#194219)
2026-10-12 dacoharkes@google.com [flutter_tools] Include dev_dependencies hooks for release integration tests (flutter/flutter#193436)
2026-10-12 engine-flutter-autoroll@skia.org Roll Skia from d8afad1b1829 to c88b49d1d6c7 (1 revision) (flutter/flutter#194198)
2026-10-12 engine-flutter-autoroll@skia.org Roll Skia from b23ddbff1265 to d8afad1b1829 (4 revisions) (flutter/flutter#194195)
2026-10-11 engine-flutter-autoroll@skia.org Roll Skia from a168863e44ec to b23ddbff1265 (1 revision) (flutter/flutter#194185)
2026-10-11 engine-flutter-autoroll@skia.org Roll Skia from 0c1d93abfa04 to a168863e44ec (1 revision) (flutter/flutter#194181)
2026-10-11 robert@odrowaz.dev iOS: Re-enable rendering on willEnterForeground (flutter/flutter#191350)
2026-10-11 engine-flutter-autoroll@skia.org Roll Skia from 93c9d3ffda25 to 0c1d93abfa04 (1 revision) (flutter/flutter#194179)
2026-10-11 116356835+AbdeMohlbi@users.noreply.github.com Remove more instances of deprecated `withOpacity` (flutter/flutter#193559)
2026-10-11 116356835+AbdeMohlbi@users.noreply.github.com Fix another `parameter_assignments` warning (flutter/flutter#193669)
2026-10-11 engine-flutter-autoroll@skia.org Roll Skia from 3136abef8f58 to 93c9d3ffda25 (1 revision) (flutter/flutter#194169)
2026-10-11 leehack@gmail.com [Impeller] Guard the sampler library caches with a mutex (flutter/flutter#193738)
2026-10-11 engine-flutter-autoroll@skia.org Roll Dart SDK from e9c89888bcc1 to 047ca322aa9c (1 revision) (flutter/flutter#194157)
2026-10-10 engine-flutter-autoroll@skia.org Roll Dart SDK from 787baa8aae78 to e9c89888bcc1 (2 revisions) (flutter/flutter#194141)
2026-10-10 33060521+aayushkedawat@users.noreply.github.com Fix OverlayPortal layout in shrink-wrapped overlays (flutter/flutter#193079)
2026-10-10 engine-flutter-autoroll@skia.org Roll Skia from 353076e27b1c to 3136abef8f58 (1 revision) (flutter/flutter#194130)
2026-10-10 engine-flutter-autoroll@skia.org Roll Dart SDK from 88a07589b1ff to 787baa8aae78 (1 revision) (flutter/flutter#194124)
2026-10-10 victorsanniay@gmail.com Route windowing view lookups through WidgetsBinding (flutter/flutter#192666)
2026-10-10 engine-flutter-autoroll@skia.org Roll Dart SDK from 2a06cab34715 to 88a07589b1ff (1 revision) (flutter/flutter#194117)
2026-10-10 engine-flutter-autoroll@skia.org Roll Skia from f9043b7aa798 to 353076e27b1c (2 revisions) (flutter/flutter#194115)
2026-10-10 30870216+gaaclarke@users.noreply.github.com Refactored PipelineLibraryGLES (flutter/flutter#193985)
2026-10-09 brackenavaron@gmail.com add shorthands for scroll physics subclasses in framework (flutter/flutter#193082)
2026-10-09 engine-flutter-autoroll@skia.org Roll Dart SDK from ba889e3cee8a to 2a06cab34715 (1 revision) (flutter/flutter#194105)
2026-10-09 engine-flutter-autoroll@skia.org Roll Skia from 5ea03d8a53a6 to f9043b7aa798 (2 revisions) (flutter/flutter#194099)
2026-10-09 mdebbar@google.com Add docs for using bb and led with LUCI (flutter/flutter#193859)
2026-10-09 engine-flutter-autoroll@skia.org Roll Skia from e329bb617820 to 5ea03d8a53a6 (5 revisions) (flutter/flutter#194087)
2026-10-09 engine-flutter-autoroll@skia.org Roll Packages from 54706bd to d8d9e97 (5 revisions) (flutter/flutter#194080)
2026-10-09 joel.winarske@gmail.com Move the Vulkan wrapped-texture helper out of the GPU surface (flutter/flutter#193001)
2026-10-09 engine-flutter-autoroll@skia.org Roll Skia from 3e54bcf4c963 to e329bb617820 (1 revision) (flutter/flutter#194078)
2026-10-09 engine-flutter-autoroll@skia.org Roll Dart SDK from 23f0308f9af8 to ba889e3cee8a (2 revisions) (flutter/flutter#194077)
2026-10-09 zarah@google.com Bump dartdoc to 9.0.11 (flutter/flutter#194076)
...
suryaKommana2662 pushed a commit to suryaKommana2662/flutter that referenced this pull request Oct 29, 2026
@gmackall @mackall-work @suryaKommana2662
[Android] Fix use-after-free of SurfaceControl.Transaction in HCPP (f…
365f782 
…lutter#192969)

Fixes a native `SIGABRT` in `SurfaceComposerClient::Transaction` that we
were seeing with HCPP under heavy platform view churn.

`createTransaction()` was adding the transaction to
`pendingRasterTransactions` before handing it back to native, but
`AHBSwapchainImplVK::Present()` keeps writing into it after that
(`setBuffer`, `setOnComplete`). So the platform thread could merge or
close it mid-write. Separately, nothing was holding a reference to the
Java object (`ASurfaceTransaction_fromJava` doesn't take one), so it
could also just get GC'd out from under us.

This splits creation from publication. Native now gets an unpublished
transaction, holds a global ref to it while writing, and calls
`publishTransaction` from `SurfaceTransaction::Apply()` after its last
write. If publishing fails for whatever reason (`FlutterJNI` is gone,
Java throws), we apply the transaction natively instead of dropping it,
so the completion callback still fires and we don't leak the buffer. I
don't think those paths are reachable today, but they're easy to just
handle.

On a stress test that cycles ~2,900 `SurfaceView`s per run on a low-core
emulator, this went from 24 aborts in 100 runs to 0 in 75.

It's a little awkward that the NDK only goes one direction here: there's
`ASurfaceTransaction_fromJava`, but no `toJava`. If we could create the
transaction natively and hand it to Java at the end, most of this
wouldn't be needed, might be feedback that is worth filing.

---------

Co-authored-by: Gray Mackall <mackall@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@reidbaker reidbaker reidbaker approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CICD Run CI/CD e: impeller Impeller rendering backend issues and features requests engine flutter/engine related. See also e: labels. platform-android Android applications specifically team-android Owned by Android platform team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gmackall @reidbaker @mackall-work