Don't call `skia_image()` for raster-owned images in `DlImage::Equals` by b-luk · Pull Request #196473 · flutter/flutter

b-luk · 2026-11-12T21:26:45Z

Fixes #195888

DlImage::Equals compared two Skia images by comparing their skia_image() results. Calling skia_image() on a DlDeferredImageGPUSkia DlImage is not allowed on the UI thread:

flutter/engine/src/flutter/lib/ui/painting/display_list_deferred_image_gpu_skia.h

Lines 46 to 51 in 3b04b3b

    
           // |DlImageSkia| 
        
           // This method is only safe to call from the raster thread. 
        
           // Callers must not hold long term references to this image and 
        
           // only use it for the immediate painting operation. It must be 
        
           // collected on the raster task runner. 
        
           sk_sp<SkImage> skia_image() const override;

DlImage::Equals could get called on the UI thread with a DlDeferredImageGPUSkia. On Skia, Picture.toImageSync and Scene.toImageSync return DlDeferredImageGPUSkia images. If this is used as a Paint's ColorSource, DisplayListBuilder's setColorSource calls NotEquals on it, which calls its DlImage::Equals method. This ends up causing crashes: #195888.

Fix

Exits early without calling skia_image() for raster-owned Skia images, which is exactly DlDeferredImageGPUSkia.

Exiting early with false is fine because In DlImage::Equals, the skia_image() equality comparison would always be false for this case: skia_image() for DlDeferredImageGPUSkia generates a new SkImage on each call, which will always compare false to other calls. So for DlDeferredImageGPUSkia, callingskia_image() is not needed and DlImage::Equals can rely on the existing pointer comparison to check for equality, just like what DlImage::Equals does for Impeller images.

I manually tested with the code sample from the issue. I could reliably repro the crash without this fix, and the crash does not occur with this fix.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

gemini-code-assist

Code Review

This pull request updates DlImage::Equals in dl_image.cc to return false if either compared image has an owning context of OwningContext::kRaster. This ensures that Skia raster-owned images rely on pointer equality and prevents accessing their underlying Skia images. Additionally, a new unit test file dl_image_unittests.cc is introduced to verify this behavior, and it is added to the build configuration in BUILD.gn. There are no review comments, and I have no feedback to provide.

gaaclarke

Why do we want to do this as opposed to returning an error state, synchronizing on the raster thread, or offloading the work the is using this to the raster thread?

gaaclarke · 2026-11-12T22:03:55Z

