Winforms: Flaky test: `AxHost_GetIPictureFromPicture_InvokeEnhancedMetafile_Roundtrips`

[RussKie]

• .NET Core Version: master @ 3750c43d5d9b5161be967e39fd848c89d6479ff5

Problem description:

AxHost_GetIPictureFromPicture_InvokeEnhancedMetafile_Roundtrips test fails on CI but is passing locally.
E.g.

• https://dev.azure.com/dnceng/public/_build/results?buildId=371654&view=ms.vss-test-web.build-test-results-tab&runId=11320176&resultId=113686&paneView=debug

Error message
System.Runtime.InteropServices.ExternalException : A generic error occurred in GDI+.

Stack trace
   at System.Drawing.SafeNativeMethods.Gdip.CheckStatus(Int32 status)
   at System.Drawing.Imaging.Metafile..ctor(IntPtr henhmetafile, Boolean deleteEmf)
   at System.Windows.Forms.AxHost.GetPictureFromParams(UInt32 handle, PICTYPE type, UInt32 paletteHandle, Int32 width, Int32 height) in D:\a\1\s\src\System.Windows.Forms\src\System\Windows\Forms\AxHost.cs:line 5085
   at System.Windows.Forms.AxHost.GetPictureFromIPicture(Object picture) in D:\a\1\s\src\System.Windows.Forms\src\System\Windows\Forms\AxHost.cs:line 5029
   at System.Windows.Forms.Tests.AxHostTests.SubAxHost.GetPictureFromIPicture(Object picture) in D:\a\1\s\src\System.Windows.Forms\tests\UnitTests\System\Windows\Forms\AxHostTests.cs:line 1370
   at System.Windows.Forms.Tests.AxHostTests.AxHost_GetIPictureFromPicture_InvokeEnhancedMetafile_Roundtrips() in D:\a\1\s\src\System.Windows.Forms\tests\UnitTests\System\Windows\Forms\AxHostTests.cs:line 1265

Expected behavior:

The test pass

/cc: @hughbe

[weltkante]

As noted on the other similar issues this test also seems to be not running on STA, being only annotated with a [Fact] attribute. Considering that OleInitialize native API initializes the thread to STA I'd expect all OLE functions to require STA ...

[hughbe]

So I don’t think I actually changed the IPicture interface. I just added tests. This suggests that either this is a test bug or a bug that was there before caused by either previous refactoring or that has always been there

[hughbe]

Scratch that: metafile support WAS recently added in .Net Core. Previously in .NET framework it didnt work

[weltkante]

Test failure is independent from OleInitialize.

[weltkante]

This and #2005 are interop mistakes, making OLE_HANDLE unsigned leads to the wrong sign extension, if the high bit in the handle is set this will fail the test. Had to reprogram the scenario from the test in C++ to figure out why it fails ...

USER and GDI handles are apparently sign extended (documented here and here for example), so regardless of whether OLE_HANDLE is signed or unsigned, when it contains such a handle it must be sign extended when passing it to a method taking an IntPtr.

The bug is in AxHost.GetPictureFromParams which takes an uint handle and casts it to IntPtr in various code paths. Other code paths than metafiles will fail randomly as well, for example icons and bitmaps also are GDI handles and thus must be sign extended.

Now the main question is whether OLE_HANDLE can contain something else than a USER or GDI handle, because if not then we could just turn it into an int instead of uint. Otherwise it needs careful auditing of every place which uses OLE_HANDLE to make sure they each are aware of the handle type before casting.

List of handle types requiring sign extension

Copied from the 2nd doc link above for reference.

USER handles:

• HWND
• HMENU
• HICON
• HCURSOR
• HDWP*
• HHOOK
• HACCEL
• HWINSTA
• HDESK
• HKL
• HMONITOR
• HWINEVENTHOOK*.

GDI handles:

• HBITMAP
• HPALETTE
• HMETAFILE
• HENHMETAFILE
• HMETAFILEPICT
• HBRUSH
• HFONT
• HDC
• HRGN

Also here it was mentioned the data type may be relevant for IPictureDisp interop, I don't know if you can silently exchange uint with int or if this would cause errors when doing IDispatch invocations.

/cc: @hughbe @JeremyKuhne @russkie for interop awareness. Also might want to reclassify this issue since the tests are detecting a real bug.

[RussKie]

Thank you @weltkante for taking the time and the analysis.

@JeremyKuhne @hughbe can you please lead on this?

[hughbe]

Now the main question is whether OLE_HANDLE can contain something else than a USER or GDI handle, because if not then we could just turn it into an int instead of uint. Otherwise it needs careful auditing of every place which uses OLE_HANDLE to make sure they each are aware of the handle type before casting.

In our uses we only do HDC/HPALETTE/`HBITMAP/HENHMETAFILE/HMETAFILE/HFONT which are all sign extended

[RussKie]

Thank you