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.
Both holders' static_assert is relaxed from is_base_of<IUnknown, T> to a duck-typed "T has a Release() member" check (holder_detail::HasReleaseMethod), so ReleaseHolder can hold non-IUnknown types that expose Release().

Converge the three COM-interface RAII holders onto the LifetimeHolder<Traits> pattern: - Retarget ReleaseHolder to LifetimeHolder<ReleaseHolderTraits<T>>, adopting ComHolderPreemp's preemptive-mode release semantics (the VM default). The Free() contract is MODE_PREEMPTIVE, gated on ENABLE_CONTRACTS_IMPL so non-VM consumers (utilcode, md, DAC, createdump) use a static-contract annotation. - Remove ComHolderPreemp entirely; ReleaseHolder replaces all uses. - Rename ComHolderAnyMode to ReleaseHolderAnyMode. - Relax both holders' static_assert from is_base_of<IUnknown, T> to a duck-typed "T has a Release() member" check (holder_detail::HasReleaseMethod), so ReleaseHolder can hold non-IUnknown types that expose Release(). Migrate all call sites off the legacy SpecializedWrapper API onto the LifetimeHolder API (Extract->Detach, Clear->Free, GetValue/IsNull/Assign/SuppressRelease removed). Read-after-suppress sites are restructured to detach into a raw pointer to preserve exception-safety and refcount behavior. BindAssemblySpec is refactored so the CoreLib singleton borrow no longer relies on SuppressRelease. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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

holder.h now includes contract.h (for the MODE_PREEMPTIVE CONTRACTL in ReleaseHolderTraits::Free), which pulled check.inl -> clrhost.h -> holder.h back into itself before Holder/LifetimeHolder were defined. Remove the clrhost.h include from check.inl to break the cycle, and make the two headers that were relying on that transitive path self-sufficient: - inc/sigbuilder.h: include corhdr.h for CorElementType/mdToken. - jit/jitstd/algorithm.h: include <utility> for std::swap used by jitstd::sort. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

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

- Added DBI_COMPILE definition for conditional compilation in clrdefinitions.cmake. - Updated Debugger::GetAndSendTransitionStubInfo to use g_pEEInterface for IP validation. - Refined contract enabling conditions in contract.h to include DBI builds. - Modified ReleaseHolderTraits to use ENABLE_EE_CONTRACTS for contract checks. - Removed unused IsIPInModule function from utilcode.h and implemented it in ExecutionManager. - Updated various files to consistently use ExecutionManager::IsIPInModule for IP validation.

Copilot

Copilot's findings

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

ReleaseHolder's Detach() clears the holder's value (unlike the old Extract(), which retained it via SuppressRelease). BindAssemblyByProbingPaths detached pAssembly into *ppAssembly and then dereferenced pAssembly for the ref-def check, hitting the operator-> null assert (holder.h:974) on the TPA bind path at startup, crashing every coreclr test process. Use a local for the detached pointer and read from it. Also sequence the image reads before Detach() in nativeimage.cpp OpenR2RFromPE, where the member accesses and Detach() were unsequenced call arguments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6457cb5c-e732-41ad-8cc0-af22e64c1f49

Copilot

Copilot's findings

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

…s across the codebase

…agement and prevent memory leaks

Copilot

Copilot's findings

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

…e global scope

Copilot

Copilot's findings

Comments suppressed due to low confidence (1)

src/coreclr/vm/wrappers.h:61

ReleaseHolderAnyModeTraits::Free calls SafeRelease(value), but SafeRelease is declared as ULONG SafeRelease(IUnknown* ...). The current static_assert only checks for a Release() member, so it will accept types that are not convertible to IUnknown*, and then fail later with a less clear conversion error (and the comment suggests non-IUnknown support). Either constrain the trait to IUnknown-convertible types, or change Free to not depend on SafeRelease(IUnknown*).

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

Copilot

Copilot's findings

Comments suppressed due to low confidence (1)

src/coreclr/vm/wrappers.h:60

ReleaseHolderAnyModeTraits::Free calls SafeRelease(value), which requires value to be convertible to IUnknown*. The new static_assert only checks for a Release() member, so non-IUnknown types will pass the assert but then fail later with a less clear conversion error. Consider asserting convertibility to IUnknown* (or reverting this holder to the IUnknown constraint) so the trait requirements match the implementation.

Files reviewed: 75/75 changed files
Comments generated: 5

 #ifdef _DEBUG
    DebugInit(crstType, flags);
 #endif