+// Stands in for an image such as DlDeferredImageGPUSkia: it is safe to hold
+// on the UI thread, but it is owned by the raster context and so its SkImage
+// may only be created and released on the raster thread.
+class RasterOwnedTestImage final : public DlImageSkia {


Can you use gmock for this please? I'm not sure why the AI's always jump to this.

gaaclarke · 2026-11-12T22:09:49Z

+
+  // Raster owned images have pointer equality.
+  EXPECT_TRUE(image1->Equals(image1));
+  EXPECT_FALSE(image1->Equals(image2));


This kind of breaks the semantics of what Equals is supposed to mean. What are the downstream ramifications of this?

Compared to the existing behavior, I don't think this is changing the semantics of what Equals means.

Each DlDeferredImageGPUSkia owns its own GrBackendTexture or SkImage which backs its image. The backing image is never the same for two different DlDeferredImageGPUSkia objects. So they never compare equal. I think that's what we want, and it makes sense semantically.

The existing behavior mostly matches this. The equality check generates the SkImages through the skia_image() calls. If the images are texture-backed, skia_image() generates a new SkImage to wrap the backing texture. For the CPU SkImage-backed case, the SkImage owned by the DlDeferredImageGPUSkia is returned. For both of these cases, the skia_image() of two different DlDeferredImageGPUSkia objects are never equal.

With the PR, we simply return false for this without needing to make the skia_image() call. This behaves the same as the existing behavior, with one exception. The exception is when skia_image() returns null. This happens if the deferred image snapshot is not yet complete. With the existing behavior, two DlDeferredImageGPUSkia whose snapshots have not completed will check null == null and return true. This seems incorrect, since the deferred images will later resolve into two different images. With the new behavior of the PR, this returns false.

Okay, that sounds good to me. Can we write a docstring for DlImage::Equals to avoid this in the future?

Done. I added a docstring that explains the Equals behavior, and also added a comment before the actual skia_image() calls to warn about calling it from the UI thread.

b-luk

Why do we want to do this as opposed to returning an error state, synchronizing on the raster thread, or offloading the work the is using this to the raster thread?

Returning an error state: We don't want an error because comparing DlImages is a valid operation regardless of what thread the comparison runs on and whether the images are raster-thread-owned.
Synchronizing on the raster thread or offloading the work to the raster thread: As I mention in my comment below, equality for DlDeferredImageGPUSkia doesn't depend on the resolved images. The resolved image for DlDeferredImageGPUSkia objects are always backed by different objects, so equality will always be false for two DlDeferredImageGPUSkia objects. This PR matches the existing behavior. If we wait to resolve the image, we're slowing things down for a result that we don't actually need to make the equality check.

b-luk · 2026-11-12T22:53:27Z

+// Stands in for an image such as DlDeferredImageGPUSkia: it is safe to hold
+// on the UI thread, but it is owned by the raster context and so its SkImage
+// may only be created and released on the raster thread.
+class RasterOwnedTestImage final : public DlImageSkia {


b-luk · 2026-11-13T00:33:16Z

+
+  // Raster owned images have pointer equality.
+  EXPECT_TRUE(image1->Equals(image1));
+  EXPECT_FALSE(image1->Equals(image2));


Compared to the existing behavior, I don't think this is changing the semantics of what Equals means.

Each DlDeferredImageGPUSkia owns its own GrBackendTexture or SkImage which backs its image. The backing image is never the same for two different DlDeferredImageGPUSkia objects. So they never compare equal. I think that's what we want, and it makes sense semantically.

The existing behavior mostly matches this. The equality check generates the SkImages through the skia_image() calls. If the images are texture-backed, skia_image() generates a new SkImage to wrap the backing texture. For the CPU SkImage-backed case, the SkImage owned by the DlDeferredImageGPUSkia is returned. For both of these cases, the skia_image() of two different DlDeferredImageGPUSkia objects are never equal.

With the PR, we simply return false for this without needing to make the skia_image() call. This behaves the same as the existing behavior, with one exception. The exception is when skia_image() returns null. This happens if the deferred image snapshot is not yet complete. With the existing behavior, two DlDeferredImageGPUSkia whose snapshots have not completed will check null == null and return true. This seems incorrect, since the deferred images will later resolve into two different images. With the new behavior of the PR, this returns false.

gaaclarke

LGTM, nice find. I do have one request though about adding missing documentation to make this more clear.

gaaclarke · 2026-11-13T00:59:49Z

+
+  // Raster owned images have pointer equality.
+  EXPECT_TRUE(image1->Equals(image1));
+  EXPECT_FALSE(image1->Equals(image2));


Okay, that sounds good to me. Can we write a docstring for DlImage::Equals to avoid this in the future?

gaaclarke

lgtm! thanks b-luk

gaaclarke · 2026-11-13T01:27:08Z

  virtual std::optional<std::string> get_error() const;

+  //----------------------------------------------------------------------------
+  /// @brief      Equality check for `DlImage` objects.


no action required: It irks me that AI picked up this formatting doxygen. We had about 50/50 none of that leading spacing. We should yank that at some point, it's just a pain to edit manually.

It matches the spacing of the rest of the file, so I think it's the most consistent fit for the comment added here. If we do want to rip it out, we can do it consistently across the codebase.

auto-submit · 2026-11-13T02:45:30Z

autosubmit label was removed for flutter/flutter/196473, because - The status or check suite Presubmit has failed. Please fix the issues identified (or deflake) before re-applying this label.

…13653) Manual roll Flutter from 9c427b5d7b1c to 1b9585481095 (32 revisions) Manual roll requested by [email protected] flutter/flutter@9c427b5...1b95854 2026-11-13 [email protected] Roll pub packages (flutter/flutter#196541) 2026-11-13 [email protected] Do not route PRs for review without cla, convert to draft (flutter/flutter#196341) 2026-11-13 [email protected] Roll Skia from d87a6c04da74 to 11f46cfe8838 (13 revisions) (flutter/flutter#196533) 2026-11-13 [email protected] [flutter_tools, flutter_test] Deprecate --experimental-faster-testing (flutter/flutter#196123) 2026-11-13 [email protected] [flutter_tools] Handle RPCError in ScreenshotCommand.runSkia (flutter/flutter#196132) 2026-11-13 [email protected] Roll Dart SDK from a8b961fd48bb to c91e8b6674e2 (2 revisions) (flutter/flutter#196502) 2026-11-13 [email protected] Roll Skia from 0c5ce4960cef to d87a6c04da74 (12 revisions) (flutter/flutter#196497) 2026-11-13 [email protected] Don't call `skia_image()` for raster-owned images in `DlImage::Equals` (flutter/flutter#196473) 2026-11-13 [email protected] [flutter_tools] Require FlutterCommandRunner DI fields and extract findCommandName (flutter/flutter#196331) 2026-11-13 [email protected] [web] Unify ui.Canvas behind a shared EngineCanvas frontend (flutter/flutter#196348) 2026-11-13 [email protected] Link to MediaQuery.displayFeaturesOf from DisplayFeatures (flutter/flutter#193728) 2026-11-13 [email protected] Removes reference to unnecessary ensureSemantics call in doc (flutter/flutter#195831) 2026-11-13 [email protected] [flutter_tools] Register trackWidgetCreation in AppleBuildOptionsBundle (flutter/flutter#196078) 2026-11-12 [email protected] [Android] Skip the HardwareBuffer defect fallback for platform views (flutter/flutter#193441) 2026-11-12 [email protected] Fix supports_decal check in framebuffer_blend.frag (flutter/flutter#196227) 2026-11-12 [email protected] [ios] Update the screen in didUpdateEffectiveGeometry (flutter/flutter#195508) 2026-11-12 [email protected] [flutter_tools] Add `Cache-Control` headers for web test assets (flutter/flutter#196414) 2026-11-12 [email protected] Roll Skia from 3b71ec08218f to 0c5ce4960cef (16 revisions) (flutter/flutter#196472) 2026-11-12 [email protected] [web] Render bounded ImageFilter.blur on wimp (flutter/flutter#196081) 2026-11-12 [email protected] Roll Dart SDK from 69567bd3c427 to a8b961fd48bb (2 revisions) (flutter/flutter#196464) 2026-11-12 [email protected] [engine] Release the Skia OpenGL resource context on shutdown (flutter/flutter#193592) 2026-11-12 [email protected] [web] Fix decoding of still AVIF images on Chrome (flutter/flutter#195958) 2026-11-12 [email protected] Gate SurfaceControl context setting on API 34+ (flutter/flutter#196394) 2026-11-12 [email protected] [flutter_tools] Only pass on a valid root tool in DASH__TOOL (flutter/flutter#196228) 2026-11-12 [email protected] Support GL_TEXTURE_EXTERNAL_OES embedder textures in Impeller (flutter/flutter#195857) 2026-11-12 [email protected] Stop building linux/wasm_release in local_engine_builds (flutter/flutter#196345) 2026-11-12 [email protected] Split flame out of Linux customer_testing (flutter/flutter#196347) 2026-11-12 Jiwon Lee [Windows] Fix Space input after Alt+Space with previous Microsoft IME (flutter/flutter#192105) 2026-11-12 [email protected] [flutter_tools] Add ToolExtensionException, exit() guard, graceful shutdown, and base modifiers (flutter/flutter#195908) 2026-11-12 [email protected] Roll Packages from 653628e to 14f9933 (9 revisions) (flutter/flutter#196450) 2026-11-12 [email protected] Roll Dart SDK from e5add7a25b32 to 69567bd3c427 (2 revisions) (flutter/flutter#196437) 2026-11-12 [email protected] Roll Skia from 9aa2513a4ac9 to 3b71ec08218f (1 revision) (flutter/flutter#196436) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 ...

commit

319eff0

flutter-dashboard Bot added the CICD Run CI/CD label Nov 12, 2026

github-actions Bot added the engine flutter/engine related. See also e: labels. label Nov 12, 2026

gemini-code-assist Bot reviewed Nov 12, 2026

View reviewed changes

Merge branch 'master' into dlimage-equals-raster

b365892

b-luk requested a review from gaaclarke November 12, 2026 21:32

gaaclarke requested changes Nov 12, 2026

View reviewed changes

use gmock

34d2f88

b-luk commented Nov 13, 2026

View reviewed changes

b-luk requested a review from gaaclarke November 13, 2026 00:47

gaaclarke requested changes Nov 13, 2026

View reviewed changes

add comments

8879b7d

b-luk requested a review from gaaclarke November 13, 2026 01:24

gaaclarke approved these changes Nov 13, 2026

View reviewed changes

b-luk mentioned this pull request Nov 13, 2026

[Skia][Android] Image shaders with toImageSync images abort the app in GrResourceCache (SIGTRAP) #195888

Closed

b-luk added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2026

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2026

b-luk added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2026

auto-submit Bot added this pull request to the merge queue Nov 13, 2026

Merged via the queue into flutter:master with commit b5f4dc4 Nov 13, 2026
26 checks passed

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2026

	// \|DlImageSkia\|
	// This method is only safe to call from the raster thread.
	// Callers must not hold long term references to this image and
	// only use it for the immediate painting operation. It must be
	// collected on the raster task runner.
	sk_sp<SkImage> skia_image() const override;

Conversation

b-luk commented Nov 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

Pre-launch Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-luk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit Bot commented Nov 13, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

b-luk commented Nov 12, 2026 •

edited

Loading