ErrorProviderAccessibleObjectTests.Ctor tests flakey

[hughbe]

• .NET Core Version:
  Master

• Have you experienced this same bug with .NET Framework?:
  N/A

Problem description:
In #3366

Process terminated. Assertion Failed
Accessing a disposed DC, forcing recreation of HDC - this will generate a Handle leak!
   at System.NoAssertContext.NoAssertListener.Fail(String message, String detailMessage) in F:\workspace\_work\1\s\src\System.Windows.Forms.Primitives\tests\TestUtilities\NoAssertContext.cs:line 105
   at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage)
   at System.Diagnostics.TraceInternal.TraceProvider.Fail(String message, String detailMessage)
   at System.Diagnostics.Debug.Assert(Boolean condition, String message)
   at System.Windows.Forms.Internal.DeviceContext.get_Hdc() in F:\workspace\_work\1\s\src\System.Windows.Forms\src\misc\GDI\DeviceContext.cs:line 102
   at System.Windows.Forms.Internal.DeviceContext.Equals(Object obj) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\misc\GDI\DeviceContext.cs:line 450
   at System.Windows.Forms.WeakRefCollection.WeakRefObject.Equals(Object obj) in F:\workspace\_work\1\s\src\System.Windows.Forms.Primitives\src\System\Windows\Forms\Internals\WeakRefCollection.cs:line 253
   at System.Collections.ArrayList.Contains(Object item)
   at System.Windows.Forms.WeakRefCollection.Contains(Object value) in F:\workspace\_work\1\s\src\System.Windows.Forms.Primitives\src\System\Windows\Forms\Internals\WeakRefCollection.cs:line 181
   at System.Windows.Forms.Internal.DeviceContexts.AddDeviceContext(DeviceContext dc) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\misc\GDI\DeviceContexts.cs:line 28
   at System.Windows.Forms.Internal.DeviceContext..ctor(IntPtr hWnd) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\misc\GDI\DeviceContext.cs:line 174
   at System.Windows.Forms.Internal.DeviceContext.FromHwnd(IntPtr hwnd) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\misc\GDI\DeviceContext.cs:line 218
   at System.Windows.Forms.ErrorProvider.ErrorWindow.Update(Boolean timerCaused) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\ErrorProvider.cs:line 1244
   at System.Windows.Forms.ErrorProvider.ErrorWindow.StartBlinking() in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\ErrorProvider.cs:line 1151
   at System.Windows.Forms.ErrorProvider.ControlItem.StartBlinking() in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\ErrorProvider.cs:line 1622
   at System.Windows.Forms.ErrorProvider.ControlItem.AddToWindow() in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\ErrorProvider.cs:line 1642
   at System.Windows.Forms.ErrorProvider.ControlItem.set_Error(String value) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\ErrorProvider.cs:line 1491
   at System.Windows.Forms.ErrorProvider.SetError(Control control, String value) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\ErrorProvider.cs:line 791
   at System.Windows.Forms.Tests.AccessibleObjects.ErrorProviderAccessibleObjectTests..ctor() in F:\workspace\_work\1\s\src\System.Windows.Forms\tests\UnitTests\AccessibleObjects\ErrorProviderAccessibleObjectTests.cs:line 44
   at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
   at System.Activator.CreateInstance(Type type, Boolean nonPublic, Boolean wrapExceptions)
   at System.RuntimeType.CreateInstanceImpl(BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture)
   at System.Activator.CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture, Object[] activationAttributes)
   at System.Activator.CreateInstance(Type type, Object[] args)
   at ReflectionAbstractionExtensions.<>c__DisplayClass0_0.<CreateTestClass>b__0() in C:\Dev\xunit\xunit\src\xunit.execution\Extensions\ReflectionAbstractionExtensions.cs:line 42
   at Xunit.Sdk.ExecutionTimer.Aggregate(Action action) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\ExecutionTimer.cs:line 31
   at ReflectionAbstractionExtensions.CreateTestClass(ITest test, Type testClassType, Object[] constructorArguments, IMessageBus messageBus, ExecutionTimer timer, CancellationTokenSource cancellationTokenSource) in C:\Dev\xunit\xunit\src\xunit.execution\Extensions\ReflectionAbstractionExtensions.cs:line 42
   at Xunit.Sdk.TestInvoker`1.CreateTestClass() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs:line 124
   at Xunit.Sdk.UITestInvoker.<RunAsync>b__2_0()
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.ExecutionContextCallback(Object s)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.MoveNext(Thread threadPoolThread)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.MoveNext()
   at Xunit.Sdk.Utilities.SyncContextAwaiter.<>c.<OnCompleted>b__5_0(Object s)
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Delegate.DynamicInvokeImpl(Object[] args)
   at System.Delegate.DynamicInvoke(Object[] args)
   at System.Windows.Forms.Control.InvokeMarshaledCallbackDo(ThreadMethodEntry tme) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 6512
   at System.Windows.Forms.Control.InvokeMarshaledCallbackHelper(Object obj) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 6469
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Windows.Forms.Control.InvokeMarshaledCallback(ThreadMethodEntry tme) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 6446
   at System.Windows.Forms.Control.InvokeMarshaledCallbacks() in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 6550
   at System.Windows.Forms.Control.WndProc(Message& m) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 13185
   at System.Windows.Forms.Control.ControlNativeWindow.OnMessage(Message& m) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Control.ControlNativeWindow.cs:line 67
   at System.Windows.Forms.Control.ControlNativeWindow.WndProc(Message& m) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Control.ControlNativeWindow.cs:line 119
   at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, WM msg, IntPtr wparam, IntPtr lparam) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\NativeWindow.cs:line 372
   at Interop.User32.DispatchMessageW(MSG& msg)
   at Interop.User32.DispatchMessageW(MSG& msg)
   at System.Windows.Forms.Application.ComponentManager.Interop.Mso.IMsoComponentManager.FPushMessageLoop(UIntPtr dwComponentID, msoloop uReason, Void* pvLoopData) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Application.ComponentManager.cs:line 345
   at System.Windows.Forms.Application.ThreadContext.RunMessageLoopInner(msoloop reason, ApplicationContext context) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Application.ThreadContext.cs:line 1130
   at System.Windows.Forms.Application.ThreadContext.RunMessageLoop(msoloop reason, ApplicationContext context) in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Application.ThreadContext.cs:line 992
   at System.Windows.Forms.Application.DoEvents() in F:\workspace\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Application.cs:line 811
   at Xunit.Sdk.WinFormsSynchronizationContextAdapter.PumpTill(SynchronizationContext synchronizationContext, Task task)
   at Xunit.Sdk.ThreadRental.<>c__DisplayClass11_0.<CreateAsync>b__0()
   at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()

[RussKie]

Yes, just saw it too. I haven't seen it before... 😕

[weltkante]

That looks like an actual bug and not just misbehaving tests

at System.Windows.Forms.WeakRefCollection.Contains(Object value)
at System.Windows.Forms.Internal.DeviceContexts.AddDeviceContext(DeviceContext dc)
in F:\workspace_work\1\s\src\System.Windows.Forms\src\misc\GDI\DeviceContexts.cs:line 28

There is a static weak reference list in play here, so this test interacts with all other tests ever having entered something in that list. Normally this is supposed to be designed threadsafe & cleanup correctly in response to GC/Disposal.

Looking at the implementation I cannot see any threadsafety logic like locks, so maybe this is just a race condition triggered in the middle of cleanup when having multiple UI threads and some other thread tries to clean up its DC at the same time someone adds his DC?

Either that or there is a more general cleanup logic bug where someone forgets to clean up his DC entirely.

I'd suggest starting by figuring out what the proper scope for adding thread safety is (unless its already threadsafe and I missed the scope at which locks are taken)

[weltkante]

Randomly happened to me locally as well, not limited to CI, but seems to be quite rare to trigger (at least for me) so I couldn't catch it under a debugger yet. I still think the above explanation of not being threadsafe makes most sense of what could be the cause. DeviceContext will set the disposed-flag before doing the actual cleanup, so not being threadsafe means it can be caught disposed before having cleaned up itself.

While investigating I also found ErrorProviderAccessibleObjectTests constructor creates controls in the test-class without disposing them ever. I think the test-class needs to be IDisposable. That isn't related to the failure though since the Assert happens during construction. Adding it to #3361

[weltkante]

#3441 should have a dump in the 2nd failed test run, maybe could try using it to confirm the multithreading issue, but probably not necessary

[hughbe]

Another case in #1932
https://dev.azure.com/dnceng/public/_build/results?buildId=689209&view=artifacts&type=publishedArtifacts

[RussKie]

Another one: https://dev.azure.com/dnceng/public/_build/results?buildId=732876&view=artifacts&type=publishedArtifacts

@JeremyKuhne do you think it can be solved in conjunction with you work?

[JeremyKuhne]

#3553 should take care of this

[weltkante]

Yes the PR removes the problematic WeakRefCollection instance so there is nothing to be made threadsafe anymore, should fix the race condition.

Looking at other usages of WeakRefCollection it looks like they have the same bug though, disposing items before removing them from the collection, without any locking. So callers from other UI threads would observe disposed elements in the collection. I'll open a separate issue since they were not yet observed as flakey yet. [edit] never mind, other usages of WeakRefCollection are ThreadStatic, so have no multithreading issues.