+    return suc;


 	_ep_rt_coreclr_config_lock_handle.lock = &_ep_rt_coreclr_config_lock;
-	_ep_rt_coreclr_config_lock_handle.lock->InitNoThrow (CrstEventPipe, (CrstFlags)(CRST_REENTRANCY | CRST_TAKEN_DURING_SHUTDOWN));
+	_ep_rt_coreclr_config_lock_handle.lock->Init (CrstEventPipe, (CrstFlags)(CRST_REENTRANCY | CRST_TAKEN_DURING_SHUTDOWN));



    spin_lock->lock = new (nothrow) CrstStatic ();
-    spin_lock->lock->InitNoThrow (CrstType::CrstEventPipe);
+    spin_lock->lock->Init (CrstType::CrstEventPipe);
 }


+            s_lock.Init(CrstGcStressControl);

            if (g_pRhConfig->GetGcStressSeed())
                s_lGcStressRNGSeed = (uint32_t)g_pRhConfig->GetGcStressSeed();


 void InitializeGCEventLock()
 {
-    g_eventStashLock.InitNoThrow(CrstGcEvent);
+    g_eventStashLock.Init(CrstGcEvent);
 }


jkotas · 2026-07-19T05:59:26Z

    // EventPipe library will intialize using thread, EventPipeBufferManager instances and will maintain these on the EventPipe library side

    spin_lock->lock = new (nothrow) CrstStatic ();
-    spin_lock->lock->InitNoThrow (CrstType::CrstEventPipe);


Did you mean to push these changes that are outside the original scope of this PR that is fairly sizable already, or is it just your local AI agents misbehaving?

Yes, I changed these... sigh. The issue started out as a misaligned contract. When I determined the issue, it became clear the Init and InitNoThrow were identical - both didn't throw. Realizing this I decided to remove InitNoThrow and make Init return a bool. It is purely cosmetic at this point as the contract should be both no throw and no trigger. I can probably reduce it, but if you look at the last commit by itself you can see the changes are rather mechanical.

I agree that the InitNoThrow vs. Init implementation is questionable. I am not sure whether unifying it to a single implementation that returns bool is an improvement since it creates a number of places that return bool without checking it.

The initialization of critical sections never fails in practice. I think it would be ok to assert it (or even fail fast) centrally level and avoid propagating the problem to callers.

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.

since it creates a number of places that return bool without checking it.

I don't disagree, but it never threw anyways so it is no better/worse than what it was.

The initialization of critical sections never fails in practice. I think it would be ok to assert it (or even fail fast) centrally level and avoid propagating the problem to callers.

That works too. This does mean I can remove some failure cases

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.

I'm trying to. The contracts system and use across the repo has degraded very badly so minor tweaks and fixes uncover a slew of issues. I'm going to give this one more shot and see the impact. The vast majority here are updating contracts though and no functional change. I will try and limit it to that for the final attempt.

github-actions · 2026-07-19T13:46:37Z

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "05b5a4a3f05aa075d893d3223e6498a697f47dbb",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "6bd973a541700487cb4a1891943c2a7a55e4c956",
  "last_reviewed_commit": "05b5a4a3f05aa075d893d3223e6498a697f47dbb",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "6bd973a541700487cb4a1891943c2a7a55e4c956",
  "last_recorded_worker_run_id": "29695952166",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "c8a6b62628ce0721f2ae5c83939a7fcff42633d9",
      "review_id": 4730876129
    },
    {
      "commit": "05b5a4a3f05aa075d893d3223e6498a697f47dbb",
      "review_id": 4731158829
    }
  ]
}

github-actions

Holistic Review

Motivation: Justified refactor by the interop area owner. The three COM-interface RAII holders (ReleaseHolder, ComHolderPreemp, ComHolderAnyMode) had overlapping, subtly different semantics and a dangerous legacy ReleaseHolder (documented "UNSAFE TO USE IN THE VM"). Converging them onto the LifetimeHolder<Traits> pattern removes a real footgun and reduces duplication.

