Reconcile ReleaseHolder, ComHolderPreemp, and ComHolderAnyMode by AaronRobinsonMSFT · Pull Request #130373 · dotnet/runtime

AaronRobinsonMSFT · 2026-07-08T19:25:13Z

Converge the three COM-interface RAII holders (ReleaseHolder, ComHolderPreemp, ComHolderAnyMode) onto the LifetimeHolder<Traits> pattern.

Changes

ReleaseHolder is retargeted to LifetimeHolder<ReleaseHolderTraits<T>>, adopting ComHolderPreemp's preemptive-mode release semantics (the VM default). Free() uses a MODE_PREEMPTIVE contract gated on ENABLE_CONTRACTS_IMPL, so non-VM consumers (utilcode, md, DAC, createdump) fall back to a static-contract annotation.
ComHolderPreemp is removed; ReleaseHolder replaces all uses.
ComHolderAnyMode is renamed to ReleaseHolderAnyMode.

dotnet-policy-service · 2026-07-08T19:27:09Z

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

Copilot

Pull request overview

This PR refactors CoreCLR’s COM / ref-counted RAII “holder” usage to converge on the LifetimeHolder<Traits> pattern: replacing legacy SpecializedWrapper APIs (Extract, Clear, GetValue, etc.), removing ComHolderPreemp, and renaming ComHolderAnyMode to ReleaseHolderAnyMode.

Changes:

Retargets ReleaseHolder to a LifetimeHolder<ReleaseHolderTraits<T>> implementation and introduces HolderDetail::HasReleaseMethod for duck-typed Release() detection.
Renames / consolidates COM holders (ComHolderAnyMode → ReleaseHolderAnyMode; removes ComHolderPreemp) and migrates call sites to Detach() / Free() patterns.
Refactors a few lifetime/ownership patterns (notably AppDomain::BindAssemblySpec) to avoid relying on legacy holder APIs.

Show a summary per file

