Visitar URL original
fix(imaging): Don't cache IgnoreImageCache loads (backport #25100) by agneszitte · Pull Request #25911 · unoplatform/uno · GitHub
Skip to content

fix(imaging): Don't cache IgnoreImageCache loads (backport #25100) - #25911

Merged
agneszitte merged 1 commit into
servicing/6.8from
dev/agzi/backport-25100-ignoreimagecache-6.8
Nov 2, 2026
Merged

agneszitte merged 1 commit into
servicing/6.8from
dev/agzi/backport-25100-ignoreimagecache-6.8

Conversation

@agneszitte

Copy link
Copy Markdown
Member

GitHub Issue: Related to #25098

PR Type:

🐞 Bugfix

What changed? 🚀

Manual backport of #25100 to servicing/6.8, for 6.7 SR3 (part of the fixes for the WebAssembly crashes while scrolling).

  • Before: on Skia, a BitmapImage with CreateOptions="IgnoreImageCache" still went into the static image cache, so its decoded image stayed in memory. An app that shows many large images this way can run out of memory on WebAssembly, and the app terminates.
  • After: images loaded with IgnoreImageCache are no longer kept in the cache, and they never replace a cache entry that other images share.
  • Why not Mergify: on 6.x this code is in BitmapImage.skia.cs, and the test goes under Tests/Windows_UI_Xaml_Media_Imaging. The changed lines are the same as on master.

PR Checklist ✅

Local validation (Skia Desktop, Windows)

Runtime tests on this branch, filtered to Tests.Windows_UI_Xaml_Media_Imaging and Given_Image: 52 of 54 passed, including the new Given_BitmapImage.When_IgnoreImageCache_Then_Not_Added_To_Cache.

  • Given_WriteableBitmap.When_WriteableBitmap_Assigned_With_Data_Present failed on its pixel check. It doesn't use BitmapImage.
  • Given_RenderTargetBitmap.When_Render_Border_GetPixelsAsync was inconclusive: it needs 100% display scaling.

🤖 Generated with Claude Code

`IgnoreImageCache` skipped the lookup in the static `_bitmapImageCache`
but still added its load task to it, so the decoded image was retained
by the cache exactly as if the option were not set. Gate the add (and
the cancellation-removal registration) on the option too, so the image
lives only as long as the `BitmapImage` that produced it and never
replaces an entry other consumers already share.

Fixes #25098

Backport note: on 6.x the code is in BitmapImage.skia.cs and the test
goes under Tests/Windows_UI_Xaml_Media_Imaging. The changed lines are
the same as on master.

(cherry picked from commit 7c1c364)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@agneszitte

Copy link
Copy Markdown
Member Author

@Mergifyio backport release/stable/6.7

@github-actions github-actions Bot added the area/skia ✏️ Categorizes an issue or PR as relevant to Skia label Nov 2, 2026
@mergify

mergify Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

backport release/stable/6.7

✅ Backports have been created

Details

@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @agneszitte's task in 1m 22s —— View job


Review: PR #25911 — fix(imaging): Don't cache IgnoreImageCache loads (backport)

  • Gather context (diff vs origin/servicing/6.8, read changed files)
  • Review BitmapImage.skia.cs changes against AGENTS.md rules
  • Review new runtime test Given_BitmapImage.cs
  • Post inline findings (none found)
  • Post summary verdict

Verdict: ✅ Ship

Issues found: 0 (no inline comments posted)

Summary

This is a faithful manual backport of master PR #25100 (confirmed via the cherry picked from commit 7c1c36477e... trailer in the commit message) onto servicing/6.8, adapted only for the file's location on this branch (BitmapImage.skia.cs vs. the master path) as the PR description states.

  • Root cause, not a guard: the bug was that the cache-Add gate only checked FeatureConfiguration.Image.EnableBitmapImageCache, ignoring CreateOptions.HasFlag(BitmapCreateOptions.IgnoreImageCache) — so IgnoreImageCache loads skipped the cache lookup but still populated the cache on write. The fix correctly gates both the _bitmapImageCache.Add(...) call and the paired cancellation-removal ct.Register(...) on !ignoreCache, which addresses the actual defect rather than adding a defensive check elsewhere.
  • Test hook pattern (GetCachedImageDataTaskForTesting) is consistent with existing internal *ForTesting hooks already used elsewhere in Uno.UI (e.g. TextBox.skia.cs, InputManager.Pointers.Managed.cs), and InternalsVisibleTo already grants Uno.UI.RuntimeTests/.Skia access, so no new exposure.
  • Test coverage: the new runtime test is correctly scoped with #if __SKIA__ (matching the .skia.cs production file), uses distinct DecodePixelWidth values per case to avoid cross-test cache collisions, and asserts both the negative case (ignored load never cached) and the non-regression case (an ignored load doesn't clobber an existing shared cache entry, via Assert.AreSame). This is a genuine red→green regression test for the fixed scenario, not a guard-only check.
  • Cleanup (WindowHelper.WindowContent = null, restoring FeatureConfiguration.Image.EnableBitmapImageCache) is correctly wrapped in finally.
  • No event/async-void/cancellation-discipline issues, no public API surface changes, no XAML/SamplesApp files touched.

