Skip to content

Commit

Permalink
Fix ComWrappers interaction with the IReferenceTracker interface (dot…
Browse files Browse the repository at this point in the history 
…net#45624)

* Fix ComWrappers' leak in aggregation scenario

Convert Managed Object Wrapper (MOW) GC Handle from HNDTYPE_STRONG
    to HNDTYPE_REFCOUNTED.

Add new CreateObjectFlags value to indicate aggregation during
    CreateObject scenario. This isn't reflected in the managed
    .NET 5 API surface area.

In the ReferenceTracker scenario the ref count may never reach 0 so
    the MOW destructor needs to handle that case. The previous expectation
    for a null in the destructor was based on the STRONG handle logic.

During aggregation scenarios involving ReferenceTracker, ownership
    of the inner is now the responsibility of the runtime.

* ComWrappers tests
  • Loading branch information
@AaronRobinsonMSFT
AaronRobinsonMSFT authored Dec 9, 2020
1 parent 84c817b commit a7c3179
Show file tree
Hide file tree
Showing 13 changed files with 1,016 additions and 253 deletions.
215 changes: 150 additions & 65 deletions src/coreclr/interop/comwrappers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,12 +223,10 @@ namespace
namespace
{
const int32_t TrackerRefShift = 32;
const ULONGLONG TrackerRefCounter = ULONGLONG{ 1 } << TrackerRefShift;
const ULONGLONG ComRefCounter = ULONGLONG{ 1 };
const ULONGLONG TrackerRefZero = 0x0000000080000000;
const ULONGLONG TrackerRefCounter = ULONGLONG{ 1 } << TrackerRefShift;
const ULONGLONG DestroySentinel = 0x0000000080000000;
const ULONGLONG TrackerRefCountMask = 0xffffffff00000000;
const ULONGLONG ComRefCountMask = 0x000000007fffffff;
const ULONGLONG RefCountMask = 0xffffffff7fffffff;

constexpr ULONG GetTrackerCount(_In_ ULONGLONG c)
{
Expand Down Expand Up @@ -419,11 +417,29 @@ HRESULT ManagedObjectWrapper::Create(
void ManagedObjectWrapper::Destroy(_In_ ManagedObjectWrapper* wrapper)
{
_ASSERTE(wrapper != nullptr);
_ASSERTE(GetComCount(wrapper->_refCount) == 0);

// Manually trigger the destructor since placement
// new was used to allocate the object.
wrapper->~ManagedObjectWrapper();
InteropLibImports::MemFree(wrapper, AllocScenario::ManagedObjectWrapper);
// Attempt to set the destroyed bit.
LONGLONG refCount;
LONGLONG prev;
do
{
prev = wrapper->_refCount;
refCount = prev | DestroySentinel;
} while (::InterlockedCompareExchange64(&wrapper->_refCount, refCount, prev) != prev);

// The destroy sentinel represents the bit that indicates the wrapper
// should be destroyed. Since the reference count field (64-bit) holds
// two counters we rely on the singular sentinal value - no other bits
// in the 64-bit counter are set. If there are outstanding bits set it
// indicates there are still outstanding references.
if (refCount == DestroySentinel)
{
// Manually trigger the destructor since placement
// new was used to allocate the object.
wrapper->~ManagedObjectWrapper();
InteropLibImports::MemFree(wrapper, AllocScenario::ManagedObjectWrapper);
}
}

ManagedObjectWrapper::ManagedObjectWrapper(
Expand All @@ -449,48 +465,9 @@ ManagedObjectWrapper::ManagedObjectWrapper(

ManagedObjectWrapper::~ManagedObjectWrapper()
{
// If the target isn't null, then a managed object
// is going to leak.
_ASSERTE(Target == nullptr);
}

ULONGLONG ManagedObjectWrapper::UniversalRelease(_In_ ULONGLONG dec)
{
OBJECTHANDLE local = Target;

LONGLONG refCount;
if (dec == ComRefCounter)
{
_ASSERTE(dec == 1);
refCount = ::InterlockedDecrement64(&_refCount);
}
else
{
_ASSERTE(dec == TrackerRefCounter);
LONGLONG prev;
do
{
prev = _refCount;
refCount = prev - dec;
} while (::InterlockedCompareExchange64(&_refCount, refCount, prev) != prev);
}

// It is possible that a target wasn't set during an
// attempt to reactive the wrapper.
if ((RefCountMask & refCount) == 0 && local != nullptr)
{
_ASSERTE(!IsSet(CreateComInterfaceFlagsEx::IsPegged));
_ASSERTE(refCount == TrackerRefZero || refCount == 0);

// Attempt to reset the target if its current value is the same.
// It is possible the wrapper is in the middle of being reactivated.
(void)TrySetObjectHandle(nullptr, local);

// Tell the runtime to delete the managed object instance handle.
InteropLibImports::DeleteObjectInstanceHandle(local);
}

return refCount;
// If the target isn't null, then release it.
if (Target != nullptr)
InteropLibImports::DeleteObjectInstanceHandle(Target);
}

void* ManagedObjectWrapper::AsRuntimeDefined(_In_ REFIID riid)
Expand Down Expand Up @@ -551,16 +528,18 @@ void ManagedObjectWrapper::ResetFlag(_In_ CreateComInterfaceFlagsEx flag)
::InterlockedAnd((LONG*)&_flags, resetMask);
}

ULONG ManagedObjectWrapper::IsActiveAddRef()
bool ManagedObjectWrapper::IsRooted() const
{
ULONG count = GetComCount(::InterlockedIncrement64(&_refCount));
if (count == 1)
bool rooted = GetComCount(_refCount) > 0;
if (!rooted)
{
// Ensure the current target is null.
::InterlockedExchangePointer(&Target, nullptr);
// Only consider tracker ref count to be a "strong" ref count if it is pegged and alive.
rooted = (GetTrackerCount(_refCount) > 0)
&& (IsSet(CreateComInterfaceFlagsEx::IsPegged)
|| InteropLibImports::GetGlobalPeggingState());
}

return count;
return rooted;
}

ULONG ManagedObjectWrapper::AddRefFromReferenceTracker()
Expand All @@ -578,7 +557,29 @@ ULONG ManagedObjectWrapper::AddRefFromReferenceTracker()

ULONG ManagedObjectWrapper::ReleaseFromReferenceTracker()
{
return GetTrackerCount(UniversalRelease(TrackerRefCounter));
if (GetTrackerCount(_refCount) == 0)
{
_ASSERTE(!"Over release of MOW - ReferenceTracker");
return (ULONG)-1;
}

LONGLONG refCount;
LONGLONG prev;
do
{
prev = _refCount;
refCount = prev - TrackerRefCounter;
} while (::InterlockedCompareExchange64(&_refCount, refCount, prev) != prev);

// If we observe the destroy sentinel, then this release
// must destroy the wrapper.
if (refCount == DestroySentinel)
{
_ASSERTE(!IsSet(CreateComInterfaceFlagsEx::IsPegged));
Destroy(this);
}

return GetTrackerCount(refCount);
}

HRESULT ManagedObjectWrapper::Peg()
Expand Down Expand Up @@ -652,12 +653,20 @@ HRESULT ManagedObjectWrapper::QueryInterface(

ULONG ManagedObjectWrapper::AddRef(void)
{
_ASSERTE((_refCount & DestroySentinel) == 0);
return GetComCount(::InterlockedIncrement64(&_refCount));
}

ULONG ManagedObjectWrapper::Release(void)
{
return GetComCount(UniversalRelease(ComRefCounter));
_ASSERTE((_refCount & DestroySentinel) == 0);
if (GetComCount(_refCount) == 0)
{
_ASSERTE(!"Over release of MOW - COM");
return (ULONG)-1;
}

return GetComCount(::InterlockedDecrement64(&_refCount));
}

namespace
Expand All @@ -684,12 +693,19 @@ NativeObjectWrapperContext* NativeObjectWrapperContext::MapFromRuntimeContext(_I

HRESULT NativeObjectWrapperContext::Create(
_In_ IUnknown* external,
_In_opt_ IUnknown* inner,
_In_ InteropLib::Com::CreateObjectFlags flags,
_In_ size_t runtimeContextSize,
_Outptr_ NativeObjectWrapperContext** context)
{
_ASSERTE(external != nullptr && context != nullptr);

// Aggregated inners are only currently supported for Aggregated
// scenarios involving IReferenceTracker.
_ASSERTE(inner == nullptr
|| ((flags & InteropLib::Com::CreateObjectFlags_TrackerObject)
&& (flags & InteropLib::Com::CreateObjectFlags_Aggregated)));

HRESULT hr;

ComHolder<IReferenceTracker> trackerObject;
Expand All @@ -710,7 +726,7 @@ HRESULT NativeObjectWrapperContext::Create(
// Contract specifically requires zeroing out runtime context.
::memset(runtimeContext, 0, runtimeContextSize);

NativeObjectWrapperContext* contextLocal = new (cxtMem) NativeObjectWrapperContext{ runtimeContext, trackerObject };
NativeObjectWrapperContext* contextLocal = new (cxtMem) NativeObjectWrapperContext{ runtimeContext, trackerObject, inner };

if (trackerObject != nullptr)
{
Expand All @@ -722,6 +738,13 @@ HRESULT NativeObjectWrapperContext::Create(
Destroy(contextLocal);
return hr;
}

// Aggregation with a tracker object must be "cleaned up".
if (flags & InteropLib::Com::CreateObjectFlags_Aggregated)
{
_ASSERTE(inner != nullptr);
contextLocal->HandleReferenceTrackerAggregation();
}
}

*context = contextLocal;
Expand All @@ -732,28 +755,59 @@ void NativeObjectWrapperContext::Destroy(_In_ NativeObjectWrapperContext* wrappe
{
_ASSERTE(wrapper != nullptr);

// Check if the tracker object manager should be informed prior to being destroyed.
IReferenceTracker* trackerMaybe = wrapper->GetReferenceTracker();
if (trackerMaybe != nullptr)
{
// We only call this during a GC so ignore the failure as
// there is no way we can handle it at this point.
HRESULT hr = TrackerObjectManager::BeforeWrapperDestroyed(trackerMaybe);
_ASSERTE(SUCCEEDED(hr));
(void)hr;
}

// Manually trigger the destructor since placement
// new was used to allocate the object.
wrapper->~NativeObjectWrapperContext();
InteropLibImports::MemFree(wrapper, AllocScenario::NativeObjectWrapper);
}

NativeObjectWrapperContext::NativeObjectWrapperContext(_In_ void* runtimeContext, _In_opt_ IReferenceTracker* trackerObject)
namespace
{
// State ownership mechanism.
enum : int
{
TrackerObjectState_NotSet = 0,
TrackerObjectState_SetNoRelease = 1,
TrackerObjectState_SetForRelease = 2,
};
}

NativeObjectWrapperContext::NativeObjectWrapperContext(
_In_ void* runtimeContext,
_In_opt_ IReferenceTracker* trackerObject,
_In_opt_ IUnknown* nativeObjectAsInner)
: _trackerObject{ trackerObject }
, _runtimeContext{ runtimeContext }
, _isValidTracker{ (trackerObject != nullptr ? TRUE : FALSE) }
, _trackerObjectDisconnected{ FALSE }
, _trackerObjectState{ (trackerObject == nullptr ? TrackerObjectState_NotSet : TrackerObjectState_SetForRelease) }
, _nativeObjectAsInner{ nativeObjectAsInner }
#ifdef _DEBUG
, _sentinel{ LiveContextSentinel }
#endif
{
if (_isValidTracker == TRUE)
if (_trackerObjectState == TrackerObjectState_SetForRelease)
(void)_trackerObject->AddRef();
}

NativeObjectWrapperContext::~NativeObjectWrapperContext()
{
DisconnectTracker();

// If the inner was supplied, we need to release our reference.
if (_nativeObjectAsInner != nullptr)
(void)_nativeObjectAsInner->Release();

#ifdef _DEBUG
_sentinel = DeadContextSentinel;
#endif
Expand All @@ -766,12 +820,43 @@ void* NativeObjectWrapperContext::GetRuntimeContext() const noexcept

IReferenceTracker* NativeObjectWrapperContext::GetReferenceTracker() const noexcept
{
return ((_isValidTracker == TRUE) ? _trackerObject : nullptr);
return ((_trackerObjectState == TrackerObjectState_NotSet) ? nullptr : _trackerObject);
}

// See TrackerObjectManager::AfterWrapperCreated() for AddRefFromTrackerSource() usage.
// See NativeObjectWrapperContext::HandleReferenceTrackerAggregation() for additional
// cleanup logistics.
void NativeObjectWrapperContext::DisconnectTracker() noexcept
{
// Attempt to disconnect from the tracker.
if (TRUE == ::InterlockedCompareExchange((LONG*)&_isValidTracker, FALSE, TRUE))
// Return if already disconnected or the tracker isn't set.
if (FALSE != ::InterlockedCompareExchange((LONG*)&_trackerObjectDisconnected, TRUE, FALSE)
|| _trackerObjectState == TrackerObjectState_NotSet)
{
return;
}

_ASSERTE(_trackerObject != nullptr);

// Always release the tracker source during a disconnect.
// This to account for the implied IUnknown ownership by the runtime.
(void)_trackerObject->ReleaseFromTrackerSource(); // IUnknown

// Disconnect from the tracker.
if (_trackerObjectState == TrackerObjectState_SetForRelease)
{
(void)_trackerObject->ReleaseFromTrackerSource(); // IReferenceTracker
(void)_trackerObject->Release();
}
}

void NativeObjectWrapperContext::HandleReferenceTrackerAggregation() noexcept
{
_ASSERTE(_trackerObjectState == TrackerObjectState_SetForRelease && _trackerObject != nullptr);

// Aggregation with an IReferenceTracker instance creates an extra AddRef()
// on the outer (e.g. MOW) so we clean up that issue here.
_trackerObjectState = TrackerObjectState_SetNoRelease;

(void)_trackerObject->ReleaseFromTrackerSource(); // IReferenceTracker
(void)_trackerObject->Release();
}
18 changes: 10 additions & 8 deletions src/coreclr/interop/comwrappers.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,6 @@ class ManagedObjectWrapper

~ManagedObjectWrapper();

// Represents a single implementation of how to release
// the wrapper. Supplied with a decrementing value.
ULONGLONG UniversalRelease(_In_ ULONGLONG dec);

// Query the runtime defined tables.
void* AsRuntimeDefined(_In_ REFIID riid);

Expand All @@ -102,8 +98,8 @@ class ManagedObjectWrapper
void SetFlag(_In_ CreateComInterfaceFlagsEx flag);
void ResetFlag(_In_ CreateComInterfaceFlagsEx flag);

// Used while validating wrapper is active.
ULONG IsActiveAddRef();
// Indicate if the wrapper should be considered a GC root.
bool IsRooted() const;

public: // IReferenceTrackerTarget
ULONG AddRefFromReferenceTracker();
Expand Down Expand Up @@ -139,7 +135,9 @@ class NativeObjectWrapperContext
{
IReferenceTracker* _trackerObject;
void* _runtimeContext;
Volatile<BOOL> _isValidTracker;
Volatile<BOOL> _trackerObjectDisconnected;
int _trackerObjectState;
IUnknown* _nativeObjectAsInner;

#ifdef _DEBUG
size_t _sentinel;
Expand All @@ -151,6 +149,7 @@ class NativeObjectWrapperContext
// Create a NativeObjectWrapperContext instance
static HRESULT NativeObjectWrapperContext::Create(
_In_ IUnknown* external,
_In_opt_ IUnknown* nativeObjectAsInner,
_In_ InteropLib::Com::CreateObjectFlags flags,
_In_ size_t runtimeContextSize,
_Outptr_ NativeObjectWrapperContext** context);
Expand All @@ -159,7 +158,7 @@ class NativeObjectWrapperContext
static void Destroy(_In_ NativeObjectWrapperContext* wrapper);

private:
NativeObjectWrapperContext(_In_ void* runtimeContext, _In_opt_ IReferenceTracker* trackerObject);
NativeObjectWrapperContext(_In_ void* runtimeContext, _In_opt_ IReferenceTracker* trackerObject, _In_opt_ IUnknown* nativeObjectAsInner);
~NativeObjectWrapperContext();

public:
Expand All @@ -171,6 +170,9 @@ class NativeObjectWrapperContext

// Disconnect reference tracker instance.
void DisconnectTracker() noexcept;

private:
void HandleReferenceTrackerAggregation() noexcept;
};

// Manage native object wrappers that support IReferenceTracker.
Expand Down
Loading

0 comments on commit a7c3179

Please sign in to comment.