[BUG] System.AccessViolationException thrown when flushing canvas or swapping buffers

[bmitc]

Description

I am using GLFW for creating a window and SkiaSharp to draw to that window's OpenGL context. SkiaSharp suddenly throws System.AccessViolationException exceptions, often without an indication of where it occurred, but it seems to occur either at SKCanvas.flush() or when instructing GLFW to swap buffers.

The exception is thrown every run, but it is indeterminate how long it takes for the exception to be thrown.

The exception message in the Visual Studio "Exception Unhandled" window that pops up:

System.AccessViolationException: 'Attempted to read or write protected memory. This is often an indication that other memory is corrupt.'

Details after selecting "Copy Details" in this window:

System.AccessViolationException
  HResult=0x80004003
  Message=Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
  Source=<Cannot evaluate the exception source>
  StackTrace:
<Cannot evaluate the exception stack trace>

Error message printed to console:

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
Repeat 2 times:
--------------------------------
   at SkiaSharp.SkiaApi.sk_refcnt_safe_unref(IntPtr)
--------------------------------
   at SkiaSharp.SKObjectExtensions.SafeUnRef(SkiaSharp.ISKReferenceCounted)
   at SkiaSharp.SKNativeObject.Dispose(Boolean)
   at SkiaSharp.SKNativeObject.Finalize()

Code

I have written my own GLFW bindings in F#, which are used below, but the wrappers below are almost one-to-one with the GLFW functions invoked.

let mutable width, height = 500, 500
let mutable framebufferWidth, framebufferHeight = width, height

let init = glfwInit()

glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 3)
glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 3)
glfwWindowHint(GLFW_OPENGL_FORWARD_COMPAT, GLFW_TRUE)
glfwWindowHint(GLFW_OPENGL_PROFILE, 0x00032001)

let window = glfwCreateWindow(width, height, "Test Window", NULL_MONITOR, NULL_WINDOW)

glfwMakeContextCurrent(window)

let grGlInterface = GRGlInterface.Create(getProcAddress)

if not(grGlInterface.Validate())
then raise (System.Exception("Invalid GRGlInterface"))

let grContext = GRContext.CreateGl(grGlInterface)

let draw(window, width, height) =
    pollEvents()
    grContext.ResetContext()
    let grGlFramebufferInfo = new GRGlFramebufferInfo(0u, uint32(0x8058))
    use grBackendRenderTarget = new GRBackendRenderTarget(width, height, 1, 0, grGlFramebufferInfo)
    let surface = SKSurface.Create(grContext, grBackendRenderTarget, GRSurfaceOrigin.BottomLeft, SKColorType.Rgba8888)
    let canvas = surface.Canvas
    canvas.Clear(SKColors.Cyan)
    use red = new SKPaint(Color = SKColor(byte(255), byte(0), byte(0), byte(255)))
    canvas.DrawCircle(float32(width)/2.0f, float32(height)/2.0f, float32(100), red)
    canvas.Flush()
    glfwSwapBuffers(window)

while not (glfwWindowShouldClose window = 1) do
    glfwGetFramebufferSize(window, &framebufferWidth, &framebufferHeight)
    draw(window, framebufferWidth, framebufferHeight)
    
glfwDestroyWindow(window)
terminate()

Expected Behavior

No exception.

Actual Behavior

A System.AccessViolationException exception is thrown, often with no stack trace.

Basic Information

• Version with issue: 2.88.0
• Last known good version: unknown
• IDE: Visual Studio Community 2022, version 17.2.3
• Platform Target Frameworks:
  • Linux: I will eventually be targeting Linux, but I have not ran this code at all on Linux yet.
  • macOS: I will also eventually be targeting macOS, but I have not ran a recent version of my bindings and code on macOS.
  • Windows Classic: Windows 11, OS Build 22000.739. This is where the exception is currently being thrown.
• Target Devices:
  • PC

Screenshots

This is a screenshot of the window and image that's being drawn:

Reproduction Link

Unavailable at the moment since the code is in a private repository. A reproduction is linked here.

[bmitc]

Another error reported to the console is:

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
Repeat 2 times:
--------------------------------
   at SkiaSharp.SkiaApi.sk_canvas_flush(IntPtr)
--------------------------------
   at SkiaSharp.SKCanvas.Flush()
   at Windowing.Window+Window.Draw()
   at <StartupCode$WindowTest>.$Program.main@()

This is using some higher-level wrappers, but it is basically doing the same thing as the above code snippet. It shows that in this case, the error occurs at SKCanvas.Flush().

[bmitc]

I have tried debugging this a million ways, and I have not been able to solve this or workaround the issue. There are several seemingly related issues in this repository that mention the System.AccessViolationException, with some of them mentioning that the issue may be due to garbage collection of some of the SkiaSharp objects, such as the canvas. However, I've tried everything from GC.KeepAlive to keeping all SkiaSharp objects in class fields to even keeping a buffer of previous objects in class fields to try and workaround this. None of that has worked.

If I do not recreate the GRBackendRenderTarget and SKSurface every loop iteration, then this does not seem to occur. However, that prevents being able to resize the window, either manually or via a resize callback with GLFW.

Attached is a very simple reproduction of this unexpected exception using only Silk.NET.GLFW, which is just using Silk.NET's GLFW binding and not their windowing abstraction, and SkiaSharp. The project was done in Visual Studio Community 2022 with the latest versions and only requires the NuGet dependencies.

The crash can take a few minutes, but it happens every time. It seems to happen much faster in other slightly more complicated scenarios, but the error is the same System.AccessViolationException at SkiaSharp.SkiaApi.sk_refcnt_safe_unref(IntPtr) as in the original description above.

GLFWTest.zip

[molesmoke]

I'm no F# expert, but should some of those "let" statements be "use"?

[bmitc]

use is used in the attached reproduction using Silk.NET's GLFW bindings.

I was playing around with let to let things try and stay in memory as long as possible since it seemed like that was a possible issue here.

As far as my understanding goes, using let over use would only result in a memory leak for objects that implement IDisposable. That's my understanding though, not sure if that's right.

[molesmoke]

Pretty sure this is just a bug in the example, though arguably it should be possible to notice some error and throw an exception earlier. The dispose pattern deals with resources - that may or may not be memory. It works for me if the unmanaged resources are properly disposed (i.e. GRBackendRenderTarget and SKSurface).

[bmitc]

Thanks for mentioning SKSurface. That seems to be the culprit. Although, like you say, I think maybe there might be some better checks internally, as there is often not even a stack trace when the exception is thrown.

At first I was confused, but the issue is because SKSurface does not have any object constructors and relies on the static method SKSurface.Create to create an SKSurface. In F#, you are notified via a warning when using a constructor for a class that implements IDisposable if you do not use new to create it, as you see for GRBackendRenderTarget and SKPaint in my GLFWTest project. That is the typical way, at least in F#, to know the object is an IDisposable and that you should either call Dispose on the created object or bind it with use, as I did for GRBackendRenderTarget and SKPaint. For example:

Using use for SKSurface or explicitly calling Dispose on it after the canvas is flushed and buffers are swapped seems to be working so far. I would like to test this a bit longer to make sure there isn't a crash.

I'll leave this issue open for now, as I think the "bug" is now at least a documentation issue. Since SKSurface uses a static method to create an SKSurface, I think the documentation should mention that it implements IDisposable. I may try my hand at a documentation PR.

[molesmoke]

Again, I'm no F# expert, but I read that warning as the "new" operator should be used when directly invoking a constructor, not that the named constructor idiom should be avoided. It's fairly common to use that pattern any time additional context is needed to distinguish constructors that otherwise would have the same signatures. Probably here to distinguish between creating a new backend render target vs. wrapping an existing one.

Edit: looks like most of the other named constructors are obsolete, so maybe regular constructors could be added. Trouble is, the existing ones would still be needed for compatibility.

[bmitc]

Yes, I know. I didn't say named constructors should be avoided. In F#, the new keyword is optional and is idiomatically not used when using object constructors. The warning I mentioned is to recommend (or force to get rid of the warning) using new when using an object constructor for a class that implements IDisposable. The warning is there to basically force using the new keyword and to remind you that you should use using, bind the object with use, or manually call Dispose.

https://fsharpforfunandprofit.com/posts/classes/#constructing-and-using-a-class

My point was that SKSurface does not have named constructors and rather uses a static method to create instances. Thus, I didn't know that SKSurface needed to be disposed like GRBackendRenderTarget (because in that case, I received the warning when constructing). That's why I think it should be mentioned in the documentation.

[molesmoke]

A named constructor is a public static method used to instantiate an object... i.e. SKSurface.Create is a named constructor.

Upstream Skia surfaces themselves use named constructors too. I don't think it's wrong to use them where it's appropriate, and I don't necessarily think that F# rule implies that there's any issue here either. As an aside, you might not even always care about disposing types that implement IDisposable (e.g. Tasks). Though probably most of the time you will want to explicitly dispose, it's up to the caller to be aware and make an appropriate judgement.

Looks like the error handling pattern here is that the named constructor returns null if the allocation failed (a regular constructor would only be able to throw an exception for errors), but it looks to me like the binding at least already does the right thing:
https://github.com/mono/SkiaSharp/blob/main/binding/Binding/SKSurface.cs

[bmitc]

Apologies for equating named constructors with object constructors. I wasn't aware that "named constructors" were a specific thing. In my opinion, it's a poor name to mean something different than constructing an object by name. 🙃 I don't really know C#, only enough to use .NET libraries from F#, and defining constructors in F# is handled a bit differently.

@mattleibow Can you please confirm the correct way to handle SKSurface? Should it be handled via using, binding with use, or manually disposed via Dispose, or is it supposed to just work? I think it might help if the documentation for SKSurface was updated.

[mgood7123]

Apologies for equating named constructors with object constructors. I wasn't aware that "named constructors" were a specific thing. In my opinion, it's a poor name to mean something different than constructing an object by name. 🙃 I don't really know C#, only enough to use .NET libraries from F#, and defining constructors in F# is handled a bit differently.

@mattleibow Can you please confirm the correct way to handle SKSurface? Should it be handled via using, binding with use, or manually disposed via Dispose, or is it supposed to just work? I think it might help if the documentation for SKSurface was updated.

generally a surface should be created, and then disposed when done using it

i dont know F# so this is C#

SKSurface surface;

if (width == 0 || height == 0 || graphicsContext == null)
{
    return null;
}

SKSurface s = SKSurface.Create(graphicsContext, false, new SKImageInfo(width, height));

if (s == null)
{
    return null;
}

// draw with s.Canvas

s.Dispose(); // s.Canvas is disposed with surface

[bmitc]

Conclusion: Surface must be disposed of.

[mgood7123]

Conclusion: Surface must be disposed of.

keep in mind anything that automatically disposes the surface will also work, for example, a using statement, another class that manages the lifetime of its own SkSurface and implements dispose interface, and so on

so long as the surface is disposed at the appropriate time