diff --git a/src/coreclr/binder/customassemblybinder.cpp b/src/coreclr/binder/customassemblybinder.cpp index 4829b19ff390b2..c223ba3cc3ac3f 100644 --- a/src/coreclr/binder/customassemblybinder.cpp +++ b/src/coreclr/binder/customassemblybinder.cpp @@ -258,6 +258,8 @@ void CustomAssemblyBinder::ReleaseLoadContext() // This method is called to release the weak and strong handles on the managed AssemblyLoadContext // once the Unloading event has been fired OBJECTHANDLE handle = reinterpret_cast(GetAssemblyLoadContext()); + + GCX_COOP(); DestroyLongWeakHandle(handle); handle = reinterpret_cast(m_ptrManagedStrongAssemblyLoadContext); DestroyHandle(handle); diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 54bde6c60c6845..35a45e207c0dd0 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -10830,16 +10830,18 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent) OBJECTHANDLE objectHandle = pEvent->DisposeHandle.vmObjectHandle.GetRawPtr(); CorDebugHandleType handleType = pEvent->DisposeHandle.handleType; + // Use the preemptive-mode-safe variant because this can run on + // the native debugger helper thread which has no managed Thread. switch (handleType) { case HANDLE_STRONG: - DestroyStrongHandle(objectHandle); + DestroyHandleInPreemptiveMode(objectHandle, HNDTYPE_STRONG); break; case HANDLE_WEAK_TRACK_RESURRECTION: - DestroyLongWeakHandle(objectHandle); + DestroyHandleInPreemptiveMode(objectHandle, HNDTYPE_WEAK_LONG); break; case HANDLE_PINNED: - DestroyPinningHandle(objectHandle); + DestroyHandleInPreemptiveMode(objectHandle, HNDTYPE_PINNED); break; default: pEvent->hr = E_INVALIDARG; @@ -12148,7 +12150,7 @@ HRESULT Debugger::UpdateForceCatchHandlerFoundTable(BOOL enableEvents, OBJECTREF } else { - DestroyLongWeakHandle(objHandle); + DestroyHandleInPreemptiveMode(objHandle, HNDTYPE_WEAK_LONG); } } else @@ -12157,7 +12159,7 @@ HRESULT Debugger::UpdateForceCatchHandlerFoundTable(BOOL enableEvents, OBJECTREF { m_pForceCatchHandlerFoundEventsTable->Remove(objHandle); } - DestroyLongWeakHandle(objHandle); + DestroyHandleInPreemptiveMode(objHandle, HNDTYPE_WEAK_LONG); } return S_OK; } diff --git a/src/coreclr/debug/ee/debugger.h b/src/coreclr/debug/ee/debugger.h index 151ea1385d25e4..c2f99e3dcebe6d 100644 --- a/src/coreclr/debug/ee/debugger.h +++ b/src/coreclr/debug/ee/debugger.h @@ -511,7 +511,7 @@ class EMPTY_BASES ForceCatchHandlerFoundSHashTraits : public DefaultSHashTraits< } static void OnRemovePerEntryCleanupAction(const OBJECTHANDLE &e) { - DestroyLongWeakHandle(e); + DestroyHandleInPreemptiveMode(e, HNDTYPE_WEAK_LONG); } }; typedef SHash ForceCatchHandlerFoundTable; diff --git a/src/coreclr/gc/gchandletable.cpp b/src/coreclr/gc/gchandletable.cpp index e70b38ebb6c339..32b9fc97f5ae7d 100644 --- a/src/coreclr/gc/gchandletable.cpp +++ b/src/coreclr/gc/gchandletable.cpp @@ -211,3 +211,8 @@ void GCHandleManager::TraceRefCountedHandles(HANDLESCANPROC callback, uintptr_t { ::Ref_TraceRefCountHandles(callback, param1, param2); } + +void GCHandleManager::DestroyHandleOfTypeLocked(OBJECTHANDLE handle, HandleType type) +{ + ::HndDestroyHandleLocked(::HndGetHandleTable(handle), type, handle); +} diff --git a/src/coreclr/gc/gchandletableimpl.h b/src/coreclr/gc/gchandletableimpl.h index 83bcab2b8fd8dd..a0eea7fc212e44 100644 --- a/src/coreclr/gc/gchandletableimpl.h +++ b/src/coreclr/gc/gchandletableimpl.h @@ -70,6 +70,8 @@ class GCHandleManager : public IGCHandleManager virtual HandleType HandleFetchType(OBJECTHANDLE handle); virtual void TraceRefCountedHandles(HANDLESCANPROC callback, uintptr_t param1, uintptr_t param2); + + virtual void DestroyHandleOfTypeLocked(OBJECTHANDLE handle, HandleType type); }; #endif // GCHANDLETABLE_H_ diff --git a/src/coreclr/gc/gcinterface.h b/src/coreclr/gc/gcinterface.h index 43bd1752cc4168..4862c0a1363ef7 100644 --- a/src/coreclr/gc/gcinterface.h +++ b/src/coreclr/gc/gcinterface.h @@ -11,7 +11,7 @@ // The minor version of the IGCHeap interface. Non-breaking changes are required // to bump the minor version number. GCs and EEs with minor version number // mismatches can still interoperate correctly, with some care. -#define GC_INTERFACE_MINOR_VERSION 8 +#define GC_INTERFACE_MINOR_VERSION 9 // The major version of the IGCToCLR interface. Breaking changes to this interface // require bumps in the major version number. @@ -638,6 +638,10 @@ class IGCHandleManager { virtual HandleType HandleFetchType(OBJECTHANDLE handle) PURE_VIRTUAL virtual void TraceRefCountedHandles(HANDLESCANPROC callback, uintptr_t param1, uintptr_t param2) PURE_VIRTUAL + + // Variant of DestroyHandleOfType that takes the handle table lock. + // Safe to call from preemptive mode or from threads not known to the runtime. + virtual void DestroyHandleOfTypeLocked(OBJECTHANDLE handle, HandleType type) PURE_VIRTUAL }; // Enum representing the type to be passed to GC.CoreCLR.cs used to deduce the type of configuration. diff --git a/src/coreclr/gc/handletable.cpp b/src/coreclr/gc/handletable.cpp index 356b0e0bdc314b..6e159d47d90526 100644 --- a/src/coreclr/gc/handletable.cpp +++ b/src/coreclr/gc/handletable.cpp @@ -396,6 +396,53 @@ void HndDestroyHandle(HHANDLETABLE hTable, uint32_t uType, OBJECTHANDLE handle) } +/* + * HndDestroyHandleLocked + * + * Entrypoint for freeing an individual handle under the handle table lock. + * Safe to call from preemptive mode or from threads not known to the runtime + * because the lock prevents races with GC scanning. + * + */ +void HndDestroyHandleLocked(HHANDLETABLE hTable, uint32_t uType, OBJECTHANDLE handle) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + CAN_TAKE_LOCK; + } + CONTRACTL_END; + + STRESS_LOG1(LF_GC, LL_INFO1000, "DestroyHandleLocked: %p\n", handle); + + FIRE_EVENT(DestroyGCHandle, (void *)handle); + FIRE_EVENT(PrvDestroyGCHandle, (void *)handle); + + // sanity check handle we are being asked to free + _ASSERTE(handle); + + // fetch the handle table pointer + HandleTable *pTable = Table(hTable); + + // sanity check the type index + _ASSERTE(uType < pTable->uTypeCount); + + _ASSERTE(HandleFetchType(handle) == uType); + + // Take the handle table lock to prevent races with GC scanning. + CrstHolder ch(&pTable->Lock); + + // return the handle to the table's cache (under lock) + TableFreeSingleHandleToCache(pTable, uType, handle); + +#if defined(ENABLE_PERF_COUNTERS) || defined(FEATURE_EVENT_TRACE) + g_dwHandles--; +#endif // defined(ENABLE_PERF_COUNTERS) || defined(FEATURE_EVENT_TRACE) +} + + /* * HndDestroyHandleOfUnknownType * diff --git a/src/coreclr/gc/handletable.h b/src/coreclr/gc/handletable.h index 4bd6aa1a7bbd6b..9568e4f3f8d556 100644 --- a/src/coreclr/gc/handletable.h +++ b/src/coreclr/gc/handletable.h @@ -68,6 +68,7 @@ uint32_t HndGetHandleTableIndex(HHANDLETABLE hTable); */ OBJECTHANDLE HndCreateHandle(HHANDLETABLE hTable, uint32_t uType, OBJECTREF object, uintptr_t lExtraInfo = 0); void HndDestroyHandle(HHANDLETABLE hTable, uint32_t uType, OBJECTHANDLE handle); +void HndDestroyHandleLocked(HHANDLETABLE hTable, uint32_t uType, OBJECTHANDLE handle); void HndDestroyHandleOfUnknownType(HHANDLETABLE hTable, OBJECTHANDLE handle); diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp index db7d66a6b0326c..58d644631bfd95 100644 --- a/src/coreclr/vm/appdomain.cpp +++ b/src/coreclr/vm/appdomain.cpp @@ -136,7 +136,15 @@ PinnedHeapHandleBucket::~PinnedHeapHandleBucket() if (m_hndHandleArray) { - DestroyStrongHandle(m_hndHandleArray); + if (GetThreadNULLOk() != NULL) + { + GCX_COOP(); + DestroyStrongHandle(m_hndHandleArray); + } + else + { + DestroyHandleInPreemptiveMode(m_hndHandleArray, HNDTYPE_STRONG); + } m_hndHandleArray = NULL; } } diff --git a/src/coreclr/vm/clrex.cpp b/src/coreclr/vm/clrex.cpp index 0c6c121b0cd8e4..c5e4112c25c8ef 100644 --- a/src/coreclr/vm/clrex.cpp +++ b/src/coreclr/vm/clrex.cpp @@ -50,7 +50,16 @@ CLRException::~CLRException() STRESS_LOG1(LF_EH, LL_INFO100, "CLRException::~CLRException destroying throwable: obj = %x\n", GetThrowableHandle()); // clear the handle first, so if we SO on destroying it, we don't have a dangling reference SetThrowableHandle(NULL); - DestroyHandle(throwableHandle); + + if (GetThreadNULLOk() != NULL) + { + GCX_COOP(); + DestroyHandle(throwableHandle); + } + else + { + DestroyHandleInPreemptiveMode(throwableHandle, HNDTYPE_DEFAULT); + } } } diff --git a/src/coreclr/vm/comcallablewrapper.cpp b/src/coreclr/vm/comcallablewrapper.cpp index a04559c8ea9030..0dd87811bf5050 100644 --- a/src/coreclr/vm/comcallablewrapper.cpp +++ b/src/coreclr/vm/comcallablewrapper.cpp @@ -1803,7 +1803,15 @@ void ComCallWrapper::ClearHandle() OBJECTHANDLE pThis = m_ppThis; if (InterlockedCompareExchangeT(&m_ppThis, NULL, pThis) == pThis) { - DestroyRefcountedHandle(pThis); + if (GetThreadNULLOk() != NULL) + { + GCX_COOP(); + DestroyRefcountedHandle(pThis); + } + else + { + DestroyHandleInPreemptiveMode(pThis, HNDTYPE_REFCOUNTED); + } } } diff --git a/src/coreclr/vm/dllimportcallback.cpp b/src/coreclr/vm/dllimportcallback.cpp index e4cb9206c9160f..b603c50a4c5ee3 100644 --- a/src/coreclr/vm/dllimportcallback.cpp +++ b/src/coreclr/vm/dllimportcallback.cpp @@ -347,7 +347,15 @@ void UMEntryThunkData::Terminate() if (pObjectHandle != NULL) { - DestroyLongWeakHandle(pObjectHandle); + if (GetThreadNULLOk() != NULL) + { + GCX_COOP(); + DestroyLongWeakHandle(pObjectHandle); + } + else + { + DestroyHandleInPreemptiveMode(pObjectHandle, HNDTYPE_WEAK_LONG); + } } s_thunkFreeList.AddToList(this); diff --git a/src/coreclr/vm/dynamicmethod.cpp b/src/coreclr/vm/dynamicmethod.cpp index 90ee69828ca602..585979f451cc0c 100644 --- a/src/coreclr/vm/dynamicmethod.cpp +++ b/src/coreclr/vm/dynamicmethod.cpp @@ -1109,6 +1109,7 @@ void LCGMethodResolver::DestroyResolver() if (m_managedResolver) { + GCX_COOP(); ::DestroyLongWeakHandle(m_managedResolver); m_managedResolver = NULL; } diff --git a/src/coreclr/vm/eedbginterface.h b/src/coreclr/vm/eedbginterface.h index f972e89e645488..3a3862130a52eb 100644 --- a/src/coreclr/vm/eedbginterface.h +++ b/src/coreclr/vm/eedbginterface.h @@ -96,8 +96,6 @@ class EEDebugInterface bool fStrongNewRef, AppDomain *pAppDomain) = 0; - virtual void DbgDestroyHandle( OBJECTHANDLE oh, bool fStrongNewRef ) = 0; - virtual OBJECTHANDLE GetThreadException(Thread *pThread) = 0; virtual bool IsThreadExceptionNull(Thread *pThread) = 0; diff --git a/src/coreclr/vm/eedbginterfaceimpl.cpp b/src/coreclr/vm/eedbginterfaceimpl.cpp index 0a461dd28dd917..7231ad1dee6ebb 100644 --- a/src/coreclr/vm/eedbginterfaceimpl.cpp +++ b/src/coreclr/vm/eedbginterfaceimpl.cpp @@ -205,30 +205,6 @@ OBJECTHANDLE EEDbgInterfaceImpl::GetHandleFromObject(void *obj, return oh; } -void EEDbgInterfaceImpl::DbgDestroyHandle(OBJECTHANDLE oh, - bool fStrongNewRef) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - } - CONTRACTL_END; - - LOG((LF_CORDB, LL_INFO1000, "EEI::GHFO: Destroyed given handle 0x%x," - "fStrong: 0x%x!\n", oh, fStrongNewRef)); - - if (fStrongNewRef) - { - DestroyStrongHandle(oh); - } - else - { - DestroyLongWeakHandle(oh); - } -} - - OBJECTHANDLE EEDbgInterfaceImpl::GetThreadException(Thread *pThread) { CONTRACTL diff --git a/src/coreclr/vm/eedbginterfaceimpl.h b/src/coreclr/vm/eedbginterfaceimpl.h index acc1b2692b8d1f..17f612620c9887 100644 --- a/src/coreclr/vm/eedbginterfaceimpl.h +++ b/src/coreclr/vm/eedbginterfaceimpl.h @@ -83,9 +83,6 @@ class EEDbgInterfaceImpl : public EEDebugInterface bool fStrongNewRef, AppDomain *pAppDomain); - void DbgDestroyHandle(OBJECTHANDLE oh, - bool fStrongNewRef); - OBJECTHANDLE GetThreadException(Thread *pThread); bool IsThreadExceptionNull(Thread *pThread); diff --git a/src/coreclr/vm/encee.cpp b/src/coreclr/vm/encee.cpp index 025df8ac2aa77f..30571e09938728 100644 --- a/src/coreclr/vm/encee.cpp +++ b/src/coreclr/vm/encee.cpp @@ -1469,20 +1469,22 @@ void EnCSyncBlockInfo::Cleanup() { NOTHROW; GC_NOTRIGGER; - MODE_ANY; + MODE_COOPERATIVE; } CONTRACTL_END; // Walk our linked list of all the fields that were added EnCAddedField *pEntry = m_pList; - while (pEntry) { - // Clean up the handle we created in EnCAddedField::Allocate - DestroyDependentHandle(*(OBJECTHANDLE*)&pEntry->m_FieldData); + while (pEntry) + { + // Clean up the handle we created in EnCAddedField::Allocate + DestroyDependentHandle(*(OBJECTHANDLE*)&pEntry->m_FieldData); - // Delete this list entry and move onto the next - EnCAddedField *next = pEntry->m_pNext; - delete pEntry; - pEntry = next; + // Delete this list entry and move onto the next + EnCAddedField *next = pEntry->m_pNext; + delete pEntry; + pEntry = next; + } } // Finally, delete the sync block info itself diff --git a/src/coreclr/vm/gchandleutilities.h b/src/coreclr/vm/gchandleutilities.h index ef5fb28a986c2e..07e76d865e1916 100644 --- a/src/coreclr/vm/gchandleutilities.h +++ b/src/coreclr/vm/gchandleutilities.h @@ -247,7 +247,7 @@ inline void DestroyHandleCommon(OBJECTHANDLE handle, HandleType type) { NOTHROW; GC_NOTRIGGER; - MODE_ANY; + MODE_COOPERATIVE; CAN_TAKE_LOCK; } CONTRACTL_END; @@ -256,6 +256,25 @@ inline void DestroyHandleCommon(OBJECTHANDLE handle, HandleType type) GCHandleUtilities::GetGCHandleManager()->DestroyHandleOfType(handle, type); } +// Variant of DestroyHandleCommon that is safe to call from preemptive mode +// and from threads that the runtime does not know about. +// It takes the handle table lock to synchronize with GC handle scanning, +// preventing races that would occur on the lock-free cache path. +inline void DestroyHandleInPreemptiveMode(OBJECTHANDLE handle, HandleType type) +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + CAN_TAKE_LOCK; + } + CONTRACTL_END; + + DiagHandleDestroyed(handle); + GCHandleUtilities::GetGCHandleManager()->DestroyHandleOfTypeLocked(handle, type); +} + inline void DestroyHandle(OBJECTHANDLE handle) { DestroyHandleCommon(handle, HNDTYPE_DEFAULT); diff --git a/src/coreclr/vm/interoplibinterface_comwrappers.cpp b/src/coreclr/vm/interoplibinterface_comwrappers.cpp index 857e4cd77bc18e..b09092919b55f2 100644 --- a/src/coreclr/vm/interoplibinterface_comwrappers.cpp +++ b/src/coreclr/vm/interoplibinterface_comwrappers.cpp @@ -236,7 +236,18 @@ namespace InteropLibImports CONTRACTL_END; ::OBJECTHANDLE objectHandle = static_cast<::OBJECTHANDLE>(handle); - DestroyRefcountedHandle(objectHandle); + // This can be called from threads without a managed Thread (e.g. via + // COM release on a native thread). Use the unsafe variant that nulls + // the handle before recycling when we cannot enter cooperative mode. + if (GetThreadNULLOk() != nullptr) + { + GCX_COOP(); + DestroyRefcountedHandle(objectHandle); + } + else + { + DestroyHandleInPreemptiveMode(objectHandle, HNDTYPE_REFCOUNTED); + } } bool GetGlobalPeggingState() noexcept diff --git a/src/coreclr/vm/jitinterface.h b/src/coreclr/vm/jitinterface.h index aebb381a0bf37d..0b7c8b86fb8f27 100644 --- a/src/coreclr/vm/jitinterface.h +++ b/src/coreclr/vm/jitinterface.h @@ -376,12 +376,17 @@ class CEEInfo : public ICorJitInfo virtual ~CEEInfo() { - LIMITED_METHOD_CONTRACT; + CONTRACTL { + NOTHROW; + GC_TRIGGERS; + } + CONTRACTL_END; #if !defined(DACCESS_COMPILE) // Free all handles used by JIT if (m_pJitHandles != nullptr) { + GCX_COOP(); OBJECTHANDLE* elements = m_pJitHandles->GetElements(); unsigned count = m_pJitHandles->GetCount(); for (unsigned i = 0; i < count; i++) diff --git a/src/coreclr/vm/syncblk.cpp b/src/coreclr/vm/syncblk.cpp index 27501d6c2f1232..654bb883e51c36 100644 --- a/src/coreclr/vm/syncblk.cpp +++ b/src/coreclr/vm/syncblk.cpp @@ -764,7 +764,7 @@ void SyncBlockCache::DeleteSyncBlock(SyncBlock *psb) INSTANCE_CHECK; THROWS; GC_TRIGGERS; - MODE_ANY; + MODE_COOPERATIVE; INJECT_FAULT(COMPlusThrowOM()); } CONTRACTL_END; diff --git a/src/coreclr/vm/threads.cpp b/src/coreclr/vm/threads.cpp index b290e205367523..450edddbfceacf 100644 --- a/src/coreclr/vm/threads.cpp +++ b/src/coreclr/vm/threads.cpp @@ -2145,19 +2145,12 @@ int Thread::DecExternalCount(BOOL holdingLock) // that is holding a reference to us. To make sure that we are not the // ones keeping the exposed object alive we need to remove the strong // reference we have to it. - if ((retVal == 1) && ((*((void**)m_StrongHndToExposedObject)) != NULL)) + if ((retVal == 1) && (m_StrongHndToExposedObject != NULL) && ((*((void**)m_StrongHndToExposedObject)) != NULL)) { - // Switch back to cooperative mode to manipulate the object. - - // Don't want to switch back to COOP until we let go of the lock - // however we are allowed to call StoreObjectInHandle here in preemptive - // mode because we are setting the value to NULL. - CONTRACT_VIOLATION(ModeViolation); - - // Clear the handle and leave the lock. - // We do not have to DisablePreemptiveGC here, because - // we just want to put NULL into a handle. - StoreObjectInHandle(m_StrongHndToExposedObject, NULL); + // Destroy the strong handle under the handle table lock to avoid + // racing with GC relocating the handle value in preemptive mode. + DestroyHandleInPreemptiveMode(m_StrongHndToExposedObject, HNDTYPE_STRONG); + m_StrongHndToExposedObject = NULL; tsLock.Release(); @@ -2167,8 +2160,6 @@ int Thread::DecExternalCount(BOOL holdingLock) pCurThread->DisablePreemptiveGC(); } - GCX_ASSERT_COOP(); - return retVal; } } @@ -2264,11 +2255,26 @@ Thread::~Thread() if (!IsAtProcessExit()) { - // Destroy any handles that we're using to hold onto exception objects - SafeSetThrowables(NULL); + // Destroy any remaining handles that weren't cleaned up by CooperativeCleanup. + // Use DestroyHandleInPreemptiveMode since the destructor can run outside + // cooperative mode (e.g., if HasStarted failed and CooperativeCleanup was skipped). + if (m_LastThrownObjectHandle != NULL && + !CLRException::IsPreallocatedExceptionHandle(m_LastThrownObjectHandle)) + { + DestroyHandleInPreemptiveMode(m_LastThrownObjectHandle, HNDTYPE_DEFAULT); + m_LastThrownObjectHandle = NULL; + } - DestroyShortWeakHandle(m_ExposedObject); - DestroyStrongHandle(m_StrongHndToExposedObject); + if (m_ExposedObject != NULL) + { + DestroyHandleInPreemptiveMode(m_ExposedObject, HNDTYPE_WEAK_SHORT); + m_ExposedObject = NULL; + } + if (m_StrongHndToExposedObject != NULL) + { + DestroyHandleInPreemptiveMode(m_StrongHndToExposedObject, HNDTYPE_STRONG); + m_StrongHndToExposedObject = NULL; + } } g_pThinLockThreadIdDispenser->DisposeId(GetThreadId()); @@ -2556,6 +2562,16 @@ void Thread::CooperativeCleanup() OBJECTREF threadObjMaybe = GetExposedObjectRaw(); if (threadObjMaybe != NULL) ((THREADBASEREF)threadObjMaybe)->SetIsDead(); + + // Destroy handles that we're using to hold onto exception objects. + // This runs under GCX_COOP() above, so it is safe to destroy handles here. + SafeSetThrowables(NULL); + + // Note: m_ExposedObject and m_StrongHndToExposedObject are intentionally + // NOT destroyed here. They are still needed by CleanupDetachedThreads() + // which calls IncExternalCount(), GetExposedObject(), and the managed + // OnThreadExiting callback after the thread has detached. + // These handles are destroyed later in ~Thread() via DestroyHandleInPreemptiveMode. } // See general comments on thread destruction (code:#threadDestruction) above.