Approach: Sound and largely mechanical. ReleaseHolder becomes LifetimeHolder<ReleaseHolderTraits<T>> with preemptive-release semantics, ComHolderPreemp is removed, ComHolderAnyMode is renamed to ReleaseHolderAnyMode, and the static_assert is relaxed to a duck-typed HasReleaseMethod check. Call sites migrate from the SpecializedWrapper API (Extract/Clear/GetValue/IsNull/SuppressRelease) to the LifetimeHolder API. The read-after-Detach restructurings (assembly binder, nativeimage.cpp OpenR2RFromPE, RCW aggregation) are correct and address the real semantic difference that Detach() clears the holder whereas the old Extract() retained it — a bug already caught and fixed within this PR (commit 9b81753). Moving IsIPInModule from utilcode into the VM behind g_pEEInterface and the contract-header include-cycle cleanup are reasonable supporting changes; the IsIPInModule body was moved verbatim.

Summary: ⚠️ Needs Human Review. The refactor is well-executed and the mechanical migration looks complete (no stray ComHolderPreemp/ComHolderAnyMode/Extract/GetValue references remain on the converted COM paths). My one substantive concern is a possible behavioral change on the preemptive release path: the removed ComHolderPreemp released through SafeReleasePreemp, which suppresses ThrowsViolation | FaultViolation to tolerate a COM message pump running managed code during Release(); the new ReleaseHolderTraits::Free calls value->Release() directly without that suppression. This is a debug/checked-build contract concern only, not a retail correctness issue, but it warrants an area-owner confirmation that it is intentional. See the inline comment on src/coreclr/inc/holder.h.

Detailed Findings

⚠️ Correctness (debug contracts) — Preemptive release no longer suppresses Throws/Fault violations

See the inline comment on src/coreclr/inc/holder.h (ReleaseHolderTraits::Free). The migrated ComHolderPreemp sites previously released via SafeReleasePreemp with CONTRACT_VIOLATION(ThrowsViolation | FaultViolation); the new trait does not. Advisory, not merge-blocking, but should be confirmed intentional by the interop owner. ReleaseHolderAnyMode still routes through SafeRelease and is unaffected.

✅ Extract→Detach conversions — Read-after-detach correctly restructured

Detach() clears the holder value (unlike the old Extract(), which retained it via an implicit suppress). The PR correctly detaches into a local before subsequent dereferences in assemblybindercommon.cpp (BindByTpaList ref-def check, pFoundAssembly) and sequences the image member reads before Detach() in nativeimage.cpp OpenR2RFromPE. A null-deref regression from this exact conversion was found and fixed earlier in the PR (commit 9b81753), which increases confidence the pattern was audited.

✅ Migration completeness — No stray legacy holder/API references on converted paths

Grep confirms no remaining ComHolderPreemp/ComHolderAnyMode usages, and the surviving .Extract()/.GetValue()/.SuppressRelease()/.IsNull() occurrences are on unrelated SpecializedWrapper/TypeHandle/StackFrame holders that this PR does not touch.

💡 Style — `NULL` vs `nullptr` in new/renamed traits

ReleaseHolderTraits::Default() and the renamed ReleaseHolderAnyModeTraits::Default() use return NULL; (the latter changed from nullptr). The native review guidance prefers nullptr; the internal value != NULL check in Free is likewise legacy style. Minor and consistent-enough with surrounding VM code, so non-blocking.

✅ IsIPInModule relocation — Verbatim move, correctly abstracted behind the EE interface

The function body moved from utilcode/util.cpp to vm/util.cpp unchanged, is declared in util.hpp, and the debugger now reaches it via g_pEEInterface->IsIPInModule (new pure-virtual on EEDebugInterface, implemented in EEDbgInterfaceImpl with WRAPPER_NO_CONTRACT and DAC_UNEXPECTED()), plus the DBI_COMPILE/DBI_COMPONENT cmake plumbing. Consistent with how other EE services are exposed to the DBI.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 163.7 AIC · ⌖ 11.7 AIC · ⊞ 10K

github-actions · 2026-07-19T14:03:12Z

+#endif // ENABLE_EE_CONTRACTS
+
+        if (value != NULL)
+            value->Release();


⚠️ Possible loss of contract-violation suppression on the preemptive release path.

Before this PR, the VM sites converted here (e.g. RCW::CreateRCWInternal, RCW::GetMarshalingType, ComWeakRefToObject, Debugger::getBoundariesHelper) used ComHolderPreemp, whose Free called SafeReleasePreemp. That helper deliberately adds CONTRACT_VIOLATION(ThrowsViolation | FaultViolation) with the comment "Message pump could happen, so arbitrary managed code could run" before invoking Release().

The new ReleaseHolderTraits::Free calls value->Release() directly under a bare NOTHROW; GC_TRIGGERS; MODE_PREEMPTIVE contract with no ThrowsViolation/FaultViolation suppression. For COM proxies whose Release() pumps messages and can run managed code that throws/faults, this is a stricter debug contract than the code these sites previously ran under, and could trip a contract assert in debug/checked builds that the old SafeReleasePreemp path tolerated.

