Random crash in SystemResPool.GetSolidBrush

[marius-klimantavicius]

Platform windows.

Note that it is rare and mostly happened to me when running AOT version of our app.

Steps to reproduce:

1. Do not use visual styles (don't know if that's relevant or not though)
2. Show message box before creating a form

Expected result:
Message box is shown correctly

Actual result:
Rarely a .NET exception dialog is shown saying that An item with the same key has already been added.

I could not debug it locally so I don't know what is causing this (especially because it is within lock - maybe recursive call due to recursive message - I don't know). Fix is to always update the dictionary/hashtable instead of trying to add:

SolidBrush brush = new SolidBrush(color);
solidbrushes[hash] = brush; // <-- instead of Add
return brush;

[madewokherd]

That is odd. I see nothing inside the lock that should process messages or allow for any kind of recursive call.

FWIW, I think caching brushes is a bad idea anyway. A SolidBrush is a small managed object and a small unmanaged allocation. There's no reason to expect their creation or destruction to be slow. I see no reason not to just create them as local objects with a simple using statement.

[marius-klimantavicius]

Yeah the fact that it was within the lock was that surprised me the most. Though I do have a hypothesis (though pure speculation that I am not planning to test):

        WndProc
          ...
SystemResPool.GetSolidBrush
           |
           V
    new SolidBrush(color)
           |
           V
       SendMessage (or even maybe a message from user interaction - though what kind as we haven't rendered anything yet)
          ...
        WndProc
          ...
SystemResPool.GetSolidBrush
           |
           V
    new SolidBrush(color) // lock useless as we are in the same thread that already holds the lock
    solidbrushes.Add(x)
          ...

After we return back we've already added it to solidbrushes.

As for using on the brush - I would not do that as it does go to gdi+ to allocate a native brush (at least on windows, don't know about other platforms).

[madewokherd]

SolidBrush constructor should just be a thin wrapper around GdipCreateSolidFill. Wine's version doesn't call anything that would process messages: https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/gdiplus/brush.c?ref_type=heads#L754

And there's no reason to believe Windows does either.

If it were a recursive call like that, I think you'd see it in the stack trace.

[marius-klimantavicius]

I see, probably my hypothesis is not correct then.

As for stack trace - I don't think I would see it as the inner request to cache brush would succeed (as the hashtable is still empty) and only once we got back to the original stack we'd crash.

[madewokherd]

Ah, well, if you really want to debug it (and I maintain there's no reason to and we should just not bother storing solid brushes long-term), I guess you could try replacing it with a ReaderWriteLockSlim with a NoRecursion policy.

[marius-klimantavicius]

I already fixed it locally to use "add or update": solidbrushes[hash] = brush :).

Out of curiosity I did try to capture the stack trace and this is what I got (it is indeed a recursive call):

   at System.Windows.Forms.SystemResPool.GetSolidBrush(Color) + 0x199 <-- Here
   at System.Windows.Forms.Control.get_BackColorBrush() + 0x63
   at System.Windows.Forms.Control.PaintControlBackground(PaintEventArgs) + 0x4bf
   at System.Windows.Forms.Control.WmPaint(Message&) + 0x162
   at System.Windows.Forms.Control.WndProc(Message&) + 0x1f2
   at System.Windows.Forms.NativeWindow.WndProc(IntPtr, Msg, IntPtr, IntPtr) + 0x132
   at System.Windows.Forms.XplatUIWin32.InternalWndProc(HWND, UInt32, WPARAM, LPARAM) + 0x65
   at System.Threading.WaitHandle.WaitForMultipleObjectsIgnoringSyncContext(IntPtr*, Int32, Boolean, Int32) + 0xbe
   at System.Threading.WaitHandle.WaitOneNoCheck(Int32) + 0xc3
   at Microsoft.Win32.SystemEvents.EnsureSystemEvents(Boolean) + 0x148
   at Microsoft.Win32.SystemEvents.AddEventHandler(Object, Delegate) + 0x99
   at System.Drawing.Internal.SystemColorTracker.Add(ISystemColorTracker) + 0xaf
   at System.Drawing.SolidBrush..ctor(Color) + 0x93
   at System.Windows.Forms.SystemResPool.GetSolidBrush(Color) + 0x2e4 <-- Here
   at System.Windows.Forms.Control.get_BackColorBrush() + 0x63
   at System.Windows.Forms.Control.PaintControlBackground(PaintEventArgs) + 0x4bf
   at System.Windows.Forms.Control.WmPaint(Message&) + 0x151
   at System.Windows.Forms.Control.WndProc(Message&) + 0x1f2
   at System.Windows.Forms.NativeWindow.WndProc(IntPtr, Msg, IntPtr, IntPtr) + 0x132
   at System.Windows.Forms.XplatUIWin32.InternalWndProc(HWND, UInt32, WPARAM, LPARAM) + 0x65
   at Windows.Win32.PInvokeCore.DispatchMessage(MSG*) + 0x3a
   at System.Windows.Forms.XplatUIWin32.DispatchMessage(MSG&) + 0x11
   at System.Windows.Forms.Application.RunLoop(Boolean, ApplicationContext) + 0x5d3
   at System.Windows.Forms.Form.ShowDialog(IWin32Window) + 0x3a4
   at System.Windows.Forms.MessageBox.MessageBoxForm.RunDialog() + 0x81

[madewokherd]

OK so dotnet's System.Drawing is doing its own caching of brushes, and it messes this up by doing its own Windows event stuff.

Sounds like more reason to remove the caching from System.Windows.Forms to me.

[madewokherd]

Made a Winehq MR for this: https://gitlab.winehq.org/mono/mono/-/merge_requests/155