File	Description
src/coreclr/vm/wrappers.h	Renames `ComHolderAnyModeTraits`/alias to `ReleaseHolderAnyMode*` and removes `ComHolderPreemp` traits.
src/coreclr/vm/weakreferencenative.cpp	Migrates `ComHolderPreemp<>` usages to `ReleaseHolder<>`.
src/coreclr/vm/stubmgr.cpp	Migrates `ComHolderAnyMode<>` usages to `ReleaseHolderAnyMode<>`.
src/coreclr/vm/stubhelpers.cpp	Migrates `ComHolderAnyMode<>` usages to `ReleaseHolderAnyMode<>`.
src/coreclr/vm/stdinterfaces.cpp	Migrates COM holders to `ReleaseHolder<>` / `ReleaseHolderAnyMode<>`.
src/coreclr/vm/runtimecallablewrapper.cpp	Migrates COM holders and adjusts a `CCWHolder` lifetime flow to use `Detach()`.
src/coreclr/vm/rejit.cpp	Removes legacy `= NULL` initialization pattern for `ReleaseHolder`.
src/coreclr/vm/peimagelayout.cpp	Replaces `Extract()` with `Detach()` when returning owned layouts.
src/coreclr/vm/peimage.cpp	Migrates metadata COM holder to `ReleaseHolderAnyMode<>`.
src/coreclr/vm/peassembly.cpp	Migrates `ComHolderPreemp<>` to `ReleaseHolder<>`.
src/coreclr/vm/olevariant.cpp	Migrates COM holders to `ReleaseHolder<>` / `ReleaseHolderAnyMode<>`.
src/coreclr/vm/olecontexthelpers.cpp	Migrates `ComHolderPreemp<>` to `ReleaseHolder<>`.
src/coreclr/vm/nativeimage.cpp	Replaces legacy holder APIs (`IsNull`/`Extract`) with `== NULL` and `Detach()`.
src/coreclr/vm/marshalnative.cpp	Updates `CCWHolder` initialization to brace-init and adjusts holder usage.
src/coreclr/vm/interoputil.cpp	Call sites migrated to new holder type names.
src/coreclr/vm/interopconverter.cpp	Migrates COM holders and replaces legacy holder APIs in a few places.
src/coreclr/vm/encee.cpp	Renames holder types in EnC path comments and variables.
src/coreclr/vm/eetoprofinterfaceimpl.cpp	Replaces `Extract()+NULL` patterns with `Detach()` for ownership transfer.
src/coreclr/vm/dllimport.cpp	Updates holder assignment to use `operator=` instead of `Assign`.
src/coreclr/vm/dispparammarshaler.cpp	Migrates COM holders to `ReleaseHolderAnyMode<>`.
src/coreclr/vm/crossloaderallocatorhash.inl	Updates holder initialization style and uses `Detach()` for returns.
src/coreclr/vm/coreassemblyspec.cpp	Uses `Detach()` when returning owned binder assembly.
src/coreclr/vm/commodule.cpp	Migrates COM holders to `ReleaseHolder<>` / `ReleaseHolderAnyMode<>`.
src/coreclr/vm/comconnectionpoints.cpp	Migrates `ComHolderAnyMode<>` to `ReleaseHolderAnyMode<>`.
src/coreclr/vm/comcallablewrapper.cpp	Migrates holders and refactors template-return ownership to use `Detach()`.
src/coreclr/vm/comcache.cpp	Migrates COM holders and replaces `Extract()` with `Detach()`.
src/coreclr/vm/clrex.cpp	Migrates error-info holder to `ReleaseHolderAnyMode<>`.
src/coreclr/vm/ceeload.cpp	Migrates symbol reader holders to `ReleaseHolder<>` and modernizes initialization.
src/coreclr/vm/assemblynative.cpp	Passes holder directly to tracing (via implicit pointer conversion).
src/coreclr/vm/assembly.cpp	Uses `Detach()` for ownership transfer and brace-init for holders.
src/coreclr/vm/appdomain.hpp	Adjusts `FindCachedFile` signature (removes unused `fThrow`).
src/coreclr/vm/appdomain.cpp	Refactors `BindAssemblySpec` lifetime management; simplifies `FindCachedFile`.
src/coreclr/md/enc/mdinternalrw.cpp	Fixes `QueryInterface` call to pass `void**`.
src/coreclr/inc/holder.h	Removes legacy `ReleaseHolder` wrapper, adds `LifetimeHolder`-based `ReleaseHolder` + `HasReleaseMethod`.
src/coreclr/dlls/mscoree/exports.cpp	Uses `Detach()` to transfer host handle ownership.
src/coreclr/debug/ee/debugger.cpp	Migrates `ComHolderPreemp<>` to `ReleaseHolder<>`.
src/coreclr/debug/daccess/enummem.cpp	Replaces legacy holder APIs (`Clear`/`GetValue`) with `Free()` and direct holder conversions.
src/coreclr/debug/daccess/dacdbiimpl.cpp	Removes `= nullptr` initialization for `ReleaseHolder`.
src/coreclr/debug/daccess/daccess.cpp	Removes `= nullptr` initialization for `ReleaseHolder`.
src/coreclr/debug/daccess/cdac.h	Replaces `Extract()` with `Detach()` in move operations.
src/coreclr/debug/createdump/threadinfo.cpp	Replaces `Extract()` with `Detach()` when transferring method ownership into a frame.
src/coreclr/debug/createdump/createdumpunix.cpp	Updates holder initialization to brace-init.
src/coreclr/debug/createdump/crashinfo.cpp	Removes `= nullptr` initialization for `ReleaseHolder`.
src/coreclr/binder/defaultassemblybinder.cpp	Replaces `Extract()` with `Detach()` for binder assembly ownership transfer.
src/coreclr/binder/customassemblybinder.cpp	Replaces `Extract()` with `Detach()` for binder assembly ownership transfer.
src/coreclr/binder/assemblybindercommon.cpp	Replaces `Extract()` with `Detach()` for binder assembly ownership transfer.
src/coreclr/binder/assembly.cpp	Replaces `Extract()` with `Detach()` for assembly-name ownership transfer.

Copilot's findings

Files reviewed: 47/47 changed files
Comments generated: 4

Copilot

Copilot's findings

Files reviewed: 50/50 changed files
Comments generated: 2

Copilot

Copilot's findings

Files reviewed: 62/62 changed files
Comments generated: 2

Copilot

Copilot's findings

Files reviewed: 62/62 changed files
Comments generated: 2

Copilot

Copilot's findings

Files reviewed: 62/62 changed files
Comments generated: 1

Copilot

Copilot's findings

Files reviewed: 62/62 changed files
Comments generated: 1

