Over-release now that VARIANT is using IDisposable.

[AaronRobinsonMSFT]

There is now an over-release in the VARIANT tests. This is caused by VARIANT now implementing IDisposable.

Found while investigating dotnet/runtime#70973.

It was also observed that the use of the using var ... = new VARIANT(...) has left a lot of cruft. Most places also continue to call Close() or Dispose() even though they have the using var semantic. I highly recommend someone go through this and clean up this as it is very confusing.

/cc @JeremyKuhne @RussKie

Microsoft Reviewers: Open in CodeFlow

[RussKie]

Thank you!

[AaronRobinsonMSFT]

There are some other issues here. One of them is the following where the assert is causing the test host to crash. The OS is Windows 11.

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Application.ThreadContext.cs

Lines 308 to 315 in 0aaae6b

	// Legacy OS/target framework scenario where ControlDpiContext is set to DPI_AWARENESS_CONTEXT.DPI_AWARENESS_CONTEXT_UNSPECIFIED
	// because of 'ThreadContextDpiAwareness' API unavailability or this feature is not enabled.
	if (User32.AreDpiAwarenessContextsEqual(context, User32.UNSPECIFIED_DPI_AWARENESS_CONTEXT))
	{
	Debug.Assert(_parkingWindows.Count == 1, "parkingWindows count can not be > 1 for legacy OS/target framework versions");
	return _parkingWindows[0];
	}

[AaronRobinsonMSFT]

There is also still the IMallocSpy issue during COM apartment clean-up.

[dreddy-work]

There are some other issues here. One of them is the following where the assert is causing the test host to crash. The OS is Windows 11.

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Application.ThreadContext.cs

Lines 308 to 315 in 0aaae6b

	// Legacy OS/target framework scenario where ControlDpiContext is set to DPI_AWARENESS_CONTEXT.DPI_AWARENESS_CONTEXT_UNSPECIFIED
	// because of 'ThreadContextDpiAwareness' API unavailability or this feature is not enabled.
	if (User32.AreDpiAwarenessContextsEqual(context, User32.UNSPECIFIED_DPI_AWARENESS_CONTEXT))
	{
	Debug.Assert(_parkingWindows.Count == 1, "parkingWindows count can not be > 1 for legacy OS/target framework versions");
	return _parkingWindows[0];
	}

@AaronRobinsonMSFT , which test was this? Did you run the test locally as well?

[AaronRobinsonMSFT]

@dreddy-work That is from running locally. I just ran build.cmd -test and that was one of the crashes that I collected. I can share the DMP file offline if you like?

[AaronRobinsonMSFT]

@dreddy-work The stack is the following:

