Fix finalizer crash in SKPath when SKPathBuilder is collected first#3796
Merged
Merged
Conversation
Contributor
📦 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 -- 3796PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 3796"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-3796/packages --name skiasharp-pr-3796More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-3796 |
Contributor
|
/azp run |
|
No pipelines are associated with this pull request. |
|
here is verbatim crash: Environment:
Crash (identical in Debug and Release, ~20-30s after window opens, no Fatal error. The app code never directly constructs SKPath — these are paths created |
ramezgerges
force-pushed
the
dev/fix-skpath-finalizer-crash
branch
from
April 30, 2026 14:46
0118d89 to
9db03cf
Compare
|
No pipelines are associated with this pull request. |
Repro from a downstream app on 3.119: the finalizer thread crashed in
sk_pathbuilder_detach_path with this stack:
SkiaApi.sk_pathbuilder_detach_path(IntPtr)
SKPath.FlushBuilder()
SKPath.get_Handle()
SKNativeObject.Dispose(Boolean)
SKObject.Dispose(Boolean)
SKPath.Dispose(Boolean)
SKNativeObject.Finalize()
SKPath holds a private SKPathBuilder for batched mutations and overrides
Handle to flush pending mutations on every read. SKPathBuilder is itself
an SKObject with its own finalizer and the CLR makes no guarantee about
finalization order — when the builder finalizes first, its native
pathbuilder is freed and its Handle is set to IntPtr.Zero. The path's
finalizer then runs SKNativeObject.Dispose(false), which reads Handle
on line 269 to check `!= IntPtr.Zero`. That dispatches into our
overridden getter, FlushBuilder calls
sk_pathbuilder_detach_path(_builder.Handle), and the C side dereferences
nullptr (or a dangling pointer if the same address was reallocated).
Skip FlushBuilder once disposal has begun. The pending builder mutations
are about to be discarded along with the path either way, and
DisposeNative still cleans up _builder defensively (the CAS in
SKNativeObject.Dispose makes a second Dispose a no-op if the builder
already finalized).
Added a regression test that deterministically reproduces the crash
without the fix (Test host process crashed) and passes with it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ramezgerges
force-pushed
the
dev/fix-skpath-finalizer-crash
branch
from
May 1, 2026 17:34
9db03cf to
cf50c6f
Compare
|
No pipelines are associated with this pull request. |
mattleibow
enabled auto-merge (squash)
May 5, 2026 15:10
mattleibow
disabled auto-merge
May 5, 2026 15:10
mattleibow
added a commit
that referenced
this pull request
Jul 3, 2026
Expand the skill's scan taxonomy from 6 to 12 families, each backed by a real historical SkiaSharp leak fix (finalizer/collection ordering #3796/#3291, Clone double-free #2904, disposing native statics #1863/#4080/#1224, field-not-nulled #1256/#1344, stream/callback/delegate-proxy lifetime #3589/#2916/#996, allocation-failure #1784/#1642). Broaden Phase 1.3 de-dup to search by api/type name (real leaks are filed as [BUG], not [memory-leak]) and add the Blob.FromStream / PR #3473 worked example. Extend the Phase 3.2 fix table to cover all 12 families. Document two verified, un-filed family-6 candidates surfaced by a model-diverse scan (SKRegion.SpanIterator missing parent ref; SKPixmap.ExtractSubset/With* not propagating pixelSource). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
Repro from a downstream app on 3.119: the finalizer thread crashed in sk_pathbuilder_detach_path with this stack:
SKPath holds a private SKPathBuilder for batched mutations and overrides Handle to flush pending mutations on every read. SKPathBuilder is itself an SKObject with its own finalizer and the CLR makes no guarantee about finalization order — when the builder finalizes first, its native pathbuilder is freed and its Handle is set to IntPtr.Zero. The path's finalizer then runs SKNativeObject.Dispose(false), which reads Handle on line 269 to check
!= IntPtr.Zero. That dispatches into our overridden getter, FlushBuilder callssk_pathbuilder_detach_path(_builder.Handle), and the C side dereferences nullptr (or a dangling pointer if the same address was reallocated).
Skip FlushBuilder once disposal has begun. The pending builder mutations are about to be discarded along with the path either way, and DisposeNative still cleans up _builder defensively (the CAS in SKNativeObject.Dispose makes a second Dispose a no-op if the builder already finalized).
Added a regression test that deterministically reproduces the crash without the fix (Test host process crashed) and passes with it.
Bugs Fixed
API Changes
None.
Behavioral Changes
None.
Required skia PR
None.
PR Checklist