Was dropping the ThrowsViolation | FaultViolation suppression intentional for the migrated ComHolderPreemp sites? If proxy Release() can pump, consider routing ReleaseHolderTraits::Free through SafeReleasePreemp (or adding the equivalent violation) to preserve prior behavior. Note ReleaseHolderAnyModeTraits::Free still goes through SafeRelease, so it is unaffected; this concern is specific to the ReleaseHolder (preemptive) path.

…ion handling

Copilot

Copilot's findings

Comments suppressed due to low confidence (3)

src/coreclr/vm/crst.cpp:49

InitWorker returns 'suc' but still marks the CRST as initialized before that (SetFlags/SetCrstInitialized), so a minipal_mutex_init failure in retail builds (where _ASSERTE is compiled out) would leave an uninitialized mutex that later gets treated as valid. Consider bailing out early on failure before setting flags / initialized state.
src/coreclr/vm/wrappers.h:60
ReleaseHolderAnyModeTraits::Free calls SafeRelease(value), which is declared as SafeRelease(IUnknown* ...). With the new duck-typed static_assert, types that merely have Release() but are not convertible to IUnknown* will fail with template/overload errors rather than the intended diagnostic. Since this holder is tied to SafeRelease’s COM semantics, the constraint should still express the IUnknown* convertibility requirement (or SafeRelease would need to be generalized).
src/coreclr/inc/holder.h:1022
ReleaseHolderTraits::Free hard-codes MODE_PREEMPTIVE for all types with a Release() method when ENABLE_EE_CONTRACTS is enabled. ReleaseHolder is also used for non-COM ref-counted objects in VM code that runs under STANDARD_VM_CONTRACT / MODE_ANY (e.g., ReleaseHolder in ReJitManager::ConfigureILCodeVersion via prestub.cpp). This will trip contract violations (and is semantically misleading) for non-COM releases. Consider splitting the concepts: keep a MODE_ANY ReleaseHolder for general ref-counted runtime types, and introduce a separate preemptive-only COM holder (or a traits parameter) for the former ComHolderPreemp use cases.

            NOTHROW;
            GC_TRIGGERS;
            MODE_PREEMPTIVE;
        } CONTRACTL_END;

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

github-actions

Holistic Review

Motivation: Unchanged. This PR converges the three COM-interface RAII holders (ReleaseHolder, ComHolderPreemp, ComHolderAnyMode) onto the LifetimeHolder<Traits> pattern, removing a documented "UNSAFE TO USE IN THE VM" footgun and reducing duplication. The supporting contract-annotation cleanups and the IsIPInModule relocation into the VM remain part of the change.

Approach: Unchanged and sound. Since the prior review at commit c8a6b62, the only new commit (05b5a4a, "Add GC_NOTRIGGER to contract definitions") adds GC_NOTRIGGER to three CONTRACTL blocks in src/coreclr/inc/metadata.h: EnumInit, EnumInitNoThrow, and EnumAllInit. These are thin HENUMInternalHolder-style wrappers that forward to IMDInternalImport::EnumInit/EnumAllInit; metadata enumeration is a non-GC-triggering operation, so tightening the contract to GC_NOTRIGGER is consistent with the sibling EnumAssociateInit path and with the PR's broader contract-refinement theme. This is a checked-build contract tightening only, with no retail behavioral effect.

