Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@elinor-fung
elinor-fung committed Mar 12, 2020
1 parent 5d2b7db commit 0a3ddb4
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 23 deletions.
3 changes: 2 additions & 1 deletion src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ internal static IntPtr GetOrCreateComInterfaceForObjectInternal(ComWrappers? imp
/// 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);

Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ public static string GetTypeInfoName(ITypeInfo typeInfo)
}

// Passing null as the ComWrapper implementation will use the globally registered wrappper (if available)
IntPtr ptrMaybe = ComWrappers.GetOrCreateComInterfaceForObjectInternal(impl: null, o, CreateComInterfaceFlags.None);
IntPtr ptrMaybe = ComWrappers.GetOrCreateComInterfaceForObjectInternal(impl: null, o, CreateComInterfaceFlags.TrackerSupport);
if (ptrMaybe != IntPtr.Zero)
return ptrMaybe;

Expand Down Expand Up @@ -421,7 +421,7 @@ public static object GetObjectForIUnknown(IntPtr /* IUnknown* */ pUnk)
}

// Passing null as the ComWrapper implementation will use the globally registered wrappper (if available)
object? objMaybe = ComWrappers.GetOrCreateObjectForComInstanceInternal(impl: null, pUnk, CreateObjectFlags.None, wrapperMaybe: null);
object? objMaybe = ComWrappers.GetOrCreateObjectForComInstanceInternal(impl: null, pUnk, CreateObjectFlags.TrackerObject, wrapperMaybe: null);
if (objMaybe != null)
return objMaybe;

Expand All @@ -439,7 +439,7 @@ public static object GetUniqueObjectForIUnknown(IntPtr unknown)
}

// Passing null as the ComWrapper implementation will use the globally registered wrappper (if available)
object? objMaybe = ComWrappers.GetOrCreateObjectForComInstanceInternal(impl: null, unknown, CreateObjectFlags.UniqueInstance, wrapperMaybe: null);
object? objMaybe = ComWrappers.GetOrCreateObjectForComInstanceInternal(impl: null, unknown, CreateObjectFlags.TrackerObject | CreateObjectFlags.UniqueInstance, wrapperMaybe: null);
if (objMaybe != null)
return objMaybe;

Expand Down
41 changes: 22 additions & 19 deletions src/coreclr/src/vm/interoplibinterface.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ namespace
HRESULT hr;

SafeComHolder<IUnknown> newWrapper;
void* wrapperRaw = NULL;
void* wrapperRawMaybe = NULL;

