Leaking DeviceContext instances in unit tests

[weltkante]

.NET Core Version:
github master

Have you experienced this same bug with .NET Framework?:
unknown (cannot debug Desktop Framework source to determine if ErrorProvider leaks there as well)

Problem description:
While investigating control handle leaks in the unit tests I also noticed significant DeviceContext leaks. They are not permanent leaks, the GC does clean them up, but they are nondeterministic and released on a different thread than they were created on - don't know if thats any issue, probably not since tests seem to be running fine generally. I also don't know if there is any relation to previously observed GDI resource bottlenecks on the CI machine.

• WindowsGraphicsCacheManager caches a DeviceContext for each UI thread but never releases it, so it will leak and be GC'ed. Since unit testing creates a lot of test threads this also cycles a lot of DeviceContext instances through the GC.

Stack Trace Example

Test:

System.Windows.Forms.Tests.AccessibleObjects.MonthCalendarAccessibleObjectTests.MonthCalendarAccessibleObject_GetCalendarCell_DoesntThrowException_If_ParentAccessibleObject_IsNull

Stacktrace:

   at System.Windows.Forms.Internal.DeviceContext..ctor(IntPtr hDC, DeviceContextType dcType)
   at System.Windows.Forms.Internal.DeviceContext.FromCompatibleDC(IntPtr hdc)
   at System.Windows.Forms.Internal.WindowsGraphics.CreateMeasurementWindowsGraphics()
   at System.Windows.Forms.Internal.WindowsGraphicsCacheManager.get_MeasurementGraphics()
   at System.Windows.Forms.Internal.WindowsFont.FromFont(Font font, QUALITY fontQuality)
   at System.Windows.Forms.MonthCalendar.GetMinReqRect(Int32 newDimensionLength, Boolean updateRows, Boolean updateCols)
   at System.Windows.Forms.MonthCalendar.GetMinReqRect()
   at System.Windows.Forms.MonthCalendar.get_DefaultSize()
   at System.Windows.Forms.Control..ctor(Boolean autoInstallSyncContext)
   at System.Windows.Forms.Control..ctor()
   at System.Windows.Forms.MonthCalendar..ctor()
   at System.Windows.Forms.Tests.AccessibleObjects.MonthCalendarAccessibleObjectTests.MonthCalendarAccessibleObject_GetCalendarCell_DoesntThrowException_If_ParentAccessibleObject_IsNull()

• The ErrorProvider creates a private nested class ErrorWindow which leaks multiple DeviceContext instances from CreateMirrorDC which are never disposed correctly even though the ErrorProvider and its controls seem to be disposed properly.

  This happens in the ErrorProviderAccessibleObjectTests which now does have Dispose method and is verified to be called, but also on other ErrorProvider tests (see example C) which don't use instance-level fields. I may have overlooked something but it is not clear to me what additional Dispose calls are needed to clean up an ErrorProvider correctly, so its currently unknown if its tests not disposing properly or the implementation not disposing properly.

Stack Trace Example A

Test:

System.Windows.Forms.Tests.AccessibleObjects.ErrorProviderAccessibleObjectTests.ErrorProvider_NameDoesntEqualControlTypeOrChildName

Stacktrace:

   at System.Windows.Forms.Internal.DeviceContext..ctor(IntPtr hDC, DeviceContextType dcType)
   at System.Windows.Forms.Internal.DeviceContext.FromHdc(IntPtr hdc)
   at System.Windows.Forms.ErrorProvider.ErrorWindow.CreateMirrorDC(IntPtr hdc, Int32 originOffset)
   at System.Windows.Forms.ErrorProvider.ErrorWindow.Update(Boolean timerCaused)
   at System.Windows.Forms.ErrorProvider.ErrorWindow.Add(ControlItem item)
   at System.Windows.Forms.ErrorProvider.ControlItem.AddToWindow()
   at System.Windows.Forms.ErrorProvider.ControlItem.set_Error(String value)
   at System.Windows.Forms.ErrorProvider.SetError(Control control, String value)
   at System.Windows.Forms.Tests.AccessibleObjects.ErrorProviderAccessibleObjectTests..ctor()

Stack Trace Example B

Test:

System.Windows.Forms.Tests.AccessibleObjects.ErrorProviderAccessibleObjectTests.ErrorProvider_CorrectErrorValue

