First round of converting System.Drawing.Common to COMWrappers

[eerhardt]

Using COM Wrappers makes the library trim compatible.

I also added a trimming test to ensure the method in question works with trimming.

@ericstj @ViktorHofer - this is another case where we are splitting ".NET Core 3.1" build with ".NET Current" build. (I had to do that in order to use ComWrappers, because that API came in 5.0.) This means we are going to be missing test coverage for all of these COM interop scenarios for System.Drawing.Common v6.0.0 when running on .NET Core 3.1.

cc @kant2002

[ghost]

Tagging subscribers to this area: @safern, @tarekgh
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Using COM Wrappers makes the library trim compatible.

I also added a trimming test to ensure the method in question works with trimming.

@ericstj @ViktorHofer - this is another case where we are splitting ".NET Core 3.1" build with ".NET Current" build. (I had to do that in order to use ComWrappers, because that API came in 5.0.) This means we are going to be missing test coverage for all of these COM interop scenarios for System.Drawing.Common v6.0.0 when running on .NET Core 3.1.

cc @kant2002

Author:	eerhardt
Assignees:	-
Labels:	area-System.Drawing
Milestone:	-

[ViktorHofer]

@ericstj @ViktorHofer - this is another case where we are splitting ".NET Core 3.1" build with ".NET Current" build. (I had to do that in order to use ComWrappers, because that API came in 5.0.) This means we are going to be missing test coverage for all of these COM interop scenarios for System.Drawing.Common v6.0.0 when running on .NET Core 3.1.

I agree that is painful and should be fixed by adding support for testing on older .NETCoreApp versions and run them either per rolling build or nightly. We don't have capacity to run them on every PR.

[ericstj]

@ViktorHofer what if this was opt-in per test? A test project could decide to add a configuration for netcore3.1-Windows and we could have one of the build legs (either windows or all-configurations) create the helix work items. Would this require more capacity than a single test additional test assembly?

[ericstj]

cc @michaelgsharp as well, who's been doing a lot of helix work lately. ML.NET has a similar requirement around wanting to run tests against multiple different framework versions.

Also, maybe we should move this discussion to an issue. Do we have a good one already?

[ViktorHofer]

@ViktorHofer what if this was opt-in per test? A test project could decide to add a configuration for netcore3.1-Windows and we could have one of the build legs (either windows or all-configurations) create the helix work items. Would this require more capacity than a single test additional test assembly?

The overall issue isn't specific to Windows, we need coverage on as many OSs that a library supports, as possible. The cost that I'm concerned about isn't observable in the amount of build agents but in the number of Helix clients that accept work items.

I imagine support for testing on older .NETCoreApp configurations could be implemented in the following way in order:

• Downgrade the TestUtilities.csproj to netcoreapp3.1 (use $(NetCoreAppMinimum) which will be introduced soon).
• Add infra support for running .NETCoreApp tests without a testhost (same as for .NETFramework testing) here.
• Add two legs (or as an optimization only one leg) for netcoreapp3.1 and net5.0 that build the respective configuration and submit jobs to helix for all OSs. Consider adding a new pipeline for that so that it can be scheduled (i.e. nightly).
• Add netcoreapp3.1/net5.0 TFMs in the OOB test projects either everywhere or on a per project basis where it makes sense (I think the former would make more sense).
• Make sure that the new pipeline is monitored to catch regressions in the older .NETCoreApp TFM assets.

[ViktorHofer]

Moved test infrastructure discussion into #54639.

[eerhardt]

Looks like the x86 tests are failing:

Fatal error. 0xC0000005
   at System.Runtime.InteropServices.Marshal.Release(IntPtr)
   at System.Drawing.Icon.Save(System.IO.Stream)
   at System.Drawing.Tests.IconTests.SaveAndCompare(System.Drawing.Icon, Boolean)
   at System.Drawing.Tests.IconTests.FromHandle_IconHandleMultipleTime_Success()

Trying to get a repro locally.

[safern]

@ViktorHofer do we have a lot of projects that would run on previous versions of .NET Core? What if we start small and run them on Ubuntu, Windows and macOS on the build agent without using helix? If there aren't may projects that would want to do this, we could get away without using helix and running them on the build agent.

[eerhardt]

Looks like the x86 tests are failing:

This is what I see when trying to debug the .dmp file. @AaronRobinsonMSFT any thoughts?

