Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use registered ComWrappers for object <-> COM interface #33485

Merged
merged 12 commits into from
Mar 25, 2020
Merged

Use registered ComWrappers for object <-> COM interface #33485

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5d2b7db
Use registered ComWrappers in Marshal APIs for object <-> IUnknown
elinor-fung Mar 11, 2020
0a3ddb4
PR feedback
elinor-fung Mar 11, 2020
dad304e
Switch to Try semantics
elinor-fung Mar 12, 2020
234c733
Check if global instance is registered
elinor-fung Mar 12, 2020
e090745
PR feedback
elinor-fung Mar 13, 2020
17e6db5
Use registered ComWrappers in InterfaceMarshaaler for object <-> IUnk…
elinor-fung Mar 17, 2020
ad962e7
PR feedback
elinor-fung Mar 17, 2020
33280fe
Use registered ComWrappers for all object <-> COM interface
elinor-fung Mar 20, 2020
6ff4d2b
Add tests
elinor-fung Mar 24, 2020
9581b57
Fix flag for marking as COM-activated
elinor-fung Mar 24, 2020
10ef236
PR fixes
elinor-fung Mar 24, 2020
574af19
PR feedback
elinor-fung Mar 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 75 additions & 15 deletions src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,35 @@ private struct ComInterfaceInstance
/// <param name="flags">Flags used to configure the generated interface.</param>
/// <returns>The generated COM interface that can be passed outside the .NET runtime.</returns>
public IntPtr GetOrCreateComInterfaceForObject(object instance, CreateComInterfaceFlags flags)
{
IntPtr ptr;
if (!TryGetOrCreateComInterfaceForObjectInternal(this, instance, flags, out ptr))
throw new ArgumentException();

return ptr;
}

/// <summary>
/// Create a COM representation of the supplied object that can be passed to a non-managed environment.
/// </summary>
/// <param name="impl">The <see cref="ComWrappers" /> implemenentation to use when creating the COM representation.</param>
/// <param name="instance">The managed object to expose outside the .NET runtime.</param>
/// <param name="flags">Flags used to configure the generated interface.</param>
/// <param name="retValue">The generated COM interface that can be passed outside the .NET runtime or IntPtr.Zero if it could not be created.</param>
/// <returns>Returns <c>true</c> if a COM representation could be created, <c>false</c> otherwise</returns>
/// <remarks>
/// If <paramref name="impl" /> is <c>null</c>, the global instance (if registered) will be used.
/// </remarks>
internal static bool TryGetOrCreateComInterfaceForObjectInternal(ComWrappers? impl, object instance, CreateComInterfaceFlags flags, out IntPtr retValue)
{
if (instance == null)
throw new ArgumentNullException(nameof(instance));

ComWrappers impl = this;
return GetOrCreateComInterfaceForObjectInternal(ObjectHandleOnStack.Create(ref impl), ObjectHandleOnStack.Create(ref instance), flags);
return TryGetOrCreateComInterfaceForObjectInternal(ObjectHandleOnStack.Create(ref impl), ObjectHandleOnStack.Create(ref instance), flags, out retValue);
}

[DllImport(RuntimeHelpers.QCall)]
private static extern IntPtr GetOrCreateComInterfaceForObjectInternal(ObjectHandleOnStack comWrappersImpl, ObjectHandleOnStack instance, CreateComInterfaceFlags flags);
private static extern bool TryGetOrCreateComInterfaceForObjectInternal(ObjectHandleOnStack comWrappersImpl, ObjectHandleOnStack instance, CreateComInterfaceFlags flags, out IntPtr retValue);

