[memory-leak] Fix CoTaskMem pixel-buffer leak in SKImage.Create raster failure path - #4455
Conversation
SKImage.Create(SKImageInfo) allocates an unmanaged pixel buffer with Marshal.AllocCoTaskMem and relies on the native raster image's release proc to free it. When sk_image_new_raster fails (e.g. a valid-BytesSize info Skia rejects), no image is created, the release proc never runs, and the buffer leaks. Free it explicitly on the failure path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…4476) Add regression-tracking benchmarks for common colour/interop paths (#4476) Derived from: #4370, #4385, #4442, #4453, #4455 CI: Track - Benchmarks run 29608575980 (green, all OS/roles) The permanent regression-tracking suite (SkiaSharp.Benchmarks.Tracking) landed recently alongside a wave of perf/agentic PRs, but it had no coverage for the managed colour/interop paths those PRs actually touch. This adds a small, curated set of trackers so a future regression (or the improvement itself) shows up as a step in the persisted per-OS time/allocation history. Following the suite's scaled-pyramid philosophy (a few small common-path trackers plus one broad composite, not a micro-benchmark per PR), four benchmarks are added under Tracking/Benchmarks/ (auto-linked into the source-mode project, so they also drive the PR column): * ColorMathBenchmark - managed SKColor->SKColorF (#4370) and SKPMColor PreMultiply/UnPreMultiply (#4385). Zero-alloc; the ToColorF/ToColor ratio (the reverse operator stays native) is a managed-regression signal even on noisy shared runners. These two ports are the real, dashboard-visible speed wins - their time lines should drop when the ports merge. * RasterImageLifecycleBenchmark - create+dispose churn of SKImage.Create raster (#4455) and SKData.Create with a managed release proc (#4453). Allocation is the tracked signal for that object-lifecycle area. * RuntimeEffectShaderBenchmark - a per-frame animated SkSL shader: build uniforms (the SKColor uniform pays the #4370 convert), ToShader, draw. Covers the uniforms lifecycle whose SKData leak #4442 touched. * SceneRenderBenchmark - one broad frame (transforms, linear+radial gradients, filled/stroked primitives, a built path, a scaled image draw, a colour-filter layer, clipping, managed colour maths) so many unrelated future optimizations each nudge it - a merge indicator rather than a micro-benchmark. Scope notes: the failure-path leak fixes (#4453, #4455), the native uniforms leak (#4442), and the UAF/double-free fixes (#4372, #4468) are correctness/native-memory changes that a throughput + managed-allocation dashboard largely will not register; they are represented here by area, not as leak assertions (those belong in soak/ leak tests). #4372 and #4468 are intentionally not benchmarked (niche debug canvases; HarfBuzz lives in a separate assembly the tracking project does not reference). All benchmarks use public API only (the project references the nightly NuGet, not internal SkiaApi), are deterministic and machine-independent (fixed seeds, no fonts or external content), and compile against every benchmarked role - verified against the 3.119.4 baseline (prev-major) and 4.151 nightly, with only SKPath/SKPathBuilder behind the existing #if. A short local run and the full CI matrix (Linux/Windows/ macOS, all version roles plus the source-built PR column) emit time and allocation data for every case. Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
📦 Try the packages from this PRWarning Do not run these scripts without first reviewing the code in this PR. Step 1 — Download the packages bash / macOS / Linux: curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4455PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4455"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4455/packages --name skiasharp-pr-4455More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4455 |
|
📖 Documentation Preview The documentation for this PR has been deployed and is available at: 🔗 View Staging Site This preview will be updated automatically when you push new commits to this PR. This comment is automatically updated by the documentation staging workflow. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an unmanaged memory leak in SKImage.Create(SKImageInfo) when raster image creation fails: the method allocates a CoTaskMem pixel buffer, but if the native factory returns null the release-proc never runs, so the buffer must be freed explicitly. It also adds a regression test to catch the leak.
Changes:
- Free the CoTaskMem pixel buffer when
sk_image_new_rasterreturns a null image. - Add a regression test that repeatedly hits the failure path and asserts process private memory doesn’t grow significantly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| binding/SkiaSharp/SKImage.cs | Frees the unmanaged pixel buffer on native raster-create failure to prevent leaks. |
| tests/Tests/SkiaSharp/SKImageTest.cs | Adds a regression test that exercises the failure path repeatedly and checks for memory growth. |
The FailedRasterCreateDoesNotLeakPixelBuffer regression test measures the leak via System.Diagnostics.Process.PrivateMemorySize64. That API is not supported on WASM (no System.Diagnostics.Process at all), iOS, or Mac Catalyst, so the test threw PlatformNotSupportedException and failed the Tests WASM (Linux) and Tests iOS (macOS) legs (and Mac Catalyst). Guard the test with SkipOnPlatform(IsBrowser || IsIOS || IsMacCatalyst), matching the existing platform-skip pattern in this suite. The SKImage.Create fix itself is platform-agnostic and remains exercised by the desktop and Android test legs, where PrivateMemorySize64 is available. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4649389c-a7bd-443d-8c26-7c6b16cda819
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
📊 SkiaSharp benchmarks — PR #4455⭐ this PR (full source build) vs 🌙 nightly · Linux · Windows · macOS
Highlights⏱️ Time — 🔴 28 slower · 🟢 35 faster
Full per-OS benchmark deltasLinux⏱️ Time (vs 🌙 nightly
Windows⏱️ Time (vs 🌙 nightly
macOS⏱️ Time (vs 🌙 nightly
|
📦 Artifact size reportPackages from this PR (build Total Packages
+37 package(s) unchanged (< 50.0 KB). Per-file changes
|
| File | Size |
|---|---|
runtimes/win-x86/native/libHarfBuzzSharp.pdb |
22.2 MB → 21.9 MB (🟢 −272.0 KB) |
runtimes/win-x64/native/libHarfBuzzSharp.pdb |
22.0 MB → 21.8 MB (🟢 −240.0 KB) |
runtimes/win-arm64/native/libHarfBuzzSharp.pdb |
22.3 MB → 22.3 MB (🟢 −8.0 KB) |
SkiaSharp.NativeAssets.NanoServer
| File | Size |
|---|---|
runtimes/win-x64/native/libSkiaSharp.pdb |
68.8 MB → 70.2 MB (🔴 +1.5 MB) |
🧬 win-x64 (runtimes/win-x64/native/libSkiaSharp.dll) |
9.6 MB → 10.2 MB (🔴 +596.5 KB) |
SkiaSharp.NativeAssets.WinUI
| File | Size |
|---|---|
runtimes/win-x86/native/libGLESv2.pdb |
67.3 MB → 67.0 MB (🟢 −240.0 KB) |
runtimes/win-x64/native/libGLESv2.pdb |
66.6 MB → 66.4 MB (🟢 −208.0 KB) |
runtimes/win-arm64/native/libGLESv2.pdb |
64.5 MB → 64.3 MB (🟢 −192.0 KB) |
runtimes/win-x64/native/libEGL.pdb |
6.4 MB → 6.3 MB (🟢 −32.0 KB) |
runtimes/win-x86/native/libEGL.pdb |
6.5 MB → 6.5 MB (🟢 −16.0 KB) |
runtimes/win-arm64/native/SkiaSharp.Views.WinUI.Native.pdb |
67.7 MB → 67.7 MB (🟢 −8.0 KB) |
runtimes/win-x64/native/SkiaSharp.Views.WinUI.Native.pdb |
68.0 MB → 68.0 MB (🟢 −8.0 KB) |
🧬 win-x64 (runtimes/win-x64/native/libGLESv2.dll) |
4.9 MB → 4.9 MB (🔴 +2.0 KB) |
🧬 win-arm64 (runtimes/win-arm64/native/libGLESv2.dll) |
4.4 MB → 4.4 MB (🔴 +1.0 KB) |
Informational only — this never blocks the PR. Native binaries are labelled by os/arch.
The fix
SKImage.Create(SKImageInfo)allocates an unmanaged pixel buffer withMarshal.AllocCoTaskMemand relies on the native raster image's release proc (SKImageRasterReleaseProxyForCoTaskMem, which callsMarshal.FreeCoTaskMem) to free it. That proc only runs when the native image is destroyed — so whensk_image_new_rasterreturns null (a valid-BytesSizeinfo that Skia rejects, e.g.Rgba8888+AlphaType.Unknown), no image is created, the proc never fires, and the buffer leaks.The fix frees the buffer explicitly on the null-handle path — the idiomatic focus-area-10 pattern (return
nullon failure, but release any allocation made along the way):Managed-C# only (
binding/SkiaSharp/SKImage.cs), no public signature change — ABI stable. The factory still returnsnullon failure (no exception-surface change).Proof (red → green)
New regression test
tests/Tests/SkiaSharp/SKImageTest.cs::FailedRasterCreateDoesNotLeakPixelBufferdrives 200 failing 4 MBCreatecalls and asserts private memory does not grow by a large fraction of what was allocated.SKImageTestclass green (45 passed / 7 hardware-skipped).Commands (single-TFM, prebuilt natives via
externals-download):Scope note
Framework bug, empirically proven. Managed-C# only, ABI-safe, internal-only change. Distinct from open PR #4453 (GCHandle leaks in
FromPixels/FromTexture/SKSurface.Create/SKData.Create) — that PR does not touchSKImage.Create(SKImageInfo)or the CoTaskMem pixel buffer.Labels:
tenet/performance+perf/memory-leak.Fixes #4454