Thread  11
Current frame: coreclr!GetCLRRuntimeHost + 0x43f03
ChildEBP RetAddr  Caller, Callee
072AD388 72d194e2 coreclr!coreclr_shutdown_2 + 0x94a52, calling coreclr!coreclr_shutdown_2 + 0x109cc
072AD470 72d194e2 coreclr!coreclr_shutdown_2 + 0x94a52, calling coreclr!coreclr_shutdown_2 + 0x109cc
072AD484 72dabc83 coreclr!GetCLRRuntimeHost + 0x43f03, calling coreclr!coreclr_shutdown_2 + 0x2f670
072AD698 72c3b2d4 coreclr!coreclr_initialize + 0x42f4, calling coreclr!coreclr_initialize + 0x430b
072AD6A8 72d194ef coreclr!coreclr_shutdown_2 + 0x94a5f, calling coreclr!GetCLRRuntimeHost + 0x43e91
072AD78C 72c8dec3 coreclr!coreclr_shutdown_2 + 0x9433, calling coreclr!coreclr_shutdown_2 + 0x945f
072AD80C 72be1501 coreclr + 0x61501, calling coreclr!coreclr_shutdown_2 + 0x169a3
072AD810 72be1462 coreclr + 0x61462, calling coreclr!coreclr_shutdown_2 + 0x169a3
072AD828 72be04fe coreclr + 0x604fe, calling coreclr + 0x60517
072AD86C 771980a9 ntdll!ExecuteHandler2 + 0x26
072AD890 7719807b ntdll!ExecuteHandler + 0x24, calling ntdll!ExecuteHandler2
072AD8B4 7719801d ntdll!RtlDispatchException + 0x127, calling ntdll!RtlpExecuteHandlerForException
072AD940 77150133 ntdll!KiUserExceptionDispatcher + 0xf, calling ntdll!RtlDispatchException
072ADE0C 90ffffd4 90ffffd4 ====> Exception cxr@072AD9A8
072ADB94 77172d9a ntdll!RtlpFreeHeap + 0x1f4, calling ntdll!RtlpCoalesceFreeBlocks
072ADBE4 0735c49a (MethodDesc 0038f78c + 0xa System.Buffer.Memmove[[System.Byte, System.Private.CoreLib]](Byte ByRef, Byte ByRef, UIntPtr)), calling (MethodDesc 00270f28 + 0 System.Buffer.Memmove(Byte ByRef, Byte ByRef, UIntPtr))
072ADD08 72bd86bc coreclr + 0x586bc, calling coreclr!coreclr_shutdown_2 + 0x169a3
072ADD0C 72bd80bd coreclr + 0x580bd, calling coreclr + 0x58149
072ADD1C 72bd810d coreclr + 0x5810d, calling coreclr!coreclr_shutdown_2 + 0x169a3
072ADD74 76615847 gdi32!IcmReleaseCachedColorSpace + 0x41, calling ntdll!RtlLeaveCriticalSection
072ADD88 7716e143 ntdll!RtlFreeHeap + 0x105, calling ntdll!RtlpLowFragHeapFree
072ADDA0 76c56eaa ole32!CRetailMalloc_Free + 0x1c [d:\w7rtm\com\ole32\com\class\memapi.cxx:687], calling ntdll!RtlFreeHeap
072ADDB4 748b470c oleaut32!MemFree + 0x4e
072ADDC0 748d357c oleaut32!CPicture::~CPicture + 0xd7, calling oleaut32!_EH_epilog3
072ADDC8 748b4795 oleaut32!operator delete + 0xd, calling oleaut32!MemFree
072ADDD4 748d34ba oleaut32!CPicture::`scalar deleting destructor' + 0x19, calling oleaut32!operator delete
072ADDE4 748d349a oleaut32!CPicture::Release + 0x27, calling oleaut32!CPicture::`scalar deleting destructor'
072ADE08 0881ce31 (MethodDesc 002e28f4 + 0x99 System.Runtime.InteropServices.Marshal.Release(IntPtr))
072ADE24 0881ce31 (MethodDesc 002e28f4 + 0x99 System.Runtime.InteropServices.Marshal.Release(IntPtr))
072ADE58 08c5b0da (MethodDesc 02388b30 + 0x2b2 System.Drawing.Icon.Save(System.IO.Stream)), calling (MethodDesc 002e28f4 + 0 System.Runtime.InteropServices.Marshal.Release(IntPtr))
072ADF08 0881b5c2 (MethodDesc 0252e85c + 0x82 System.Drawing.Tests.IconTests.SaveAndCompare(System.Drawing.Icon, Boolean)), calling (MethodDesc 02388b30 + 0 System.Drawing.Icon.Save(System.IO.Stream))

[AaronRobinsonMSFT]

@eerhardt How can I get a hold of that DMP file?

[eerhardt]

I have it locally, and can send it to you. Or:

dotnet tool install --global runfo
dotnet tool update --global runfo