/// <summary>
/// Compute the desired Vtable for <paramref name="obj"/> respecting the values of <paramref name="flags"/>.
Expand All @@ -140,13 +159,23 @@ public IntPtr GetOrCreateComInterfaceForObject(object instance, CreateComInterfa
/// All memory returned from this function must either be unmanaged memory, pinned managed memory, or have been
/// allocated with the <see cref="System.Runtime.CompilerServices.RuntimeHelpers.AllocateTypeAssociatedMemory(Type, int)"/> API.
///
/// If the interface entries cannot be created and <code>null</code> is returned, the call to <see cref="ComWrappers.GetOrCreateComInterfaceForObject(object, CreateComInterfaceFlags)"/> will throw a <see cref="System.ArgumentNullException"/>.
/// If the interface entries cannot be created and a negative <paramref name="count" /> or <code>null</code> and a non-zero <paramref name="count" /> are returned,
/// the call to <see cref="ComWrappers.GetOrCreateComInterfaceForObject(object, CreateComInterfaceFlags)"/> will throw a <see cref="System.ArgumentException"/>.
/// </remarks>
protected unsafe abstract ComInterfaceEntry* ComputeVtables(object obj, CreateComInterfaceFlags flags, out int count);

// Call to execute the abstract instance function
internal static unsafe void* CallComputeVtables(ComWrappers? comWrappersImpl, object obj, CreateComInterfaceFlags flags, out int count)
=> (comWrappersImpl ?? s_globalInstance!).ComputeVtables(obj, flags, out count);
{
ComWrappers? impl = comWrappersImpl ?? s_globalInstance;
if (impl is null)
{
count = -1;
return null;
}

return impl.ComputeVtables(obj, flags, out count);
}

/// <summary>
/// Get the currently registered managed object or creates a new managed object and registers it.
Expand All @@ -156,7 +185,11 @@ public IntPtr GetOrCreateComInterfaceForObject(object instance, CreateComInterfa
/// <returns>Returns a managed object associated with the supplied external COM object.</returns>
public object GetOrCreateObjectForComInstance(IntPtr externalComObject, CreateObjectFlags flags)
{
return GetOrCreateObjectForComInstanceInternal(externalComObject, flags, null);
object? obj;
if (!TryGetOrCreateObjectForComInstanceInternal(this, externalComObject, flags, null, out obj))
throw new ArgumentNullException();

return obj!;
}

/// <summary>
Expand All @@ -172,7 +205,13 @@ public object GetOrCreateObjectForComInstance(IntPtr externalComObject, CreateOb

// Call to execute the abstract instance function
internal static object? CallCreateObject(ComWrappers? comWrappersImpl, IntPtr externalComObject, CreateObjectFlags flags)
=> (comWrappersImpl ?? s_globalInstance!).CreateObject(externalComObject, flags);
{
ComWrappers? impl = comWrappersImpl ?? s_globalInstance;
if (impl == null)
return null;

return impl.CreateObject(externalComObject, flags);
}

/// <summary>
/// Get the currently registered managed object or uses the supplied managed object and registers it.
Expand All @@ -189,24 +228,37 @@ public object GetOrRegisterObjectForComInstance(IntPtr externalComObject, Create
if (wrapper == null)
throw new ArgumentNullException(nameof(externalComObject));

return GetOrCreateObjectForComInstanceInternal(externalComObject, flags, wrapper);
object? obj;
if (!TryGetOrCreateObjectForComInstanceInternal(this, externalComObject, flags, wrapper, out obj))
throw new ArgumentNullException();

return obj!;
}

private object GetOrCreateObjectForComInstanceInternal(IntPtr externalComObject, CreateObjectFlags flags, object? wrapperMaybe)
/// <summary>
/// Get the currently registered managed object or creates a new managed object and registers it.
/// </summary>
/// <param name="impl">The <see cref="ComWrappers" /> implemenentation to use when creating the managed object.</param>
elinor-fung marked this conversation as resolved.
Show resolved Hide resolved
/// <param name="externalComObject">Object to import for usage into the .NET runtime.</param>
/// <param name="flags">Flags used to describe the external object.</param>
/// <param name="wrapperMaybe">The <see cref="object"/> to be used as the wrapper for the external object.</param>
/// <param name="retValue">The managed object associated with the supplied external COM object or <c>null</c> if it could not be created.</param>
/// <returns>Returns <c>true</c> if a managed object could be retrieved/created, <c>false</c> otherwise</returns>
/// <remarks>
/// If <paramref name="impl" /> is <c>null</c>, the global instance (if registered) will be used.
/// </remarks>
internal static bool TryGetOrCreateObjectForComInstanceInternal(ComWrappers? impl, IntPtr externalComObject, CreateObjectFlags flags, object? wrapperMaybe, out object? retValue)
{
if (externalComObject == IntPtr.Zero)
throw new ArgumentNullException(nameof(externalComObject));

ComWrappers impl = this;
object? wrapperMaybeLocal = wrapperMaybe;
object? retValue = null;
GetOrCreateObjectForComInstanceInternal(ObjectHandleOnStack.Create(ref impl), externalComObject, flags, ObjectHandleOnStack.Create(ref wrapperMaybeLocal), ObjectHandleOnStack.Create(ref retValue));

return retValue!;
retValue = null;
return TryGetOrCreateObjectForComInstanceInternal(ObjectHandleOnStack.Create(ref impl), externalComObject, flags, ObjectHandleOnStack.Create(ref wrapperMaybeLocal), ObjectHandleOnStack.Create(ref retValue));
}

[DllImport(RuntimeHelpers.QCall)]
private static extern void GetOrCreateObjectForComInstanceInternal(ObjectHandleOnStack comWrappersImpl, IntPtr externalComObject, CreateObjectFlags flags, ObjectHandleOnStack wrapper, ObjectHandleOnStack retValue);
private static extern bool TryGetOrCreateObjectForComInstanceInternal(ObjectHandleOnStack comWrappersImpl, IntPtr externalComObject, CreateObjectFlags flags, ObjectHandleOnStack wrapper, ObjectHandleOnStack retValue);

/// <summary>
/// Called when a request is made for a collection of objects to be released outside of normal object or COM interface lifetime.
Expand Down Expand Up @@ -237,6 +289,14 @@ public void RegisterAsGlobalInstance()
}
}

/// <summary>
/// Get whether or not a global <see cref="ComWrappers" /> instance has been registered.
/// </summary>
internal static bool IsGlobalInstanceRegistered()
{
return s_globalInstance != null;
}

/// <summary>
/// Get the runtime provided IUnknown implementation.
/// </summary>
Expand Down
89 changes: 86 additions & 3 deletions src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,21 @@ public static string GetTypeInfoName(ITypeInfo typeInfo)
/// </summary>
public static IntPtr /* IUnknown* */ GetIUnknownForObject(object o)
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
if (o is null)
{
throw new ArgumentNullException(nameof(o));
}

if (ComWrappers.IsGlobalInstanceRegistered())
{
// Passing null as the ComWrapper implementation will use the globally registered wrappper (if available)
IntPtr ptrMaybe;
if (ComWrappers.TryGetOrCreateComInterfaceForObjectInternal(impl: null, o, CreateComInterfaceFlags.TrackerSupport, out ptrMaybe))
{
return ptrMaybe;
}
}

return GetIUnknownForObjectNative(o, false);
}

