Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

taublast
Copy link
Contributor

@taublast taublast commented Jun 13, 2025

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
Copy link
Contributor Author

taublast commented Jun 13, 2025

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()
Copy link
Contributor

@mattleibow mattleibow left a comment

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!

@github-project-automation github-project-automation bot moved this to Changes Requested in SkiaSharp Backlog Jun 19, 2025
@mattleibow
Copy link
Contributor

/azp run

Copy link

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

@mattleibow
Copy link
Contributor

/azp run

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

2 participants