Stacktrace:

   at System.Windows.Forms.Internal.DeviceContext..ctor(IntPtr hDC, DeviceContextType dcType)
   at System.Windows.Forms.Internal.DeviceContext.FromHdc(IntPtr hdc)
   at System.Windows.Forms.ErrorProvider.ErrorWindow.CreateMirrorDC(IntPtr hdc, Int32 originOffset)
   at System.Windows.Forms.ErrorProvider.ErrorWindow.Update(Boolean timerCaused)
   at System.Windows.Forms.ErrorProvider.ErrorWindow.StartBlinking()
   at System.Windows.Forms.ErrorProvider.ControlItem.StartBlinking()
   at System.Windows.Forms.ErrorProvider.ControlItem.AddToWindow()
   at System.Windows.Forms.ErrorProvider.ControlItem.set_Error(String value)
   at System.Windows.Forms.ErrorProvider.SetError(Control control, String value)
   at System.Windows.Forms.Tests.AccessibleObjects.ErrorProviderAccessibleObjectTests..ctor()

Stack Trace Example C

Test:

System.Windows.Forms.Tests.ErrorProviderTests.ErrorProvider_Items_CallEvents_Success

Stacktrace:

   at System.Windows.Forms.Internal.DeviceContext..ctor(IntPtr hDC, DeviceContextType dcType)
   at System.Windows.Forms.Internal.DeviceContext.FromHdc(IntPtr hdc)
   at System.Windows.Forms.ErrorProvider.ErrorWindow.CreateMirrorDC(IntPtr hdc, Int32 originOffset)
   at System.Windows.Forms.ErrorProvider.ErrorWindow.Update(Boolean timerCaused)
   at System.Windows.Forms.ErrorProvider.ErrorWindow.StartBlinking()
   at System.Windows.Forms.ErrorProvider.ControlItem.StartBlinking()
   at System.Windows.Forms.ErrorProvider.ControlItem.AddToWindow()
   at System.Windows.Forms.ErrorProvider.ControlItem.OnParentVisibleChanged(Object sender, EventArgs e)
   at System.Windows.Forms.Control.OnVisibleChanged(EventArgs e)
   at System.Windows.Forms.Control.SetVisibleCore(Boolean value)
   at System.Windows.Forms.Control.set_Visible(Boolean value)
   at System.Windows.Forms.Tests.ErrorProviderTests.ErrorProvider_Items_CallEvents_Success(ErrorBlinkStyle blinkStyle, SubControl control, String error, String expected)

Expected behavior:
Ideally deterministic cleanup of GDI resources (instead of letting the GC clean up lazily)

Minimal repro:
Run unit tests under a debugger and put a conditional breakpoint in DeviceContext finalizer. Condition it on _disposed == false.

(Sometimes the finalizer is called on disposed objects, thats an issue with finalization order and cross-class internal calls from other finalized classes to Dispose(bool) not implementing the disposable pattern correctly. Doesn't cause any harm just a redundant call.)

[weltkante]

/cc @JeremyKuhne these are probably interesting to look at whenever you get around to turning DeviceContext into a struct. They either need to be fixed or need a replacement finalizer, otherwise the leaks currently cleaned up by GC would become permanent leaks when switching to structs.

[JeremyKuhne]

@weltkante The hope is that we can keep almost everything on the stack. In any cases where we're forced to maintain a longer lifetime I'll look for a separate and more targeted tracking mechanism.

[hughbe]

(Sometimes the finalizer is called on disposed objects, thats an issue with finalization order and cross-class internal calls from other finalized classes to Dispose(bool) not implementing the disposable pattern correctly. Doesn't cause any harm just a redundant call.)

We should fix this right? (If we can)

[weltkante]

(slightly offtopic, this comment is not about the leaks in the issue description, but on my side comment about a wrong dispose pattern; mentioning it explicitly to avoid confusion)

If we are going to replace DeviceContext by a struct anyways its not worth the effort to correct this, in particular since changing the code to conform to the dispose pattern may introduce new edge cases.

Right now it happens to work out correctly even if things go wrong, several mistakes end up aligning just right so that the indeterministic finalizer order doesn't make a difference in this scenario, the only net effect is an unnecessary finalizer call. DeviceContext is considered leaked but since WindowsGraphics passes the wrong flag to Dispose(bool) it doesn't make a difference, the implementation of DeviceContext.Dispose(bool) cannot tell the difference and will release "as if leaked" even when called by WindowsGraphics. Maybe that always was the intention but its a major violation of the dispose pattern and not commented.

Fixing the dispose pattern to pass the correct flag and be independent of finalizer order would require very careful review not to introduce new leaks.

[JeremyKuhne]

#3553 should take care of this