Skip to content

[macOS] Fix back buffer cache returning a wrong-size surface (Impeller null deref in Canvas::SetupRenderPass) - #192522

Open
abdelaziz-mahdy wants to merge 6 commits into
flutter:masterfrom
abdelaziz-mahdy:fix/macos-backbuffer-cache-wrong-size
Open

abdelaziz-mahdy wants to merge 6 commits into
flutter:masterfrom
abdelaziz-mahdy:fix/macos-backbuffer-cache-wrong-size

Conversation

@abdelaziz-mahdy

@abdelaziz-mahdy abdelaziz-mahdy commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

On macOS with Impeller, the raster thread can crash in impeller::Canvas::SetupRenderPass() (color0.texture->GetSize() on a null texture) via EmbedderExternalViewEmbedder::SubmitFlutterView. It reproduces reliably in a video-player app by toggling native fullscreen (NSWindow toggleFullScreen:) while a Texture widget is on screen, and has been reported upstream on foregrounding, display hot-plug and window drag.

Root cause

-[FlutterBackBufferCache removeSurfaceForSize:] only compared the requested size with the first cached surface, then returned the youngest idle surface without checking that surface's size. When frame sizes flip-flop (A → B → A) while the previous A surface is still held by the window server (IOSurfaceIsInUse), the cache ends up holding both sizes and the next request for A receives the B surface.

MakeRenderTargetFromBackingStoreImpeller then wraps an MTLTexture whose real size disagrees with the requested backing-store size. The failure is silent all the way down:

  1. TextureMTL invalidates itself (The texture and its descriptor disagree about its size. — a VALIDATION_LOG, compiled out in release).
  2. RenderTarget::SetColorAttachment drops the invalid attachment without logging.
  3. Canvas::SetupRenderPass reads GetColorAttachment(0) (a default ColorAttachment{}) and dereferences the null texture.

Fix

  1. FlutterSurfaceManager.mm — returnSurfaces: discards returned surfaces whose size differs from what the cache already holds. The cache only ever contains one size (the last requested one, since removeSurfaceForSize: purges on a size change), so surfaces coming back with another size are stale by definition. This keeps the existing single-size invariant instead of adding a size check on the take side (approach suggested in review by @knopp; the doc comment on removeSurfaceForSize: now states the invariant).
  2. embedder.cc — the Impeller Metal backing-store wrapper rejects invalid (not just null) wrapped textures. A bad backing store now drops that layer for one frame through the existing null-target path (Layer::RenderFlutterContents guards it, the target never enters the render-target cache, the next frame gets a fresh backing store) and logs the requested size, instead of taking the process down.

Tests

  • FlutterSurfaceManager.BackBufferCacheNeverReturnsSurfaceOfDifferentSize (flutter_desktop_darwin_unittests) — constructs the A → B → A / in-use sequence against a real FlutterSurfaceManager. Fails before fix 1 (cache returned 50x50 for a 100x100 request), passes after; also asserts the stale surface never enters the cache (count stays 1).
  • EmbedderTest.WrongSizeMetalBackingStoreDoesNotCrashImpeller (embedder_unittests) — the compositor hands the engine a Metal texture half the requested size under --enable-impeller. Before fix 2 the raster thread dies (SIGABRT on the size DCHECK in Layer::RenderFlutterContentsImpeller in unopt builds; the null dereference above in release); after, the frame is presented without the layer and the engine shuts down cleanly.
  • flutter_desktop_darwin_unittests (236) and embedder_unittests EmbedderTest.* (96 + 8 skipped) pass locally on host_debug_unopt_arm64.

Reproduction

Standalone app: https://github.com/abdelaziz-mahdy/flutter-macos-backbuffer-repro (stock flutter create macOS app; a Swift timer shrinks the window by 1 px and restores it synchronously every 2.5 s, which yields frame sizes A → B → A with one frame at B while the window server still holds the previous A surface). Run on host_debug_unopt_arm64, Apple M2 Pro, macOS 26.6.2:

Engine Result
4e6768eca70 (master, unfixed) 3/3 runs crash on the second flip within 7 s: The texture and its descriptor disagree about its size. then Check failed: render_target_->GetRenderTargetSize() == c->GetRenderSurfaceSize() on io.flutter.raster. With a temporary log in surfaceForSize: the handoff is visible: req=1600x1200 REUSED got=1598x1198.
this branch (a8073db1e1d) 3 × 60 s runs (23–24 flips each) plus one 4-minute run (97 flips): 0 validation lines, 0 crashes. Same result on the earlier take-side version (cec7d27aa2c).

