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 1 commit
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
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
4 changes: 2 additions & 2 deletions src/coreclr/src/vm/ecalllist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,8 +958,8 @@ FCFuncStart(gObjectMarshalerFuncs)
FCFuncEnd()

FCFuncStart(gInterfaceMarshalerFuncs)
FCFuncElement("ConvertToNative", StubHelpers::InterfaceMarshaler__ConvertToNative)
FCFuncElement("ConvertToManaged", StubHelpers::InterfaceMarshaler__ConvertToManaged)
FCFuncElement("ConvertToNativeInternal", StubHelpers::InterfaceMarshaler__ConvertToNative)
FCFuncElement("ConvertToManagedInternal", StubHelpers::InterfaceMarshaler__ConvertToManaged)
QCFuncElement("ClearNative", StubHelpers::InterfaceMarshaler__ClearNative)
FCFuncElement("ConvertToManagedWithoutUnboxing", StubHelpers::InterfaceMarshaler__ConvertToManagedWithoutUnboxing)
FCFuncEnd()
Expand Down
11 changes: 11 additions & 0 deletions ...tests/src/Interop/COM/ComWrappers/MockReferenceTrackerRuntime/ReferenceTrackerRuntime.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,3 +337,14 @@ extern "C" DLL_EXPORT int STDMETHODCALLTYPE Trigger_NotifyEndOfReferenceTracking
{
return TrackerRuntimeManager.NotifyEndOfReferenceTrackingOnThread();
}

extern "C" DLL_EXPORT int STDMETHODCALLTYPE UpdateTestObject(IUnknown *obj, int i)
{
if (obj == nullptr)
return E_POINTER;

HRESULT hr;
ComSmartPtr<ITest> testObj;
RETURN_IF_FAILED(obj->QueryInterface(&testObj))
return testObj->SetValue(i);
}
30 changes: 30 additions & 0 deletions src/coreclr/tests/src/Interop/COM/ComWrappers/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,16 @@ struct MockReferenceTrackerRuntime
extern public static int Trigger_NotifyEndOfReferenceTrackingOnThread();
}

struct MarshalInterface
{
[DllImport(nameof(MockReferenceTrackerRuntime))]
[return: MarshalAs(UnmanagedType.IUnknown)]
extern public static object? CreateTrackerObject();

[DllImport(nameof(MockReferenceTrackerRuntime))]
extern public static void UpdateTestObject([MarshalAs(UnmanagedType.IUnknown)] object testObj, int i);
}

[Guid("42951130-245C-485E-B60B-4ED4254256F8")]
public interface ITrackerObject
{
Expand Down Expand Up @@ -497,6 +507,9 @@ static void ValidateGlobalInstanceScenarios()
ValidateMarshalAPIs(wrappers1, true);
ValidateMarshalAPIs(wrappers1, false);

ValidateInterfaceMarshaler(wrappers1, true);
ValidateInterfaceMarshaler(wrappers1, false);

Console.WriteLine($"Validate NotifyEndOfReferenceTrackingOnThread()...");

int hr;
Expand Down Expand Up @@ -542,6 +555,23 @@ private static void ValidateMarshalAPIs(TestComWrappers registeredWrapper, bool
Marshal.Release(trackerObjRaw);
}

private static void ValidateInterfaceMarshaler(TestComWrappers registeredWrapper, bool validateUseRegistered)
{
registeredWrapper.ReturnInvalid = !validateUseRegistered;
string scenario = validateUseRegistered ? "use registered wrapper" : "fall back to runtime";

Console.WriteLine($"Validate ConvertToNative: {scenario}...");
var testObj = new Test();
int value = 10;
MarshalInterface.UpdateTestObject(testObj, value);
Assert.AreEqual(validateUseRegistered, value == testObj.GetValue());
Assert.AreEqual(testObj, registeredWrapper.LastComputeVtablesObject, "Registered ComWrappers instance should have been called");

Console.WriteLine($"Validate ConvertToManaged: {scenario}...");
object obj = MarshalInterface.CreateTrackerObject();
Assert.AreEqual(validateUseRegistered, obj is ITrackerObjectWrapper, $"Should{(validateUseRegistered ? string.Empty : "not")} have returned {nameof(ITrackerObjectWrapper)} instance");
}

static int Main(string[] doNotUse)
{
try
Expand Down