Expand All @@ -344,6 +359,11 @@ public static string GetTypeInfoName(ITypeInfo typeInfo)
/// </summary>
public static IntPtr /* IDispatch */ GetIDispatchForObject(object o)
{
if (o is null)
{
throw new ArgumentNullException(nameof(o));
}

return GetIDispatchForObjectNative(o, false);
}

Expand All @@ -356,6 +376,16 @@ public static string GetTypeInfoName(ITypeInfo typeInfo)
/// </summary>
public static IntPtr /* IUnknown* */ GetComInterfaceForObject(object o, Type T)
{
if (o is null)
{
throw new ArgumentNullException(nameof(o));
}

if (T is null)
{
throw new ArgumentNullException(nameof(T));
}

return GetComInterfaceForObjectNative(o, T, false, true);
}

Expand All @@ -368,15 +398,68 @@ public static string GetTypeInfoName(ITypeInfo typeInfo)
/// </summary>
public static IntPtr /* IUnknown* */ GetComInterfaceForObject(object o, Type T, CustomQueryInterfaceMode mode)
{
if (o is null)
{
throw new ArgumentNullException(nameof(o));
}

if (T is null)
{
throw new ArgumentNullException(nameof(T));
}

bool bEnableCustomizedQueryInterface = ((mode == CustomQueryInterfaceMode.Allow) ? true : false);
return GetComInterfaceForObjectNative(o, T, false, bEnableCustomizedQueryInterface);
}