runfo get-helix-payload -j 2ebc65eb-6738-4487-9823-d41180299501 -w System.Drawing.Common.Tests -o %WOUTDIR%

See https://github.com/dotnet/runtime/blob/main/eng/testing/debug-dump-template.md

[kant2002]

Line using DrawingComWrappers.IPicture picture = (DrawingComWrappers.IPicture)DrawingComWrappers.Instance.GetOrCreateObjectForComInstance(lpPicture, CreateObjectFlags.None); already release instance of lpPicture in Dispose.

https://github.com/dotnet/runtime/pull/54636/files#diff-f587d46981bfe7aeb74b6390ab4035ef7355e1c5cfe849583efb14da68c0aa49R288 maybe here should be added AddRef.

[eerhardt]

Line using DrawingComWrappers.IPicture picture = (DrawingComWrappers.IPicture)DrawingComWrappers.Instance.GetOrCreateObjectForComInstance(lpPicture, CreateObjectFlags.None); already release instance of lpPicture in Dispose.

The AddRef happens (in QueryInterface) before the PictureWrapper class is created, here:

https://github.com/dotnet/runtime/pull/54636/files#diff-f587d46981bfe7aeb74b6390ab4035ef7355e1c5cfe849583efb14da68c0aa49R54-R58

Notice that x64 passes. It is just failing on x86.

[eerhardt]

There are also x64 tests failing, and these I can repro on my machine, but not 100% of the time.

The test failure is:

Assert.Throws() Failure
Expected: typeof(System.ObjectDisposedException)
Actual:   typeof(System.NotSupportedException): Specified method is not supported.
---- System.NotSupportedException : Specified method is not supported.

We are expecting an ObjectDisposedException, but a NotSupportedException is being thrown. This appears to be a quirk/bug with Marshal.ThrowExceptionForHR. I got it to repro on my machine and here's what I see in the debugger:

0x80131622 is the error code for ObjectDisposedException:

runtime/src/libraries/Common/src/System/HResults.cs

Lines 78 to 79 in 57bfe47

	internal const int COR_E_NOTSUPPORTED = unchecked((int)0x80131515);
	internal const int COR_E_OBJECTDISPOSED = unchecked((int)0x80131622);

But Marshal.ThrowExceptionForHR is throwing NotSupportedException, which is odd. @jkotas (or anyone else) have you ever seen this before? The only thing I found while searching was #47329, but that seems like something else.

[eerhardt]

There are also x64 tests failing

I was able to catch some of these in a debugger, and have figured out some more information. The case where it is failing is getting into:

runtime/src/coreclr/vm/excep.cpp

Lines 3004 to 3008 in 57bfe47

	if (pErrInfo != NULL)
	{
	// If this represents a managed object...
	// ...then get the managed exception object and also check if it is a __ComObject...
	if (IsManagedObject(pErrInfo))

pErrInfo is non-null. When this occurs, the code inside that if is setting pProtectedThrowable to a NotSupportedException, and that is what is getting returned to managed code and getting thrown by Marshal.ThrowExceptionForHR, even though I explicitly passed in 0x80131622 (COR_E_OBJECTDISPOSED) to the method.

AFAICT, the reason pErrInfo is non-null is because of this line in the calling method:

runtime/src/coreclr/vm/marshalnative.cpp

Line 647 in 57bfe47

if (SafeGetErrorInfo(&pErrorInfo) != S_OK)

The managed C# code is passing in NULL for the error info, but this line is reading it from SafeGetErrorInfo. Sometimes that is returning S_OK and an error info. From where? I don't have a clue 😄. But it feels like someone isn't clearing the previous error info here.

A naïve question: If I'm calling Marshal.ThrowExceptionForHR(int) and not passing in an errorInfo, should this code even be reading SafeGetErrorInfo? It looks like there is support to pass -1 to the native code, and it won't read it. So should this C# line

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs

Line 654 in 57bfe47

public static Exception? GetExceptionForHR(int errorCode) => GetExceptionForHR(errorCode, IntPtr.Zero);

be changed to GetExceptionForHR(errorCode, new IntPtr(-1));?

[jkotas]

Yes, COM IErrorInfo is mess. .NET interop assumes that all COM calls set it, but it is not the case in practice.

If I'm calling Marshal.ThrowExceptionForHR(int) and not passing in an errorInfo, should this code even be reading SafeGetErrorInfo?

Changing that would be breaking behavior change. It should be fine to pass in new IntPtr(-1) in the wrappers you have written in this PR.

[eerhardt]

The musl library test leg is failing HTTP and sockets tests - unrelated to this change.

The CoreClr Pri0 arm64 test failure also look unrelated, but I can’t find a current issue tracking the failures.

[eerhardt]

Test failures are unrelated. Merging.