SKAutoCanvasRestore avoid crash at Dispose when canvas was already disposed#3291
SKAutoCanvasRestore avoid crash at Dispose when canvas was already disposed#3291mattleibow merged 5 commits intomono:mainfrom
Conversation
|
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 |
System.NullReferenceException: Object reference not set to an instance of an object. at SkiaSharp.Views.Windows.SKXamlCanvas.Invalidate()
mattleibow
left a comment
There was a problem hiding this comment.
Thanks for the continued work!
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
There was a problem hiding this comment.
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
canvasis null orcanvas.Handleis 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 ()
| public static void OnInvalidateSurface(SKGLViewHandler handler, ISKGLView view, object? args) | ||
| { | ||
| if (handler?.PlatformView == null) |
There was a problem hiding this comment.
[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.
| 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)) |
Description of Change
Have catched this few times myself: might be my bad coding style, but nevertheless
SKAutoCanvasRestoreshould not crash app if the canvas inside was already disposed for any reason whatever. So checking to avoid that insideSKAutoCanvasRestoreDisposemethod.Bugs Fixed
SKAutoCanvasRestorecould crash if GC-ed/disposed after the containing canvas native Handler was already released whilecanvasfield still not NULL andRestore()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.
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