Skip to content

SKAutoCanvasRestore avoid crash at Dispose when canvas was already disposed#3291

Merged
mattleibow merged 5 commits into
mono:mainfrom
taublast:main
Jun 24, 2025
Merged

SKAutoCanvasRestore avoid crash at Dispose when canvas was already disposed#3291
mattleibow merged 5 commits into
mono:mainfrom
taublast:main

Conversation

@taublast

@taublast taublast commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Description of Change

Have catched this few times myself: might be my bad coding style, but nevertheless SKAutoCanvasRestore should not crash app if the canvas inside was already disposed for any reason whatever. So checking to avoid that inside SKAutoCanvasRestore Dispose method.

Bugs Fixed

SKAutoCanvasRestore could crash if GC-ed/disposed after the containing canvas native Handler was already released while canvas field still not NULL and Restore() would be called, then crash app accessing Handle 0.

EDIT: also added safety checks to mappers (second commit), after was running n Debug and catched landing into mapper code with a NULL handler parameter when navigating between pages with accelerated canvases. Would guess there want be any harm for adding those too to solve occasional very rare and random crashes.

  • Fixes

No known issues identified, still could be related to some unknown random crashes.

No Api changes, a small code change, please see 1 file change below.

Required skia PR

None.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Merged related skia PRs
  • Changes adhere to coding standard
  • Updated documentation

@taublast

taublast commented Jun 13, 2025

Copy link
Copy Markdown
Contributor Author

omg. might be the fix for the navigating away from page with skglview crash.

EDIT:

Yeah! It doesn't crash when i just comment this out! So when we merge i could uncomment :)

  //using (new SKAutoCanvasRestore(_retainedSurface.Canvas, true))
  {
      OnPaintSurface(new(_retainedSurface, _renderTarget, SurfaceOrigin, ColorType));
  }

Re-EDIT:

Ha just realized i can just use a custom-fixed SKAutoCanvasRestore meanwhile..

taublast added 2 commits June 17, 2025 12:07
System.NullReferenceException: Object reference not set to an instance of an object.
   at SkiaSharp.Views.Windows.SKXamlCanvas.Invalidate()

@mattleibow mattleibow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the continued work!

Comment thread binding/SkiaSharp/SKCanvas.cs Outdated
@github-project-automation github-project-automation Bot moved this to Changes Requested in SkiaSharp Backlog Jun 19, 2025
@mattleibow

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@mattleibow

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@mattleibow

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@mattleibow
mattleibow requested review from Copilot and mattleibow June 24, 2025 16:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds safety checks to prevent null reference and disposed-object crashes in several canvas and handler classes.

  • Guard Invalidate calls against null dispatchers in SKXamlCanvas
  • Add null-check guards in SKGLViewHandler and SKCanvasViewHandler mapping and invalidate methods
  • Update SKCanvas.Restore to verify the native handle before restoring and always clear the canvas reference

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
source/SkiaSharp.Views/SkiaSharp.Views.WinUI/SKXamlCanvas.cs Use null-conditional calls for DispatcherQueue/Dispatcher
source/SkiaSharp.Views.Maui/SkiaSharp.Views.Maui.Core/Handlers/SKGLView/SKGLViewHandler.iOS.cs Add null guard for handler.PlatformView in invalidate and mappers
source/SkiaSharp.Views.Maui/SkiaSharp.Views.Maui.Core/Handlers/SKGLView/SKGLViewHandler.Windows.cs Add null guards before calling Invalidate and mapping methods
source/SkiaSharp.Views.Maui/SkiaSharp.Views.Maui.Core/Handlers/SKGLView/SKGLViewHandler.MacCatalyst.cs Add null guards for invalidate and mapping methods
source/SkiaSharp.Views.Maui/SkiaSharp.Views.Maui.Core/Handlers/SKGLView/SKGLViewHandler.Android.cs Add null guards for invalidate and mapping methods
source/SkiaSharp.Views.Maui/SkiaSharp.Views.Maui.Core/Handlers/SKCanvasView/SKCanvasViewHandler.Windows.cs Add null guard around Invalidate and map methods
source/SkiaSharp.Views.Maui/SkiaSharp.Views.Maui.Core/Handlers/SKCanvasView/SKCanvasViewHandler.Tizen.cs Add null guards for invalidate and mapping calls
source/SkiaSharp.Views.Maui/SkiaSharp.Views.Maui.Core/Handlers/SKCanvasView/SKCanvasViewHandler.Apple.cs Add null guards for invalidate and mapping calls
binding/SkiaSharp/SKCanvas.cs Check Handle before RestoreToCount and always clear canvas
Comments suppressed due to low confidence (2)

binding/SkiaSharp/SKCanvas.cs:1082

  • Consider adding a unit test to verify that calling Restore() does not throw when canvas is null or canvas.Handle is IntPtr.Zero, ensuring this safety branch is covered.
			if (canvas != null && canvas.Handle != IntPtr.Zero) {

binding/SkiaSharp/SKCanvas.cs:1079

  • The XML doc for Restore() should be updated to note that it now safely handles cases where the underlying canvas has already been disposed or its handle is invalid.
		public void Restore ()

Comment on lines 54 to +56
public static void OnInvalidateSurface(SKGLViewHandler handler, ISKGLView view, object? args)
{
if (handler?.PlatformView == null)

Copilot AI Jun 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This null-check pattern is repeated across multiple handler methods; consider extracting a shared guard or helper method to reduce duplication and improve maintainability.

Suggested change
public static void OnInvalidateSurface(SKGLViewHandler handler, ISKGLView view, object? args)
{
if (handler?.PlatformView == null)
private static bool IsHandlerValid(SKGLViewHandler? handler) =>
handler?.PlatformView != null;
public static void OnInvalidateSurface(SKGLViewHandler handler, ISKGLView view, object? args)
{
if (!IsHandlerValid(handler))

Copilot uses AI. Check for mistakes.
@mattleibow
mattleibow merged commit 8c32855 into mono:main Jun 24, 2025
1 of 2 checks passed
@github-project-automation github-project-automation Bot moved this from Changes Requested to Done in SkiaSharp Backlog Jun 24, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants