Repository navigation
[macOS] Fix back buffer cache returning a wrong-size surface (Impeller null deref in Canvas::SetupRenderPass) - #192522
Conversation
There was a problem hiding this comment.
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.
|
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 |
|
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;
} |
|
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: |
|
Same crash here on 3.47.0 stable in a macOS app, same raster thread stack via Went through the diff, the @knopp this now follows your suggestion from Sep 12, mind taking another look? |
|
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 macOS 26.6.2, M4 Pro. |
| 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 |
There was a problem hiding this comment.
nit:
| // 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 |
| /** | ||
| * 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. |
There was a problem hiding this comment.
Super pedantic wording nit:
| * 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.
…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]>
9b9198a to
98d9cb8
Compare
## 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.
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.
98d9cb8 to
a96ed38
Compare
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]>
…shing in Impeller
…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.
a96ed38 to
e8b68ce
Compare
|
@cbracken, can you restart google tests? |

On macOS with Impeller, the raster thread can crash in
impeller::Canvas::SetupRenderPass()(color0.texture->GetSize()on a null texture) viaEmbedderExternalViewEmbedder::SubmitFlutterView. It reproduces reliably in a video-player app by toggling native fullscreen (NSWindow toggleFullScreen:) while aTexturewidget 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.MakeRenderTargetFromBackingStoreImpellerthen wraps anMTLTexturewhose real size disagrees with the requested backing-store size. The failure is silent all the way down:TextureMTLinvalidates itself (The texture and its descriptor disagree about its size.— aVALIDATION_LOG, compiled out in release).RenderTarget::SetColorAttachmentdrops the invalid attachment without logging.Canvas::SetupRenderPassreadsGetColorAttachment(0)(a defaultColorAttachment{}) and dereferences the null texture.Fix
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, sinceremoveSurfaceForSize: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 onremoveSurfaceForSize:now states the invariant).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::RenderFlutterContentsguards 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 realFlutterSurfaceManager. 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 (SIGABRTon the sizeDCHECKinLayer::RenderFlutterContentsImpellerin 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) andembedder_unittestsEmbedderTest.*(96 + 8 skipped) pass locally onhost_debug_unopt_arm64.Reproduction
Standalone app: https://github.com/abdelaziz-mahdy/flutter-macos-backbuffer-repro (stock
flutter createmacOS 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 onhost_debug_unopt_arm64, Apple M2 Pro, macOS 26.6.2:4e6768eca70(master, unfixed)The texture and its descriptor disagree about its size.thenCheck failed: render_target_->GetRenderTargetSize() == c->GetRenderSurfaceSize()onio.flutter.raster. With a temporary log insurfaceForSize:the handoff is visible:req=1600x1200 REUSED got=1598x1198.a8073db1e1d)cec7d27aa2c).Plain fullscreen toggling (the trigger we hit in production, with a video
Textureon 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
///).