Reconcile ReleaseHolder, ComHolderPreemp, and ComHolderAnyMode

[AaronRobinsonMSFT]

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]

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/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]

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.

[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

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (2)

src/coreclr/inc/holder.h:1002

• ReleaseHolderTraits::Free declares STATIC_CONTRACT_GC_NOTRIGGER, but Release() on COM interfaces (and other ref-counted objects) can trigger GC/message pumping (e.g., SafeReleasePreemp is annotated GC_TRIGGERS). The current contract is too strict and can cause contract violations in checked builds.

        STATIC_CONTRACT_GC_NOTRIGGER;

src/coreclr/vm/wrappers.h:75

• This comment says the holder releases a "type with a Release() method", but ReleaseHolderAnyModeTraits::Free calls SafeRelease(value), whose signature takes IUnknown*. This holder is COM/IUnknown-specific, so the comment should reflect that to avoid accidental misuse.

• Files reviewed: 63/64 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (1)

src/coreclr/inc/holder.h:1004

• ReleaseHolderTraits::Free is marked GC_NOTRIGGER and lacks the same contract-violation allowances as SafeReleasePreemp, but it calls IUnknown::Release directly (which can message-pump and run arbitrary managed code). This can lead to contract assertions / incorrect assumptions in callers that replaced ComHolderPreemp with ReleaseHolder.

        STATIC_CONTRACT_NOTHROW;
        STATIC_CONTRACT_GC_NOTRIGGER;
        STATIC_CONTRACT_MODE_PREEMPTIVE;

• Files reviewed: 63/64 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (2)

src/coreclr/inc/holder.h:1007

• ReleaseHolderTraits::Free is used as the preemptive-mode COM release holder replacement, but it currently only has static-contract annotations (which are no-ops) and it annotates GC_NOTRIGGER, even though COM Release() can message-pump / run managed code (see SafeReleasePreemp which is GC_TRIGGERS). This loses the debug-mode contract enforcement that ComHolderPreemp had and documents the call as non-GC-triggering when it isn't.

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

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

src/coreclr/vm/wrappers.h:75

• The comment for ReleaseHolderAnyMode says it works for any "type with a Release() method", but Free calls SafeRelease, which takes an IUnknown*. This holder is effectively for COM interfaces (IUnknown-derived); the broader wording is misleading and makes it easier to misuse the trait template.

• Files reviewed: 63/64 changed files
• Comments generated: 0 new

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (3)

src/coreclr/inc/holder.h:1007

• ReleaseHolderTraits::Free() calls value->Release() directly under a NOTHROW static contract, but COM-style Release can pump messages and run arbitrary managed code (see SafeReleasePreemp), which is why SafeReleasePreemp uses CONTRACT_VIOLATION(ThrowsViolation | FaultViolation). As written, this can trigger contract asserts in debug builds and diverges from the intended preemptive-mode release semantics described in the PR.

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:60

• ReleaseHolderAnyModeTraits::Free calls SafeRelease(IUnknown*), so TYPE must be IUnknown-derived. The removal of the static_assert both weakens diagnostics and makes the trait/comment suggest it works for any type with Release().
  src/coreclr/vm/wrappers.h:75
• The comment for ReleaseHolderAnyMode says it works for any held type with a Release() method, but the implementation routes through SafeRelease(IUnknown*). This should describe COM interface pointers (IUnknown-derived) to avoid misleading future callers.

• Files reviewed: 65/66 changed files
• Comments generated: 0 new