Copilot

Copilot's findings

Comments suppressed due to low confidence (1)

src/coreclr/vm/crst.cpp:42

CrstBase::InitWorker is annotated NOTHROW, but it calls minipal_mutex_init, which uses InitializeCriticalSection on Windows and can raise STATUS_NO_MEMORY. Today this is only guarded by _ASSERTE(suc), which is compiled out in retail and would leave m_lock uninitialized if init fails, leading to undefined behavior.

Please make initialization robust by catching any raised exception and/or handling a false return, and fail-fast (or otherwise handle) on failure instead of relying on _ASSERTE. This also preserves the intent of removing InitNoThrow call sites without introducing new crash paths.

Files reviewed: 80/80 changed files
Comments generated: 0 new

AaronRobinsonMSFT · 2026-07-22T02:58:23Z

If possible, I would leave addressing this and other issues for a follow up PR. I suspect that it will need a bunch more iterations and growing set of changes.

@jkotas The changes I've made here have uncovered a rat's nest of issues and contract violations. I keep finding them and that will likely continue. I'm going to switch this to Draft for now and drive them to ground. Once that is done I'll update this PR with the narrow changes and a subsequent PR with all the correctness fixes.

- Replaced instances of ComHolderAnyMode with ReleaseHolder and ReleaseHolderAnyMode across multiple files to enhance memory management and ensure proper release of COM interfaces. - Updated the ListLockEntryHolder to use LifetimeHolder with custom traits for better encapsulation of resource management. - Removed unnecessary comments and clarified code where applicable, particularly in EEContract files. - Improved consistency in handling COM interface pointers, ensuring that ownership semantics are clear and that resources are released appropriately. - Adjusted various methods in interop and runtime files to utilize the new holder classes, ensuring that the code adheres to modern C++ practices for resource management.

Copilot

Copilot's findings

Comments suppressed due to low confidence (1)

src/coreclr/vm/wrappers.h:59

ReleaseHolderAnyModeTraits calls SafeRelease, which is specifically for IUnknown* and performs a GC-mode transition. The updated comment says this holder works for any type with a Release() method, and the prior IUnknown constraint (static_assert(std::is_base_of<IUnknown, TYPE>::value)) was removed. This makes it easier to accidentally use ReleaseHolderAnyMode for non-COM types and get either confusing compile errors or incorrect assumptions about GC-mode behavior.

Consider restoring the IUnknown constraint and updating the comment back to explicitly describe a COM interface holder.

Files reviewed: 62/63 changed files
Comments generated: 1

Copilot

Copilot's findings

Comments suppressed due to low confidence (1)

src/coreclr/vm/wrappers.h:60

ReleaseHolderAnyModeTraits dropped the compile-time constraint that TYPE must be a COM interface (IUnknown-derived), but Free() still calls SafeRelease(IUnknown*), so non-IUnknown types will fail to compile with a less obvious error. Keeping the static_assert makes misuse failures clearer and aligns with SafeRelease’s actual signature.

Files reviewed: 62/63 changed files
Comments generated: 1

Copilot

Copilot's findings

Comments suppressed due to low confidence (3)

src/coreclr/inc/holder.h:1003