Plain fullscreen toggling (the trigger we hit in production, with a video Texture on screen) produces monotonic sizes during the animation and only crashes occasionally; the flip is the same A → B → A pattern reduced to one frame.

Fixes #185394

Pre-launch Checklist

@abdelaziz-mahdy
abdelaziz-mahdy marked this pull request as ready for review September 10, 2026 00:27
@abdelaziz-mahdy
abdelaziz-mahdy requested a review from a team as a code owner September 10, 2026 00:27

@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 prevents crashes in Impeller when a Metal texture's size disagrees with the requested backing store size. It updates FlutterBackBufferCache to filter out cached surfaces of different sizes individually rather than purging the entire cache, and adds validation checks to ensure wrapped Metal textures and MSAA textures are valid before use. Corresponding regression tests are added for both the macOS surface manager and the embedder. Feedback on these changes suggests refactoring the block-based NSPredicate filtering in removeSurfaceForSize: to a manual backwards loop to avoid heap allocations in the rendering hot path.

@github-actions github-actions Bot added engine flutter/engine related. See also e: labels. platform-macos Building on or for macOS specifically a: desktop Running on desktop team-macos Owned by the macOS platform team labels Sep 10, 2026
@okorohelijah
okorohelijah requested a review from knopp September 10, 2026 21:55
@knopp

knopp commented Sep 11, 2026

Copy link
Copy Markdown
Member

I think this is a valid issue but I'm not sure I like the solution. I'd prefer if we kept the invariant of having all surfaces in the cache having same size. That probably means ignoring wrong size surfaces inside returnSurfaces:.

@knopp

knopp commented Sep 11, 2026

Copy link
Copy Markdown
Member

I think I'd go with