[MethodImpl(MethodImplOptions.InternalCall)]
private static extern IntPtr /* IUnknown* */ GetComInterfaceForObjectNative(object o, Type t, bool onlyInContext, bool fEnalbeCustomizedQueryInterface);
private static extern IntPtr /* IUnknown* */ GetComInterfaceForObjectNative(object o, Type t, bool onlyInContext, bool fEnableCustomizedQueryInterface);

/// <summary>
/// Return the managed object representing the IUnknown*
/// </summary>
public static object GetObjectForIUnknown(IntPtr /* IUnknown* */ pUnk)
{
if (pUnk == IntPtr.Zero)
{
throw new ArgumentNullException(nameof(pUnk));
}

elinor-fung marked this conversation as resolved.
Show resolved Hide resolved
if (ComWrappers.IsGlobalInstanceRegistered())
{
// Passing null as the ComWrapper implementation will use the globally registered wrappper (if available)
object? objMaybe;
if (ComWrappers.TryGetOrCreateObjectForComInstanceInternal(impl: null, pUnk, CreateObjectFlags.TrackerObject, wrapperMaybe: null, out objMaybe))
{
return objMaybe!;
}
}

return GetObjectForIUnknownNative(pUnk);
}

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern object GetObjectForIUnknown(IntPtr /* IUnknown* */ pUnk);
private static extern object GetObjectForIUnknownNative(IntPtr /* IUnknown* */ pUnk);

public static object GetUniqueObjectForIUnknown(IntPtr unknown)
{
if (unknown == IntPtr.Zero)
{
throw new ArgumentNullException(nameof(unknown));
}

elinor-fung marked this conversation as resolved.
Show resolved Hide resolved
if (ComWrappers.IsGlobalInstanceRegistered())
{
// Passing null as the ComWrapper implementation will use the globally registered wrappper (if available)
object? objMaybe;
if (ComWrappers.TryGetOrCreateObjectForComInstanceInternal(impl: null, unknown, CreateObjectFlags.TrackerObject | CreateObjectFlags.UniqueInstance, wrapperMaybe: null, out objMaybe))
{
return objMaybe!;
}
}

return GetUniqueObjectForIUnknownNative(unknown);
}

/// <summary>
/// Return a unique Object given an IUnknown. This ensures that you receive a fresh
Expand All @@ -385,7 +468,7 @@ public static string GetTypeInfoName(ITypeInfo typeInfo)
/// ReleaseComObject on a RCW and not worry about other active uses ofsaid RCW.
/// </summary>
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern object GetUniqueObjectForIUnknown(IntPtr unknown);
private static extern object GetUniqueObjectForIUnknownNative(IntPtr unknown);

/// <summary>
/// Return an Object for IUnknown, using the Type T.
Expand Down
54 changes: 52 additions & 2 deletions src/coreclr/src/System.Private.CoreLib/src/System/StubHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,11 +686,61 @@ internal static long ConvertToManaged(double nativeDate)
#if FEATURE_COMINTEROP
internal static class InterfaceMarshaler
{
// See interopconverter.h
[Flags]
private enum ItfMarshalFlags : int
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
ITF_MARSHAL_INSP_ITF = 0x01,
ITF_MARSHAL_SUPPRESS_ADDREF = 0x02,
ITF_MARSHAL_CLASS_IS_HINT = 0x04,
ITF_MARSHAL_DISP_ITF = 0x08,
ITF_MARSHAL_USE_BASIC_ITF = 0x10,
ITF_MARSHAL_WINRT_SCENARIO = 0x20,
};

private static bool IsForIUnknown(int flags)
{
ItfMarshalFlags interfaceFlags = (ItfMarshalFlags)flags;
return (interfaceFlags & ItfMarshalFlags.ITF_MARSHAL_USE_BASIC_ITF) != 0
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
&& (interfaceFlags & ItfMarshalFlags.ITF_MARSHAL_INSP_ITF) == 0
&& (interfaceFlags & ItfMarshalFlags.ITF_MARSHAL_DISP_ITF) == 0;
}

