Fishy part in the Font marshaller

[kant2002]

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Control.ActiveXFontMarshaler.cs

Lines 48 to 61 in 1bcfb9d

	Guid iid = typeof(Ole32.IFont).GUID;
	Ole32.IFont oleFont = Oleaut32.OleCreateFontIndirect(ref fontDesc, in iid);
	IntPtr pFont = Marshal.GetIUnknownForObject(oleFont);
	int hr = Marshal.QueryInterface(pFont, ref iid, out IntPtr pIFont);
	Marshal.Release(pFont);
	if (((HRESULT)hr).Failed())
	{
	Marshal.ThrowExceptionForHR(hr);
	}
	return pIFont;

I found it quite strange to create RCW out of IFont Com pointer to later convert it to IUnknow and then QI to IFont again.

[RussKie]

I don't think we'll ever find out the reason behind this code... It should be fine to do this, right? /cc: @AaronRobinsonMSFT

diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Control.ActiveXFontMarshaler.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Control.ActiveXFontMarshaler.cs
index b8d6c9df2..a5ae9a488 100644
--- a/src/System.Windows.Forms/src/System/Windows/Forms/Control.ActiveXFontMarshaler.cs
+++ b/src/System.Windows.Forms/src/System/Windows/Forms/Control.ActiveXFontMarshaler.cs
@@ -48,17 +48,7 @@ namespace System.Windows.Forms
                 Guid iid = typeof(Ole32.IFont).GUID;
                 Ole32.IFont oleFont = Oleaut32.OleCreateFontIndirect(ref fontDesc, in iid);
                 IntPtr pFont = Marshal.GetIUnknownForObject(oleFont);
-
-                int hr = Marshal.QueryInterface(pFont, ref iid, out IntPtr pIFont);
-
-                Marshal.Release(pFont);
-
-                if (((HRESULT)hr).Failed())
-                {
-                    Marshal.ThrowExceptionForHR(hr);
-                }
-
-                return pIFont;
+                return pFont;
             }

[AaronRobinsonMSFT]

This code is needed. The call to Marshal.GetIUnknownForObject(oleFont) returns the identity IUnknown. It is entirely possible that the IFont implementation is providing the default IUnknown, but it is not required, nor can it ever be assumed. From the IUnknown returned we then query for the IFont again to ensure that is the interface we have is what we want. The if (((HRESULT)hr).Failed()) could probably be removed because that already proved successful in the marshal - perhaps assert? This is a long way around getting the IntPtr out of the default COM interop marshalling.

Removing the redundancy here is easier if we update the OleCreateFontIndirect P/Invoke to the following:

[DllImport(Libraries.Oleaut32, ExactSpelling = true, EntryPoint = "OleCreateFontIndirect")]
public static extern int OleCreateIFontDispIndirect(ref FONTDESC lpFontDesc, in Guid riid, out IntPtr ptr);

This would allow you to check the return for success and then immediate return the ptr since the API will have queried for riid internally, in this case IFont.

[RussKie]

🤯 🤯 🤯

Thank you!

[RussKie]

@kant2002 are you tinkering with this code? If so, do you mind adding the Aaron's explanation as a comment? If not, let me know and I can do it.

[RussKie]

@AaronRobinsonMSFT can/should the return be HRESULT instead of int?

[AaronRobinsonMSFT]

@AaronRobinsonMSFT can/should the return be HRESULT instead of int?

That is appropriate. The OleCreateFontIndirect does return an HRESULT.