- (void)returnSurfaces:(nonnull NSArray<FlutterSurface*>*)returnedSurfaces {
  @synchronized(self) {
    if (_surfaces.count > 0 && returnedSurfaces.count > 0 &&
        !CGSizeEqualToSize(returnedSurfaces.firstObject.size, _surfaces.firstObject.size)) {
      // Any cached surface size will match last requested size (see removeSurfaceForSize:). If the
      // size of incoming surfaces don't match cached surface size it means they are stale and
      // should be discarded.
      return;
    }

@abdelaziz-mahdy

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback! I gave your version a try and it solves the issue too, so I don't mind going with it. Switched in a8073db: removeSurfaceForSize: is back to the original and returnSurfaces: now drops stale surfaces of a different size; the regression test checks the cache stays single-sized. Tests and my repro are still clean (0 crashes, where master crashes on the second flip).

@StarProxima

Copy link
Copy Markdown

Same crash here on 3.47.0 stable in a macOS app, same raster thread stack via EmbedderExternalViewEmbedder::SubmitFlutterView. For us it happens when entering native fullscreen, no Texture on screen. We turned Impeller off for now.

Went through the diff, the returnSurfaces: change makes sense to me. I'll try this branch with a local engine build on our app and report back.

@knopp this now follows your suggestion from Sep 12, mind taking another look?

@StarProxima

Copy link
Copy Markdown

Tried it on our app. Applied this PR on top of 3.47.0, built host_release arm64, app in release with Impeller on, same 1px shrink/restore trigger as your repro.

Stock 3.47.0 crashed in 10/10 runs within 1-7s, always right after The texture and its descriptor disagree about its size. With the PR, 0 crashes in 16 runs (flip every 0.3s and every 2.5s, up to 60s each, plus fullscreen toggling), no validation errors, and the new Could not wrap log never fired, so the cache change alone covers our case. Reverting the PR in the same checkout brings the crash back, 10/10.

macOS 26.6.2, M4 Pro.

cbracken
cbracken previously approved these changes Oct 2, 2026

@cbracken cbracken 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.

LGTM stamp from a Japanese personal seal

if (_surfaces.count > 0 && returnedSurfaces.count > 0 &&
!CGSizeEqualToSize(returnedSurfaces.firstObject.size, _surfaces.firstObject.size)) {
// Any cached surface size will match last requested size (see removeSurfaceForSize:). If the
// size of incoming surfaces don't match cached surface size it means they are stale and

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.

nit:

Suggested change
// size of incoming surfaces don't match cached surface size it means they are stale and
// size of incoming surfaces doesn't match cached surface size it means they are stale and

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 9b9198a, thanks!

/**
* Removes surface with given size from cache (if available) and returns it.
* All cached surfaces have the size of the last request; a request for a
* different size purges the cache first.

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.

Super pedantic wording nit:

Suggested change
* different size purges the cache first.
* All cached surfaces are the same size; a request for a different size
* purges the cache first.

Your test has an example of where the original phrasing is inaccurate.

  • Frame 1: empty cache. size A. allocates surfaceA1.
  • Frame 2: empty cache. size B. nothing to clear. allocates surfaceB1.
  • Present surfaceB1: returnSurfaces: called with @[surfaceA1].

So (somewhat pedantically) it's the size set either by the last request (if the cache wasn't empty) or by the first surface returned into an empty cache, which might be a frame behind.

This is the case after any size change (as well as the first frame); after that they all match the last requested size until the next size change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 9b9198a, thanks!

@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Oct 2, 2026
drusifer added a commit to drusifer/happening that referenced this pull request Oct 7, 2026
…nt state

Disable Impeller on macOS (flutter/flutter#192522), F-33 Countdown Swing
stories/task board, F-32 board archive, PRD update, version/pubspec
sync, persona state updates, and golden failure diffs from macOS run.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

@hayward-davidlondono hayward-davidlondono left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@knopp
knopp force-pushed the fix/macos-backbuffer-cache-wrong-size branch from 9b9198a to 98d9cb8 Compare October 14, 2026 15:06
@knopp knopp added the CICD Run CI/CD label Oct 14, 2026
AbdurahmanAlmehdi added a commit to AbdurahmanAlmehdi/x-path that referenced this pull request Oct 17, 2026
## Problem

XPath 1.0.0 (54) crashed on macOS 26.7.1 while it was in the background:
`EXC_BAD_ACCESS` at `0x0` on `io.flutter.raster`, in
`impeller::Canvas::SetupRenderPass()` (canvas.cc:1615), called from
`EmbedderExternalViewEmbedder::SubmitFlutterView`.

## Root cause: a Flutter engine bug

This is
[flutter/flutter#185394](flutter/flutter#185394),
"[Impeller] Crash on macOS when app is foregrounded" (open, P2,
team-macos). The macOS back-buffer cache (`FlutterBackBufferCache`) can
hand back a surface of the wrong size when the window's frame size
changes and then changes back (A → B → A). Examples are a display
hot-plug or reconfiguration, a Space or full-screen change, a window
drag, or bringing the app to the front. Impeller wraps the mismatched
`MTLTexture`, the texture invalidates itself, and the render target
silently drops it. `SetupRenderPass` then dereferences the null texture.

- Fix:
[flutter/flutter#192522](flutter/flutter#192522)
is still **open** (not merged). It is not in 3.47.7 (the latest stable,
per its CHANGELOG) or in the 3.50 beta.
- Also reported: #193095 (same stack) and #192829 (Cmd-Tab on macOS 27,
closed as a duplicate).

**Reproduced on our pinned SDK (3.47.6, release build).** This used a
minimal scratch app, not XPath. A Swift timer shrinks the window by 1 pt
and restores it every 2.5 s, which is the driver from #192522. The
Flutter side redraws a text once a second, the way `XpClock` does:

| Renderer | UI | Result |
|---|---|---|
| Impeller (default) | 1 s ticking text | **SIGSEGV after 5 s, at flip
#1.** The stack matches the field crash frame for frame (canvas.cc:1615
/ 372, dl_dispatcher.cc:1318, embedder_external_view_embedder.cc:376 /
598). |
| Impeller | still UI | survived 60 s (17 flips) |
| Skia (`FLTEnableImpeller=false`) | 1 s ticking text | **survived 90 s
(36 flips)**, with no validation errors and the log line `Using the Skia
rendering backend (Metal).` |

## Changes

1. **`macos/Runner/Info.plist`: `FLTEnableImpeller = false`** (with a
comment linking the issues). I checked the 3.47.6 engine source to
confirm macOS still reads this key: `FlutterDartProject.enableImpeller`
reads it, and `FlutterEngine runWithEntrypoint` then skips
`--enable-impeller=true` and uses Skia (Metal). XPath uses no
Impeller-only features (no fragment shaders, Flutter GPU or backdrop
filters). Goldens and widget tests are unaffected because `flutter test`
does not read `Info.plist`.
2. **`XpClock` follows the app lifecycle** using an
`AppLifecycleListener`. It ticks only while the app is `resumed`. When
the app is `inactive` (visible but not in front), `hidden` (occluded or
minimised) or `paused`, the timer stops. On resume, every live duration
is rebuilt at once against the current time. The new
`XpClock.isLive(context)` tells widgets whether the app is in front.
3. **`XpSpinner`** stands still while `!XpClock.isLive`, as it already
does under Reduce Motion.

Change 1 is the fix. Changes 2 and 3 mean XPath draws no frames of its
own while it is not in front (it used to draw one a second for the
clock, and 60 fps per spinner). That lowers the crash exposure and saves
CPU and battery. On its own it does **not** fix the engine bug: a
display change still causes a frame, and upstream reports idle apps
crashing too.

## Tests

`apps/xp_desktop/test/widgets/xp_clock_test.dart` drives the real
`flutter/lifecycle` platform channel, the same messages the macOS
embedder sends:
- `XpClock ticks every second while the app is in front`
- `XpClock stops while inactive|hidden|paused and catches up on resume`
- `XpClock a still clock stays still in front and away`
- `XpClock starts still when the app is already away`
- `XpClock without a clock above, the system time is live`
- `XpClock spinners stand still while the app is away`

## Trade-offs and follow-ups

- Skia is Flutter's legacy renderer on macOS. The Flutter docs warn that
the Impeller opt-out will be removed in a future release, and with Skia
there is no wide-gamut colour. Once a Flutter stable ships #192522,
remove the key (the Info.plist comment says so).
- While XPath is visible but not in front, elapsed times stay frozen
until it comes back to the front, and then catch up at once.
TimeLord2010 added a commit to TimeLord2010/soniox_transcriptor that referenced this pull request Oct 19, 2026
Impeller's back-buffer cache can hand a wrong-size surface to the raster
thread after the window/display size flip-flops (wake from sleep with an
external monitor, resize, fullscreen), which null-derefs in
impeller::Canvas::SetupRenderPass and kills the app with SIGSEGV on
io.flutter.raster.

Upstream: flutter/flutter#185394, fix in flutter/flutter#192522 (open,
not in any stable release yet). Re-enable FLTEnableImpeller once a
stable Flutter ships that fix.
@knopp
knopp force-pushed the fix/macos-backbuffer-cache-wrong-size branch from 98d9cb8 to a96ed38 Compare October 20, 2026 14:29
@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Oct 20, 2026
@knopp knopp added the CICD Run CI/CD label Oct 20, 2026
ShreyashKore added a commit to ShreyashKore/eagly that referenced this pull request Oct 21, 2026
The Mirror pane now takes iPhones. On macOS the picture comes from
QuickTime's capture route: once screen-capture devices are allowed,
macOS offers a USB iPhone as a camera (IosScreenCapturePlugin.swift, so
camera permission). Elsewhere it is CoreDevice screenshots, under two a
second.

Input goes through the CoreDevice HID services in Xcode 27's developer
disk image, the ones Device Hub uses (api/ios_mirror.rs, behind
IosMirrorService): live touch, keys as HID usages, Home / Lock / volume,
paste through the pasteboard service, and rotation. Without Developer
Mode and the disk image the mirror is view-only and says why.

What an iPhone 16e (iOS 26.6) showed, and the code now relies on:
- Starting the capture re-enumerates the phone on USB, so input
  connects only after the first frame.
- A drag that stops and then lets go flings unless the still finger
  wobbles a pixel; the input worker does that every 16 ms.
- The phone opens its per-session virtual touchscreen about 150 ms
  after connecting and queues or drops events sent earlier, so a session
  is handed out after 300 ms, and closing waits 100 ms.
- Send calls are synchronous bridge calls, so touches keep their order.

On iOS 27 the phone wants an open displayservice stream before it takes
HID events; the session opens one and drains it, untried on hardware.
The plugin holds its redraws back while the window changes, to dodge an
unfixed Flutter Impeller crash (flutter/flutter#192522).

test/device/ios_mirror_device_test.dart passes on the iPhone 16e.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…urfaces on return

Per review: instead of filtering wrong-size surfaces when a surface is
requested, keep the existing invariant that every cached surface has the size
of the last request and refuse returned surfaces of a different size. They are
from before the size changed and cannot be reused.
The doc comment claimed all cached surfaces have the size of the last
request. That is not strictly true right after a size change: the first
surface returned into an empty cache can be one frame behind. State the
invariant that actually holds (all cached surfaces share one size) and
fix subject-verb agreement in the returnSurfaces: comment.
@knopp
knopp force-pushed the fix/macos-backbuffer-cache-wrong-size branch from a96ed38 to e8b68ce Compare November 5, 2026 20:27
@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Nov 5, 2026
@knopp knopp added the CICD Run CI/CD label Nov 6, 2026
@knopp

knopp commented Nov 13, 2026

Copy link
Copy Markdown
Member

@cbracken, can you restart google tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: desktop Running on desktop CICD Run CI/CD engine flutter/engine related. See also e: labels. platform-macos Building on or for macOS specifically team-macos Owned by the macOS platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Impeller] Crash on macOS when app is foregrounded?

5 participants