struct
{
Expand All @@ -513,7 +513,7 @@ namespace
_ASSERTE(syncBlock->IsPrecious());

// Query the associated InteropSyncBlockInfo for an existing managed object wrapper.
if (!interopInfo->TryGetManagedObjectComWrapper(&wrapperRaw))
if (!interopInfo->TryGetManagedObjectComWrapper(&wrapperRawMaybe))
{
// Compute VTables for the new existing COM object using the supplied COM Wrappers implementation.
//
Expand All @@ -524,7 +524,7 @@ namespace
void* vtables = CallComputeVTables(&gc.implRef, &gc.instRef, flags, &vtableCount);

// Re-query the associated InteropSyncBlockInfo for an existing managed object wrapper.
if (!interopInfo->TryGetManagedObjectComWrapper(&wrapperRaw)
if (!interopInfo->TryGetManagedObjectComWrapper(&wrapperRawMaybe)
&& ((vtables != nullptr && vtableCount > 0) || (vtableCount == 0)))
{
OBJECTHANDLE instHandle = GetAppDomain()->CreateTypedHandle(gc.instRef, InstanceHandleType);
Expand All @@ -551,7 +551,7 @@ namespace

// If the managed object wrapper couldn't be set, then
// it should be possible to get the current one.
if (!interopInfo->TryGetManagedObjectComWrapper(&wrapperRaw))
if (!interopInfo->TryGetManagedObjectComWrapper(&wrapperRawMaybe))
{
UNREACHABLE();
}
Expand All @@ -564,18 +564,18 @@ namespace
{
// A new managed object wrapper was created, remove the object from the holder.
// No AddRef() here since the wrapper should be created with a reference.
wrapperRaw = newWrapper.Extract();
STRESS_LOG1(LF_INTEROP, LL_INFO100, "Created MOW: 0x%p\n", wrapperRaw);
wrapperRawMaybe = newWrapper.Extract();
STRESS_LOG1(LF_INTEROP, LL_INFO100, "Created MOW: 0x%p\n", wrapperRawMaybe);
}
else if (wrapperRaw != NULL)
else if (wrapperRawMaybe != NULL)
{
// It is possible the supplied wrapper is no longer valid. If so, reactivate the
// wrapper using the protected OBJECTREF.
IUnknown* wrapper = static_cast<IUnknown*>(wrapperRaw);
IUnknown* wrapper = static_cast<IUnknown*>(wrapperRawMaybe);
hr = InteropLib::Com::IsActiveWrapper(wrapper);
if (hr == S_FALSE)
{
STRESS_LOG1(LF_INTEROP, LL_INFO100, "Reactivating MOW: 0x%p\n", wrapperRaw);
STRESS_LOG1(LF_INTEROP, LL_INFO100, "Reactivating MOW: 0x%p\n", wrapperRawMaybe);
OBJECTHANDLE h = GetAppDomain()->CreateTypedHandle(gc.instRef, InstanceHandleType);
hr = InteropLib::Com::ReactivateWrapper(wrapper, static_cast<InteropLib::OBJECTHANDLE>(h));
}
Expand All @@ -586,7 +586,7 @@ namespace

GCPROTECT_END();

RETURN wrapperRaw;
RETURN wrapperRawMaybe;
}

OBJECTREF GetOrCreateObjectForComInstanceInternal(
Expand All @@ -610,7 +610,7 @@ namespace
{
OBJECTREF implRef;
OBJECTREF wrapperMaybeRef;
OBJECTREF objRef;
OBJECTREF objRefMaybe;
} gc;
::ZeroMemory(&gc, sizeof(gc));
GCPROTECT_BEGIN(gc);
Expand All @@ -631,7 +631,7 @@ namespace

if (extObjCxt != NULL)
{
gc.objRef = extObjCxt->GetObjectRef();
gc.objRefMaybe = extObjCxt->GetObjectRef();
}
else
{
Expand All @@ -646,15 +646,18 @@ namespace
COMPlusThrowHR(hr);

// The user could have supplied a wrapper so assign that now.
gc.objRef = gc.wrapperMaybeRef;
gc.objRefMaybe = gc.wrapperMaybeRef;

// If the wrapper hasn't been set yet, call the implementation to create one.
if (gc.objRef == NULL)
if (gc.objRefMaybe == NULL)
{
gc.objRef = CallGetObject(&gc.implRef, identity, flags);
gc.objRefMaybe = CallGetObject(&gc.implRef, identity, flags);
}

if (gc.objRef != NULL)
// The object may be null if the specified ComWrapper implementation returns null
// or there is no registered global instance. It is the caller's responsibility
// to handle this case and error if necessary.
if (gc.objRefMaybe != NULL)
{
// Construct the new context with the object details.
DWORD flags = (resultHolder.Result.FromTrackerRuntime
Expand All @@ -667,7 +670,7 @@ namespace
resultHolder.GetContext(),
identity,
GetCurrentCtxCookie(),
gc.objRef->GetSyncBlockIndex(),
gc.objRefMaybe->GetSyncBlockIndex(),
flags);

if (uniqueInstance)
Expand All @@ -686,7 +689,7 @@ namespace
if (extObjCxt == resultHolder.GetContext())
{
// Update the object's SyncBlock with a handle to the context for runtime cleanup.
SyncBlock* syncBlock = gc.objRef->GetSyncBlock();
SyncBlock* syncBlock = gc.objRefMaybe->GetSyncBlock();
InteropSyncBlockInfo* interopInfo = syncBlock->GetInteropInfo();
_ASSERTE(syncBlock->IsPrecious());

Expand Down Expand Up @@ -714,7 +717,7 @@ namespace

GCPROTECT_END();

RETURN gc.objRef;
RETURN gc.objRefMaybe;
}
}

Expand Down

0 comments on commit 0a3ddb4

Please sign in to comment.