parkingWindows count can not be > 1 for legacy OS/target framework versions
   at System.NoAssertContext.NoAssertListener.Fail(String message, String detailMessage) in F:\winforms\src\System.Windows.Forms.Primitives\tests\TestUtilities\NoAssertContext.cs:line 104
   at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage) in D:\runtime\src\libraries\System.Diagnostics.TraceSource\src\System\Diagnostics\TraceInternal.cs:line 267
   at System.Diagnostics.TraceInternal.TraceProvider.Fail(String message, String detailMessage) in D:\runtime\src\libraries\System.Diagnostics.TraceSource\src\System\Diagnostics\TraceInternal.cs:line 17
   at System.Diagnostics.Debug.Fail(String message, String detailMessage)
   at System.Windows.Forms.Application.ThreadContext.GetParkingWindowForContext(IntPtr context) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Application.ThreadContext.cs:line 312
   at System.Windows.Forms.Application.ThreadContext.GetParkingWindow(IntPtr context) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Application.ThreadContext.cs:line 274
   at System.Windows.Forms.Application.UnparkHandle(HandleRef handle, IntPtr context) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Application.cs:line 1105
   at System.Windows.Forms.Control.SetParentHandle(IntPtr value) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 10974
   at System.Windows.Forms.Control.ControlCollection.Add(Control value) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Control.ControlCollection.cs:line 46
   at System.Windows.Forms.Form.ControlCollection.Add(Control value) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Form.ControlCollection.cs:line 55
   at System.Windows.Forms.Tests.ErrorProviderAccessibleObjectTests..ctor() in F:\winforms\src\System.Windows.Forms\tests\UnitTests\System\Windows\Forms\ErrorProviderAccessibleObjectTests.cs:line 36
   at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean wrapExceptions)
   at System.RuntimeType.CreateInstanceImpl(BindingFlags bindingAttr, Binder binder, Object[] args, CultureInfo culture)
   at System.Activator.CreateInstance(Type type, Object[] args)
   at ReflectionAbstractionExtensions.<>c__DisplayClass0_0.<CreateTestClass>b__0() in /_/src/xunit.execution/Extensions/ReflectionAbstractionExtensions.cs:line 42
   at Xunit.Sdk.ExecutionTimer.Aggregate(Action action) in /_/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 /_/src/xunit.execution/Extensions/ReflectionAbstractionExtensions.cs:line 42
   at Xunit.Sdk.TestInvoker`1.CreateTestClass() in /_/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 InvokeStub_SendOrPostCallback.Invoke(Object, Object, IntPtr*)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Delegate.DynamicInvokeImpl(Object[] args)
   at System.Windows.Forms.Control.InvokeMarshaledCallbackDo(ThreadMethodEntry tme) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 6593
   at System.Windows.Forms.Control.InvokeMarshaledCallbackHelper(Object obj) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 6546
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Windows.Forms.Control.InvokeMarshaledCallback(ThreadMethodEntry tme) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 6523
   at System.Windows.Forms.Control.InvokeMarshaledCallbacks() in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 6631
   at System.Windows.Forms.Control.WndProc(Message& m) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Control.cs:line 13352
   at System.Windows.Forms.Control.ControlNativeWindow.OnMessage(Message& m) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Control.ControlNativeWindow.cs:line 65
   at System.Windows.Forms.Control.ControlNativeWindow.WndProc(Message& m) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Control.ControlNativeWindow.cs:line 114
   at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, WM msg, IntPtr wparam, IntPtr lparam) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\NativeWindow.cs:line 369
   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:\winforms\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:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Application.ThreadContext.cs:line 1117
   at System.Windows.Forms.Application.ThreadContext.RunMessageLoop(msoloop reason, ApplicationContext context) in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Application.ThreadContext.cs:line 981
   at System.Windows.Forms.Application.DoEvents() in F:\winforms\src\System.Windows.Forms\src\System\Windows\Forms\Application.cs:line 809
   at Xunit.Sdk.WinFormsSynchronizationContextAdapter.PumpTill(SynchronizationContext synchronizationContext, Task task)
   at Xunit.Sdk.ThreadRental.<>c__DisplayClass11_0.<CreateAsync>b__0()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)

[dreddy-work]

@dreddy-work The stack is the following:

I am running this test locally and in debug mode and it does not fail. Only reason why we enter into this situation is, for some reason, on the machine where we are running these tests or the way we are running tests, we couldn't set thread's DpiawarenessContext. I am guessing this is noise and shouldn't be consistent. Please let me know if you keep seeing this.

[AaronRobinsonMSFT]

I am guessing this is noise and shouldn't be consistent. Please let me know if you keep seeing this.

I'll let you know. I am running against a Checked build of coreclr so it may behave slightly different.

[AaronRobinsonMSFT]

I'd appreciate another look here. I've removed the IMallocSpy implementation from the codebase. It was only used to validate two simple internal data types that are trivial to audit visually. The issue here is the impact of IMallocSpy is very difficult to address because of how it creeps in. However, I am planning on adding a test in dotnet/runtime to ensure we don't regress this, but for winforms I don't see the value and it is leading to troublesome clean-up during application shutdown.

[JeremyKuhne]

@AaronRobinsonMSFT can you add some more details on how IMallocSpy doesn't play nice?

[AaronRobinsonMSFT]

@AaronRobinsonMSFT can you add some more details on how IMallocSpy doesn't play nice?

Sure. IMallocSpy isn't really the issue itself. The real issue is that it is implemented in managed code. This turned out to have benefits and substantial issues. A benefit was that it helped track down places in the CLR where we weren't transitioning to preemptive mode (i.e., non-GC controlled) and so I fixed those bugs.

The issues overshadow these wins though. The biggest annoyance is that it is close to impossible to properly detach the IMallocSpy from the system when running tests correctly. Once an IMallocSpy is registered it can't be revoked until all registered outstanding allocations are cleaned up. This means that even if a test registers and revokes the IMallocSpy, it is still global and therefore any other concurrently running tests will be able to retain it while they are being cleaned up. This then becomes a race between attempting to track and untrack allocations across all tests and their associated scenarios. Given the number of tests this becomes difficult.

The part that makes this substantially more difficult is when any cross-apartment marshalling occurs, because that also allocates memory that IMallocSpy monitors. The RPC data structures can live far longer than the average test. During process shutdown, long after the CLR has terminated and possibly unloaded, the COM subsystem does a clean-up and, of course, tries to call the registered IMallocSpy. This can fail in unpredictable ways because the CLR is shutdown but worse the implemented .NET assembly may have already been unloaded.

The gist here is IMallocSpy should not be implemented in managed code in practice. It is helpful for the runtime to find issues but it comes with far too many issues to be used in practice outside of a runtime test.

[RussKie]

Thank you @AaronRobinsonMSFT.

@JeremyKuhne I'm going to merge this to unblock the backlog. Maybe we could get together with @AaronRobinsonMSFT and chat about options to tests

winforms/src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.CALPOLESTR.cs

Lines 39 to 47 in 0aaae6b

	public unsafe void Free()
	{
	for (int i = 0; i < cElems; i++)
	{
	Marshal.FreeCoTaskMem((IntPtr)pElems[i]);
	}
	Marshal.FreeCoTaskMem((IntPtr)pElems);
	}

winforms/src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.CADWORD.cs

Line 30 in 0aaae6b

public void Free() => Marshal.FreeCoTaskMem((IntPtr)pElems);