Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion binding/SkiaSharp/SKPath.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,17 @@
// reads path.Handle directly and P/Invokes with it. If mutations have been batched
// into _builder but not yet flushed, base.Handle points at a stale native SkPath.
// Flushing in the getter keeps every reader — internal or external — honest.
//
// Skip the flush once disposal has begun. SKPathBuilder is itself an SKObject with
// its own finalizer, so on the finalizer thread it may already have been collected
// (its Handle is IntPtr.Zero, native pointer freed). Touching it here would call
// sk_pathbuilder_detach_path on a null/dangling handle. The pending mutations are
// going to be discarded with the path anyway, and DisposeNative cleans up _builder
// defensively.
public override IntPtr Handle {
get {
FlushBuilder ();
if (!IsDisposed)
FlushBuilder ();
return base.Handle;
}
protected set => base.Handle = value;
Expand Down Expand Up @@ -770,7 +778,7 @@

public SKPathVerb Next (Span<SKPoint> points)
{
if (points == null)

Check warning on line 781 in binding/SkiaSharp/SKPath.cs

View workflow job for this annotation

GitHub Actions / Build Site

Comparing a span to 'null' might be redundant, the 'null' literal will be implicitly converted to a 'Span<T>.Empty' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2265)
throw new ArgumentNullException (nameof (points));
if (points.Length != 4)
throw new ArgumentException ("Must be an array of four elements.", nameof (points));
Expand Down Expand Up @@ -811,7 +819,7 @@

public SKPathVerb Next (Span<SKPoint> points)
{
if (points == null)

Check warning on line 822 in binding/SkiaSharp/SKPath.cs

View workflow job for this annotation

GitHub Actions / Build Site

Comparing a span to 'null' might be redundant, the 'null' literal will be implicitly converted to a 'Span<T>.Empty' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2265)
throw new ArgumentNullException (nameof (points));
if (points.Length != 4)
throw new ArgumentException ("Must be an array of four elements.", nameof (points));
Expand Down
26 changes: 26 additions & 0 deletions tests/Tests/SkiaSharp/SKPathTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -725,5 +725,31 @@ public void TransformInPlaceWhenSrcEqualsDst()
}

#pragma warning restore CS0618

[SkippableFact]
public void PathWithPendingBuilderMutationsSurvivesFinalization()
{
// Reproduces a finalizer-ordering crash: SKPath holds a private SKPathBuilder
// for batched mutations. Both are SKObjects with their own finalizers, and the
// CLR may finalize the builder first. When the path's finalizer then accesses
// path.Handle, the override calls FlushBuilder, which calls
// sk_pathbuilder_detach_path on the builder's already-freed handle.
MakeAndAbandon();

CollectGarbage();
CollectGarbage();

// If the bug is present, the second collect crashes the finalizer thread on
// sk_pathbuilder_detach_path(IntPtr.Zero). Reaching here without an unhandled
// native exception means the disposal path skipped FlushBuilder correctly.

static void MakeAndAbandon()
{
var path = new SKPath();
path.MoveTo(0, 0);
path.LineTo(10, 10);
// drop both references; the builder is private and goes with it
}
}
}
}
Loading