Delete GC handles in COOP mode by EgorBo · Pull Request #125350 · dotnet/runtime

EgorBo · 2026-03-09T21:46:49Z

Fixes #117138 crash.

Addresses the idea in this comment: #117138 (comment)

I've placed the GCX_COOP everywhere where the contract is either PREEMPTIVE or ANY (but also checked the callers of the callers)
It was not clear to me what is the right contract for destructors, though.

src/coreclr/vm/encee.cpp

dotnet-policy-service · 2026-03-09T21:47:47Z

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

src/coreclr/vm/syncblk.cpp

Copilot

Pull request overview

This PR addresses a GC-handle-related crash (#117138) by ensuring GC handle destruction sites run in cooperative GC mode, and by tightening the contract on common handle-destruction helpers.

Changes:

Add cooperative-mode transitions (GCX_COOP) before various GC handle destruction calls across VM, interop, debugger, and binder code paths.
Update DestroyHandleCommon’s contract to require cooperative mode (MODE_COOPERATIVE) rather than allowing any mode.
Adjust a few cleanup loops/destructors to perform handle destruction under a cooperative-mode scope.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/coreclr/vm/threads.cpp	Switch to coop before destroying thread-associated handles.
src/coreclr/vm/syncblk.cpp	Switch to coop before destroying `SyncBlock` lock handle.
src/coreclr/vm/jitinterface.h	Switch to coop before freeing per-JIT handles in `CEEInfo` dtor.
src/coreclr/vm/interoplibinterface_comwrappers.cpp	Switch to coop before destroying refcounted handles exposed to interop.
src/coreclr/vm/gchandleutilities.h	Tighten destruction helper contract to require cooperative mode.
src/coreclr/vm/exinfo.h	Switch to coop before destroying exception throwable handle.
src/coreclr/vm/exinfo.cpp	Switch to coop before destroying exception throwable handle in `ReleaseResources`.
src/coreclr/vm/encee.cpp	Switch to coop during EnC sync block cleanup when destroying dependent handles.
src/coreclr/vm/eedbginterfaceimpl.cpp	Switch to coop before destroying debug-created handles.
src/coreclr/vm/dynamicmethod.cpp	Switch to coop before destroying long weak handle for resolver.
src/coreclr/vm/dllimportcallback.cpp	Switch to coop before destroying long weak handle in thunk termination.
src/coreclr/vm/comconnectionpoints.h	Switch to coop before destroying connection cookie handle.
src/coreclr/vm/comcallablewrapper.cpp	Switch to coop before destroying refcounted CCW handle.
src/coreclr/vm/clrex.cpp	Switch to coop before destroying throwable handle in exception destructor.
src/coreclr/vm/appdomain.cpp	Switch to coop before destroying pinned heap handle bucket strong handle.
src/coreclr/debug/ee/debugger.h	Switch to coop before destroying long weak handles on SHash entry removal.
src/coreclr/debug/ee/debugger.cpp	Switch to coop before destroying debugger-disposed handles and force-catch table handles.
src/coreclr/binder/customassemblybinder.cpp	Switch to coop before destroying ALC weak/strong handles on release.

src/coreclr/debug/ee/debugger.cpp

src/coreclr/debug/ee/debugger.h

src/coreclr/vm/interoplibinterface_comwrappers.cpp

src/coreclr/debug/ee/debugger.cpp

src/coreclr/vm/eedbginterfaceimpl.cpp

src/coreclr/vm/encee.cpp

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/vm/jitinterface.h:399

CEEInfo::~CEEInfo() is still annotated with LIMITED_METHOD_CONTRACT, but it now explicitly switches GC mode (GCX_COOP) and destroys GC handles. LIMITED_METHOD_CONTRACT is intended for trivial leaf methods, and this destructor is doing non-trivial runtime/GC work; the contract should be updated to a CONTRACTL (or equivalent) that accurately declares the GC mode behavior and lock-taking semantics of the handle destruction path.

    virtual ~CEEInfo()
    {
        LIMITED_METHOD_CONTRACT;

#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++)
            {

Copilot · 2026-03-10T02:19:56Z

src/coreclr/vm/interoplibinterface_comwrappers.cpp

+        // This can be called from threads without a managed Thread (e.g. via
+        // COM release on a native thread). Only switch to cooperative mode
+        // when a managed thread exists; otherwise the MODE_COOPERATIVE
+        // contract check in DestroyHandleCommon is a no-op.
+        if (GetThreadNULLOk() != nullptr)
+        {
+            GCX_COOP();
+            DestroyRefcountedHandle(objectHandle);
+        }
+        else
+        {
+            DestroyRefcountedHandle(objectHandle);
+        }


DestroyHandle is called in both branches here; the only difference is whether a GCX_COOP scope is entered. This can be simplified to avoid duplicated code (e.g., conditionally enter the coop scope when a managed thread exists, then call DestroyRefcountedHandle once).

jkotas · 2026-03-10T02:23:20Z

src/coreclr/vm/interoplibinterface_comwrappers.cpp

        ::OBJECTHANDLE objectHandle = static_cast<::OBJECTHANDLE>(handle);

-        DestroyRefcountedHandle(objectHandle);
+        // This can be called from threads without a managed Thread (e.g. via


This will make the contract infrastructure happy, but it is not fixing the race condition on this path.

We may be able to create a slow DestroyHandle variant that is safe to call in pre-emptive mode, including threads that the runtime does not know about.

jkotas · 2026-03-11T01:50:32Z

src/coreclr/vm/gchandleutilities.h

+    }
+    CONTRACTL_END;
+
+    // Null the handle value first so that GC will not follow a stale


This won't solve the problem. DestroyHandleOfType nulls the handle already (first line in TableFreeSingleHandleToCache ).

We need to make sure that the GC handles are not getting scanned by the GC when the handle is released in preemptive mode. We can do that by taking the "handle manager lock" - take the slow path in GC handle allocator by skipping all lock-free caches.

Copilot

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/coreclr/debug/ee/debugger.cpp:11003

GCX_COOP_EEINTERFACE_IFTHREAD() is a no-op when running on the native debugger helper thread (no managed Thread), but the code still calls DestroyHandle helpers that ultimately use the normal (unlocked) handle free path. If DisposeHandle can run concurrently with GC, this still risks the original race/crash. Consider using the new locked/unsafe handle destruction path when g_pEEInterface->GetThread() == NULL (mapping CorDebugHandleType to the corresponding HNDTYPE_), similar to other call sites that branch on GetThreadNULLOk().

            // Switch to cooperative mode if a managed thread exists.
            // On the native debugger helper thread (no managed Thread),
            // the IFTHREAD variant is a no-op and the MODE_COOPERATIVE
            // contract in DestroyHandleCommon is also a no-op.
            GCX_COOP_EEINTERFACE_IFTHREAD();

            switch (handleType)
            {
            case HANDLE_STRONG:
                DestroyStrongHandle(objectHandle);
                break;
            case HANDLE_WEAK_TRACK_RESURRECTION:
                DestroyLongWeakHandle(objectHandle);
                break;
            case HANDLE_PINNED:
                DestroyPinningHandle(objectHandle);
                break;

Copilot · 2026-03-11T03:19:00Z

src/coreclr/gc/handletable.cpp

+    CrstHolder ch(&pTable->Lock);
+
+    // return the handle to the table's cache (under lock)
+    TableFreeSingleHandleToCache(pTable, uType, handle);


HndDestroyHandleLocked takes pTable->Lock and then calls TableFreeSingleHandleToCache, which can call TableCacheMissOnFree and attempt to take pTable->Lock again. That re-entrant lock acquisition can deadlock. Consider implementing a lock-aware free path (e.g., a TableFreeSingleHandleToCacheLocked variant that, on cache miss, calls TableQuickRebalanceCache directly under the already-held lock) instead of calling the existing helper that may lock internally.

Suggested change

TableFreeSingleHandleToCache(pTable, uType, handle);

TableFreeSingleHandleToCacheLocked(pTable, uType, handle);

Copilot · 2026-03-11T03:19:00Z

src/coreclr/vm/gchandleutilities.h

+        CAN_TAKE_LOCK;
+    }
+    CONTRACTL_END;
+


DestroyHandleUnsafe bypasses DestroyHandleCommon and currently does not call DiagHandleDestroyed. That means GC profiling callbacks (HandleDestroyed) won’t fire for handle destroys that go through the unsafe path. If the unsafe path is intended to be functionally equivalent (just safer wrt GC scanning), it should likely invoke DiagHandleDestroyed as well (possibly guarded if profiler callbacks require an attached runtime thread).

Suggested change

DiagHandleDestroyed(handle);

Copilot · 2026-03-11T03:19:00Z

src/coreclr/gc/handletable.cpp

+    STRESS_LOG2(LF_GC, LL_INFO1000, "DestroyHandleLocked: *%p->%p\n", handle, *(_UNCHECKED_OBJECTREF *)handle);
+


HndDestroyHandleLocked dereferences the handle value in the STRESS_LOG call before acquiring pTable->Lock. This defeats the purpose of the locked variant (safe w/ concurrent GC scanning) and can still race/crash when called from preemptive mode / unknown threads. Consider moving any handle-value dereference (or removing it from logging) until after the table lock is held.

jkotas · 2026-03-11T04:02:12Z

src/coreclr/vm/threads.cpp


-        DestroyShortWeakHandle(m_ExposedObject);
-        DestroyStrongHandle(m_StrongHndToExposedObject);
+        // The thread is already detached from TLS (GetThread() returns NULL),


We can move this to Thread::CooperativeCleanup.

jkotas · 2026-03-11T04:04:50Z

src/coreclr/vm/threads.cpp

@@ -2297,8 +2297,11 @@ Thread::~Thread()
        // Destroy any handles that we're using to hold onto exception objects
        SafeSetThrowables(NULL);


SetLastThrownObject can call DestroyHandle too

jkotas · 2026-03-11T04:07:57Z

src/coreclr/vm/gchandleutilities.h

+// and from threads that the runtime does not know about.
+// It takes the handle table lock to prevent races with GC scanning,
+// bypassing the lock-free cache path.
+inline void DestroyHandleUnsafe(OBJECTHANDLE handle, HandleType type)


This can use more descriptive name, like DestroyHandleInPreeemptiveMode

Delete GC handles in COOP mode

d427657

Copilot AI review requested due to automatic review settings March 9, 2026 21:46

github-actions bot added the area-VM-coreclr label Mar 9, 2026

dotnet-policy-service bot assigned EgorBo Mar 9, 2026

EgorBo commented Mar 9, 2026

View reviewed changes

src/coreclr/vm/encee.cpp Show resolved Hide resolved

Copilot started reviewing on behalf of EgorBo March 9, 2026 21:47 View session

EgorBo commented Mar 9, 2026

View reviewed changes

src/coreclr/vm/syncblk.cpp Outdated Show resolved Hide resolved

Copilot AI reviewed Mar 9, 2026

View reviewed changes

address feedback

ad930c0

jkotas reviewed Mar 10, 2026

View reviewed changes

src/coreclr/vm/eedbginterfaceimpl.cpp Outdated Show resolved Hide resolved

jkotas reviewed Mar 10, 2026

View reviewed changes

src/coreclr/vm/encee.cpp Outdated Show resolved Hide resolved

Feedback

f72dfce

Copilot AI review requested due to automatic review settings March 10, 2026 02:11

Copilot started reviewing on behalf of EgorBo March 10, 2026 02:12 View session

Feedback

926e258

Copilot AI reviewed Mar 10, 2026

View reviewed changes

jkotas reviewed Mar 10, 2026

View reviewed changes

This was referenced Mar 10, 2026

Tests failing on tvos with "The app 'net.dot.System.Runtime.Tests' terminated with signal 11" #124072

Open

[8.0] System.Exception: Failed to list devices - tvOS, iOS #125135

Open

will this work?

1248f0b

jkotas reviewed Mar 11, 2026

View reviewed changes

Add HndDestroyHandleLocked

e5128fd

Copilot AI review requested due to automatic review settings March 11, 2026 03:10

Copilot started reviewing on behalf of EgorBo March 11, 2026 03:13 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

jkotas reviewed Mar 11, 2026

View reviewed changes

	TableFreeSingleHandleToCache(pTable, uType, handle);
	TableFreeSingleHandleToCacheLocked(pTable, uType, handle);

		STRESS_LOG2(LF_GC, LL_INFO1000, "DestroyHandleLocked: %p->%p\n", handle, (_UNCHECKED_OBJECTREF *)handle);

		@@ -2297,8 +2297,11 @@ Thread::~Thread()
		// Destroy any handles that we're using to hold onto exception objects
		SafeSetThrowables(NULL);

Conversation

EgorBo commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dotnet-policy-service bot commented Mar 9, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EgorBo commented Mar 9, 2026 •

edited

Loading