internal static IntPtr ConvertToNative(object objSrc, IntPtr itfMT, IntPtr classMT, int flags)
{
if (ComWrappers.IsGlobalInstanceRegistered() && IsForIUnknown(flags))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to handle all cases? I think this needs to handle all cases in order to be useful.

For example, I can imagine this being useful for statically pre-compiled COM interop carried with the apps.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine this being useful for statically pre-compiled COM interop carried with the apps.

This may be a good test case: Take a test or an app that does a bunch of COM interop, and inject the COM interop into it via this so that the build-in COM interop is not used.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this all encompassing approach is being done now - was hoping for post .NET 5 - then we should also update Marshal.GetIDispatchForObject().

Edit: IDispatch shouldn't be support here.
Edit2: Adding back after offline conversation/convincing this will be okay.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this after .NET 5 would be a breaking change, so we would need to think twice about the breaking potential, etc.

It would be best to try to make this work the way we want for .NET 5.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of the next major version (e.g. .NET 6). I appreciate the breaking change concern, which is why I thought we were limiting this to just the CsWinRT scenarios since learnings would fall out of that and we could fold that into general support. My concern with this now is merely the lack of concrete best practices and we will probably need enhancements (e.g. new enums values) - without some real world learnings I'm not confident the API is ready.

That could be me just being shy of providing such a base building block, but I don't really see the need to get it completely wired up when the only scenario that really needs it is CsWinRT for .NET 5.

Not say that we can't wire all it up, but I don't see the pressing concern to push something right now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is going to be a public API. It is likely that other people are going to show up who will start using it.

If we are not confident in the shape and the behavior of the API, we should ship it as experimental (e.g. similar to Utf8 string) so that it is not present in the officially supported public surface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just do it. I am not sure how to convey my concerns properly.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with all options except for the IDispatch support. We shouldn't expect the ComWrappers to support that yet since it is so onerous. Perhaps we can address that in the future but for right now I would prefer if we at least block that support.

Edit: I have been convinced this isn't as big a concern as I was making it out to be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to basically do this for everything now (including COM activation) if there is a ComWrappers registered globally.

{
// Passing null as the ComWrapper implementation will use the globally registered wrappper (if available)
IntPtr ptrMaybe;
if (ComWrappers.TryGetOrCreateComInterfaceForObjectInternal(impl: null, objSrc, CreateComInterfaceFlags.TrackerSupport, out ptrMaybe))
{
return ptrMaybe;
}
}

return ConvertToNativeInternal(objSrc, itfMT, classMT, flags);
}

[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern IntPtr ConvertToNative(object objSrc, IntPtr itfMT, IntPtr classMT, int flags);
internal static extern IntPtr ConvertToNativeInternal(object objSrc, IntPtr itfMT, IntPtr classMT, int flags);

internal static object ConvertToManaged(IntPtr pUnk, IntPtr itfMT, IntPtr classMT, int flags)
{
if (ComWrappers.IsGlobalInstanceRegistered() && IsForIUnknown(flags))
{
// Passing null as the ComWrapper implementation will use the globally registered wrappper (if available)
object? objMaybe;
if (ComWrappers.TryGetOrCreateObjectForComInstanceInternal(impl: null, Marshal.ReadIntPtr(pUnk), CreateObjectFlags.TrackerObject, wrapperMaybe: null, out objMaybe))
{
return objMaybe!;
}
}

return ConvertToManagedInternal(pUnk, itfMT, classMT, flags);
}

[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern object ConvertToManaged(IntPtr pUnk, IntPtr itfMT, IntPtr classMT, int flags);
internal static extern object ConvertToManagedInternal(IntPtr pUnk, IntPtr itfMT, IntPtr classMT, int flags);

[DllImport(RuntimeHelpers.QCall)]
internal static extern void ClearNative(IntPtr pUnk);
Expand Down
Loading