Summary: ⚠️ Needs Human Review — verdict unchanged from the prior review. The new commit is a low-risk, correct contract annotation and introduces no new findings. My one carried-over substantive concern from the prior review (the preemptive-release path in ReleaseHolderTraits::Free no longer suppressing ThrowsViolation | FaultViolation as the removed ComHolderPreemp's SafeReleasePreemp did) is unaffected by this commit and still warrants interop area-owner confirmation.

Assessment History:

review 4730876129 reviewed commit c8a6b62 with verdict ⚠️ Needs Human Review. Current verdict: ⚠️ Needs Human Review — unchanged. The incremental commit 05b5a4a only adds GC_NOTRIGGER to three metadata enum-init contracts, which does not alter the motivation, approach, or risk profile of the PR.

Detailed Findings

No new actionable findings in the incremental scope. The GC_NOTRIGGER additions in src/coreclr/inc/metadata.h (EnumInit, EnumInitNoThrow, EnumAllInit) are correct and consistent with surrounding contract annotations. Prior-review findings on the cumulative PR (the ReleaseHolderTraits::Free preemptive-release contract-suppression concern and the minor NULL vs nullptr style note) remain applicable but are outside this incremental scope and are unchanged.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 59.6 AIC · ⌖ 16.2 AIC · ⊞ 10K

jkotas · 2026-07-20T01:53:54Z

Now failing with

Assert failure(PID 5692 [0x0000163c], Thread: 5268 [0x1494]): CONTRACT VIOLATION by CUnorderedArrayWithAllocator<struct EECodeGenManager::DomainCodeHeapList *,5,class CUnorderedArray__Allocator<struct EECodeGenManager::DomainCodeHeapList *> >::AppendThrowing at "D:\a\_work\1\s\src\coreclr\inc\utilcode.h":823

GC_TRIGGERS encountered in a GC_NOTRIGGER scope

                        CONTRACT in CUnorderedArrayWithAllocator<struct EECodeGenManager::DomainCodeHeapList *,5,class CUnorderedArray__Allocator<struct EECodeGenManager::DomainCodeHeapList *> >::AppendThrowing at "D:\a\_work\1\s\src\coreclr\inc\utilcode.h":823
VIOLATED-->  CONTRACT in EECodeGenManager::CreateCodeHeapList at "D:\a\_work\1\s\src\coreclr\vm\codeman.cpp":3419
                        CONTRACT in EECodeGenManager::AllocCodeWorker at "D:\a\_work\1\s\src\coreclr\vm\codeman.cpp":2986

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

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

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

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

Comment thread src/coreclr/inc/holder.h

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

Open

Merge branch 'main' into reconcile-releaseholder

38b718f

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

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

Helix: System.IO.IOException: No space left on device #128758

Open

AaronRobinsonMSFT added 2 commits July 17, 2026 10:26

Move IsIPInModule function to a VM util location and update reference…

9c52398

…s across the codebase

Refactor assembly binding logic to use ReleaseHolder for assembly man…

038aa2d

…agement and prevent memory leaks

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

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

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Comment thread src/coreclr/vm/eedbginterfaceimpl.cpp

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

Error while retrieving client Settings for PipelineArtifact. HttpRequestException: nodename nor servname provided, or not known (vsblobprodcus3.vsblob.visualstudio.com:443) #96798

Open

Refactor contract-related comments and update IsIPInModule call to us…

3661d00

…e global scope

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

Copilot started reviewing on behalf of AaronRobinsonMSFT July 17, 2026 22:11 View session

AaronRobinsonMSFT requested review from elinor-fung, jkotas and sbomer July 17, 2026 22:12

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Comment thread src/coreclr/inc/contract.h

jkotas approved these changes Jul 17, 2026

View reviewed changes

Refactor Crst initialization methods to unify error handling

c8a6b62

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

AaronRobinsonMSFT requested a review from MichalStrehovsky as a code owner July 19, 2026 05:26

AaronRobinsonMSFT requested a review from jkotas July 19, 2026 05:26

Copilot started reviewing on behalf of AaronRobinsonMSFT July 19, 2026 05:26 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

jkotas reviewed Jul 19, 2026

View reviewed changes

github-actions Bot reviewed Jul 19, 2026

View reviewed changes

Add GC_NOTRIGGER to contract definitions for improved garbage collect…

05b5a4a

…ion handling

Copilot AI review requested due to automatic review settings July 19, 2026 16:40

Copilot started reviewing on behalf of AaronRobinsonMSFT July 19, 2026 16:41 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

github-actions Bot reviewed Jul 19, 2026

View reviewed changes

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!

jkotas Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

AaronRobinsonMSFT Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

AaronRobinsonMSFT Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

AaronRobinsonMSFT Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Holistic Review

Detailed Findings

⚠️ Correctness (debug contracts) — Preemptive release no longer suppresses Throws/Fault violations

✅ Extract→Detach conversions — Read-after-detach correctly restructured

✅ Migration completeness — No stray legacy holder/API references on converted paths

💡 Style — NULL vs nullptr in new/renamed traits

✅ IsIPInModule relocation — Verbatim move, correctly abstracted behind the EE interface

Uh oh!

github-actions Bot Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

AaronRobinsonMSFT commented Jul 8, 2026 •

edited

Loading

github-actions Bot commented Jul 19, 2026 •

edited

Loading

💡 Style — `NULL` vs `nullptr` in new/renamed traits