ReleaseHolderTraits::Free() only uses STATIC_CONTRACT_* macros, which are no-ops even when contracts are enabled. Since ReleaseHolder is now used throughout the VM to represent the preemptive-mode release holder, it would be better to preserve runtime contract checking (at least MODE_PREEMPTIVE) when ENABLE_CONTRACTS_IMPL is defined, while keeping the static annotations for non-VM consumers.

    static void Free(Type value)
    {
        STATIC_CONTRACT_NOTHROW;
        STATIC_CONTRACT_GC_TRIGGERS;
        STATIC_CONTRACT_MODE_PREEMPTIVE;

src/coreclr/vm/wrappers.h:59

ReleaseHolderAnyModeTraits::Free() calls SafeRelease(IUnknown*), so the trait still effectively requires TYPE to derive from IUnknown. Re-adding the static_assert improves diagnostics and keeps the template constraint explicit (the current comment also suggests non-COM types would work, but they won’t with SafeRelease).
src/coreclr/vm/wrappers.h:75
The comment for ReleaseHolderAnyMode says it works for any type with a Release() method, but the implementation calls SafeRelease (which is typed to IUnknown*). Updating the comment will avoid implying support for non-COM types.

Files reviewed: 62/63 changed files
Comments generated: 0 new

azure-pipelines · 2026-07-25T04:13:32Z

Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run.

Co-authored-by: Aaron R Robinson <arobins@microsoft.com>

Copilot

Copilot's findings

Comments suppressed due to low confidence (2)

src/coreclr/inc/holder.h:1007

ReleaseHolderTraits::Free() currently uses only STATIC_CONTRACT_* annotations, which are no-ops (see staticcontract.h). That means contract-enabled VM builds won’t enforce the intended MODE_PREEMPTIVE requirement (unlike the removed ComHolderPreempTraits which used CONTRACTL { MODE_PREEMPTIVE; }). If the intent is to catch accidental cooperative-mode COM Release() calls in the VM, this should use a real CONTRACTL under ENABLE_CONTRACTS_IMPL and fall back to STATIC_CONTRACT_* otherwise.

template <typename TYPE>
struct ReleaseHolderTraits final
{
    using Type = TYPE*;
    static constexpr Type Default() { return NULL; }
    static void Free(Type value)
    {
        STATIC_CONTRACT_NOTHROW;
        STATIC_CONTRACT_GC_TRIGGERS;
        STATIC_CONTRACT_MODE_PREEMPTIVE;

        if (value != NULL)
            value->Release();
    }

src/coreclr/vm/wrappers.h:75

The comment says this holder works for any held type with a Release() method, but the implementation calls SafeRelease(value), which requires the held pointer to be convertible to IUnknown*. This is effectively a COM interface holder; the comment should reflect that to avoid accidental misuse.

Files reviewed: 62/63 changed files
Comments generated: 0 new

jkotas · 2026-07-26T06:33:08Z


    HRESULT hr = S_OK;

-    CONTRACT_VIOLATION(GCViolation);    // ComHolderAnyMode goes to preemptive mode, which will trigger a GC


This method is GC_NOTRIGGER, but ReleaseHolder is GC_TRIGGERS - not enforced yet in this PR. I assume that you will turn on the enforcement in a follow up.

Should this method use a custom ReleaseHolders that is GC_NOTRIGGER?

jkotas · 2026-07-26T06:42:39Z

 include(${CMAKE_CURRENT_LIST_DIR}/clrfeatures.cmake)

 add_compile_definitions($<$<BOOL:$<TARGET_PROPERTY:DAC_COMPONENT>>:DACCESS_COMPILE>)
+add_compile_definitions($<$<BOOL:$<TARGET_PROPERTY:DBI_COMPONENT>>:DBI_COMPILE>)


Can

runtime/src/coreclr/debug/di/CMakeLists.txt

Line 2 in ca073c0

-DDBI_COMPILE

be deleted?

jkotas · 2026-07-26T06:49:00Z

-    ComHolderAnyMode<ITypeLib> pITLB;
-    ComHolderAnyMode<ITypeInfo> pTI;
-    ComHolderAnyMode<ITypeInfo> pTIDef;  // Default typeinfo of a coclass.
+    ReleaseHolderAnyMode<ITypeLib> pITLB;


Nit: This can be regular ReleaseHolder if we move GCX_PREEMP before local vars

Copilot AI review requested due to automatic review settings July 8, 2026 19:25

github-actions Bot added the area-VM-coreclr label Jul 8, 2026

AaronRobinsonMSFT added this to the 11.0.0 milestone Jul 8, 2026

Copilot started reviewing on behalf of AaronRobinsonMSFT July 8, 2026 19:25 View session

dotnet-policy-service Bot assigned AaronRobinsonMSFT Jul 8, 2026

github-project-automation Bot added this to AppModel Jul 8, 2026

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread src/coreclr/inc/holder.h Outdated

Comment thread src/coreclr/vm/wrappers.h Outdated

Comment thread src/coreclr/inc/holder.h Outdated

Comment thread src/coreclr/vm/appdomain.cpp Outdated

Copilot AI review requested due to automatic review settings July 8, 2026 20:37

Copilot started reviewing on behalf of AaronRobinsonMSFT July 8, 2026 20:38 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread src/coreclr/inc/holder.h Outdated

Comment thread src/coreclr/vm/wrappers.h Outdated

build-analysis Bot mentioned this pull request Jul 9, 2026

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

Copilot AI review requested due to automatic review settings July 10, 2026 17:46

Copilot started reviewing on behalf of AaronRobinsonMSFT July 10, 2026 17:46 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Comment thread src/coreclr/vm/wrappers.h Outdated

Comment thread src/coreclr/vm/ceeload.cpp Outdated

Copilot AI review requested due to automatic review settings July 10, 2026 21:38

Copilot started reviewing on behalf of AaronRobinsonMSFT July 10, 2026 21:39 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Comment thread src/coreclr/vm/wrappers.h Outdated

Comment thread src/coreclr/inc/holder.h Outdated

elinor-fung mentioned this pull request Jul 11, 2026

[browser] Use the new version of V8 with exnref #129849

Closed

This was referenced Jul 11, 2026

[android] Android.Device_Emulator.JIT.Test failing on emulators with CoreCLR #112633

Open

Flaky timeout in CreateFileChangeToken_RootDeletedAndRecreated_TokenFiresWhenFileCreated #129691

Closed

Copilot AI review requested due to automatic review settings July 12, 2026 01:00

AaronRobinsonMSFT marked this pull request as ready for review July 12, 2026 01:00

Copilot started reviewing on behalf of AaronRobinsonMSFT July 12, 2026 01:00 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

Comment thread src/coreclr/vm/wrappers.h Outdated

Copilot AI review requested due to automatic review settings July 13, 2026 14:28

Copilot started reviewing on behalf of AaronRobinsonMSFT July 13, 2026 14:29 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

Comment thread src/coreclr/inc/holder.h Outdated

Copilot AI reviewed Jul 21, 2026

View reviewed changes

AaronRobinsonMSFT marked this pull request as draft July 22, 2026 02:58

Copilot AI review requested due to automatic review settings July 24, 2026 22:28

AaronRobinsonMSFT force-pushed the reconcile-releaseholder branch from 676775a to f4722e7 Compare July 24, 2026 22:28

Copilot started reviewing on behalf of AaronRobinsonMSFT July 24, 2026 22:29 View session

AaronRobinsonMSFT closed this Jul 24, 2026

AaronRobinsonMSFT reopened this Jul 24, 2026

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread src/coreclr/debug/daccess/enummem.cpp

Copilot AI review requested due to automatic review settings July 24, 2026 22:37

AaronRobinsonMSFT force-pushed the reconcile-releaseholder branch from f4722e7 to 9d9b795 Compare July 24, 2026 22:37

Copilot started reviewing on behalf of AaronRobinsonMSFT July 24, 2026 22:38 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread src/coreclr/inc/holder.h

Fix template syntax for LifetimeHolder in CrossLoaderAllocatorHash

b4647c6

Copilot AI review requested due to automatic review settings July 24, 2026 22:56

Copilot started reviewing on behalf of AaronRobinsonMSFT July 24, 2026 22:56 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

AaronRobinsonMSFT marked this pull request as ready for review July 25, 2026 04:12

AaronRobinsonMSFT requested a review from jkotas July 25, 2026 04:12

AaronRobinsonMSFT commented Jul 25, 2026

View reviewed changes

Comment thread src/coreclr/inc/contract.h Outdated

Apply suggestions from code review

ff7c36f

Co-authored-by: Aaron R Robinson <arobins@microsoft.com>

AaronRobinsonMSFT commented Jul 25, 2026

View reviewed changes

Comment thread src/coreclr/inc/contract.h Outdated

Apply suggestion from @AaronRobinsonMSFT

a6e8d64

Copilot AI reviewed Jul 25, 2026

View reviewed changes

build-analysis Bot mentioned this pull request Jul 25, 2026

Unable to pull image from mcr.microsoft.com #117164

Open

jkotas reviewed Jul 26, 2026

View reviewed changes


		HRESULT hr = S_OK;

		CONTRACT_VIOLATION(GCViolation); // ComHolderAnyMode goes to preemptive mode, which will trigger a GC

Uh oh!

Conversation

AaronRobinsonMSFT commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

dotnet-policy-service Bot commented Jul 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

AaronRobinsonMSFT commented Jul 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

azure-pipelines Bot commented Jul 25, 2026

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

jkotas Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 26, 2026

AaronRobinsonMSFT commented Jul 8, 2026 •

edited

Loading