I did not build or run the test suite locally in this sandboxed review; the PR description already includes local Skia Desktop validation results (52/54 passed, including the new test, with the 2 unrelated failures explained).

@agneszitte
agneszitte merged commit 0375801 into servicing/6.8 Nov 2, 2026
13 of 23 checks passed
@agneszitte
agneszitte deleted the dev/agzi/backport-25100-ignoreimagecache-6.8 branch November 2, 2026 03:24
agneszitte added a commit that referenced this pull request Nov 2, 2026
…6.7/pr-25911

fix(imaging): Don't cache IgnoreImageCache loads (backport #25100) (backport #25911)
@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here:

mikernet added a commit to Singulink/uno that referenced this pull request Nov 14, 2026
Takes Uno's servicing/6.8 from 6951c2c up to 5254ace (42 pull
requests). Conflicts, by area:

IME and TextBox (unoplatform#25984, backport of unoplatform#25804; unoplatform#25952, backport of unoplatform#25619;
unoplatform#25976, backport of unoplatform#25818)
- Takes upstream's composition design (unoplatform#25804): the browser's input is
  the source of truth for the text, compositions included. This drops
  the composition parts of the unoplatform#24502 port (90746b8): its
  TypeScript composition handling, the StartIndex/TextChangePending
  event arguments, the IsReadOnly forwarding to the hidden input, and
  its Given_TextBox.BrowserSoftKeyboard tests.
- Keeps the non-IME browser fixes on top of it, in the form their
  master pull requests have on unoplatform#25804: Enter raised once per press and
  tracked by key code (unoplatform#25174, with Given_TextBox.BrowserEnterKey from
  master in place of the Enter tests that lived in the dropped file),
  the iOS refocus on tap (unoplatform#25185), pastes without text and isIOS from
  BrowserInputHelper.
- Moves the unoplatform#26005 MaxLength fixes onto unoplatform#25804's native input path, as
  master has them: a refused browser edit keeps the TextBox's selection,
  TextBoxView.UpdateTextFromNative reports whether the edit went in, and
  the IME keeps its range on the applied text (master be85699, which
  d226897 had ported onto the unoplatform#24502 IME). Adds that commit's browser
  and platform-applied IME tests.
- TextBox.pointers.skia.cs: upstream's file (unoplatform#25619 iOS touch editing,
  unoplatform#25818, unoplatform#25804), with the unoplatform#25126 port (the hold acts when it starts,
  a touch right-tap keeps the caret) in the form master's unoplatform#25126 branch
  has it on unoplatform#25619, including focusing before the iOS hold timer acts
  and its iOS not-focusable test.

Pointers (unoplatform#25976, backport of unoplatform#25818)
- Tapped and RightTapped are raised once the PointerReleased bubbled
  (unoplatform#25818), with the unoplatform#25732 port's window coordinates (2-argument
  routed event args, RawPosition for ContextRequested), as master's
  unoplatform#25732 branch has them.
- The release wrapper of unoplatform#25818 keeps the unoplatform#25126 port's
  StopContextMenuTimer at the start of the release.

DependencyObject (unoplatform#26142, backport of unoplatform#26123)
- Takes upstream, then the code of unoplatform#25839 (97a2f17), which replaces
  this branch's unoplatform#25102 ports (800247f, be41da6, fd40bf9,
  7546379). DependencyObjectStore.Binder.cs, .alc.cs,
  Application.Alc.cs, ResourceDictionary.alc.cs and their tests now
  match unoplatform#25839; this drops 800247f's own
  SingleParentNonFE_Parent_Removes_Value_Then_Next_Parent_Starts_Clean
  test, which unoplatform#25839 doesn't have.

Imaging (unoplatform#25597, unoplatform#25915, unoplatform#25911, unoplatform#25897)
- Keeps this branch's side: it has the later forms of these fixes
  (unoplatform#25124, unoplatform#25559, unoplatform#25633 and unoplatform#26370 ports), where upstream has the
  6.8-only unoplatform#25597 design and earlier snapshots. Upstream's shared
  download, IgnoreImageCache and failed-load fixes are covered by
  SharedImageDecode. Adds upstream's When_Source_Outlives_Brushes test.

ToolTip (unoplatform#26098, backport of unoplatform#25137 and unoplatform#26096)
- Keeps this branch's unoplatform#25137 port and adds unoplatform#26096 in master's form: the
  service releases only a ToolTip it opened, and leaves one the app
  opened during the show delay to the app. Keeps both sides' tests.

Flyout (unoplatform#26062, backport of unoplatform#26060)
- This branch had the same commits already; keeps the "Keep
  non-resizable flyouts in window" port and its tests.

Also removes a helper pair in Given_AccessibleTextBox that both sides
added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/skia ✏️ Categorizes an issue or PR as relevant to Skia

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants