SkiaSharp.SKObject.Dispose can throw ArgumentNullException on finalization

[davkean]

Had a customer complaining about crashes in Visual Studio while using SkiaSharp in the WinForms designer: https://developercommunity.visualstudio.com/content/problem/228761/crashing-when-debugging-a-skiasharp-winforms-app-i.html.

Looking into his crashes it looks like SkiaSharp.SKObject.Dispose can throw on finalization:

   at System.Threading.Monitor.ReliableEnter(System.Object, Boolean ByRef)
   at SkiaSharp.SKObject.Dispose(Boolean)
   at SkiaSharp.SKPaint.Dispose(Boolean)
   at SkiaSharp.SKNativeObject.Finalize()

If you look at Dispose, you can see multiple paths where it attempts to access managed instance and static fields that have no guarantee about already being cleaned up by the time the finalizer has run:

SkiaSharp/binding/Binding/SKObject.cs

Line 63 in 9a0bff8

{

Any access to member/static reference fields should be guarded with if (disposing). Or Better yet, wrap the unmanaged handle in a SafeHandle and let it free it.

[davkean]

Actually I'm wrong here - while you shouldn't really touch reference fields in finalize, none of them should be null. What I suspect is happening, is that SKPaint is throwing in its constructor here:

SkiaSharp/binding/Binding/SKPaint.cs

Line 21 in 9a0bff8

public SKPaint ()

.

This leaves the base field ownedObjects null, but the finalizer still is run. The following demonstrates:

    class Program
    {
        static void Main(string[] args)
        {
            try
            {
                new SKPaint();
            }
            catch (Exception)
            {

            }
        }
    }


    class SKPaint : SKObject
    {
        public SKPaint()
            : base(sk_paint_new())
        {
        }

        private static IntPtr sk_paint_new() { throw new Exception(); }
    }

    class SKObject
    {
        private readonly List<string> ownedObjects = new List<string>();

        internal SKObject(IntPtr handle)
        {
        }

        ~SKObject()
        {
            lock (ownedObjects)
            {
            }
        }
    }

Which outputs:

Unhandled Exception: System.ArgumentNullException: Value cannot be null.
   at System.Threading.Monitor.ReliableEnter(Object obj, Boolean& lockTaken)
   at System.Threading.Monitor.Enter(Object obj, Boolean& lockTaken)
   at ConsoleApp237.SKObject.Finalize()

[mattleibow]

Do we have a larger stack trace? I see the two you provided are different.
Also, what is the constructor throwing? Are we able to see this at all?

We can guard against this by doing a null check, but it would be great to know more.

I'll write a test case and see if I can fix things.

[aktzbn]

I encountered the same issue on Debian when libSkiaSharp.so was absent.

[mattleibow]

Yeah, that is what I was thinking. The only time the methods would throw would be when the native library was not found. the C+ just returns null on error.

Thanks for the confirmation. And this issue relates to the designer and VS and debugging - that is a volatile combo that sometimes doesn't work.

[mattleibow]

@davkean Thanks for your info and research. I fixed this issue and it is now in master.