Fix finalizer fail-fast when unadvising reused COM event sinks

[jkoritzinsky]

Why

The Interop/COM/Dynamic test intermittently fail-fasts the finalizer thread with 0x80131623 on checked/debug CI legs. A dump from build 1573972 showed:

Environment.FailFast <- Debug.Fail <- ComEventsSink.Unadvise
  <- ComEventsSink.RemoveAll
  <- ComEventSinksContainer.DisposeAll
  <- ComEventSinksContainer.Finalize
  <- GC.RunFinalizers

Removing the last handler for a COM event source unadvises the sink but deliberately keeps it in the RCW's sink container so it can be reused: FromRuntimeCallableWrapper looks for a sink whose _iidSourceItf is Guid.Empty and re-Initializes it. When the RCW is later collected, the container finalizer unadvises every sink it holds, including the already-unadvised ones, and trips Debug.Assert(_connectionPoint != null). Because that runs on the finalizer thread it takes the whole process down rather than failing a test.

What changed

Product: drop the assert in ComEventsSink.Unadvise. The existing if (_connectionPoint == null) return; already handles this correctly; only the assert was wrong.

I first tried the narrower Debug.Assert(_connectionPoint != null || _iidSourceItf == Guid.Empty, ...) and confirmed it holds on the normal path, but it is not a real invariant:

• Initialize assigns _iidSourceItf before calling Advise, so a failure inside Advise on a reused sink (FindConnectionPoint failing, or cp.Advise returning CONNECT_E_ADVISELIMIT) leaves a non-empty iid with a null connection point, still sitting in the container.
• RemoveHandler runs outside lock (comEventSinks), so the window between Unadvise() and _iidSourceItf = Guid.Empty is observable by a concurrent -=.

Removing the assert also matches what Unadvise already does: it swallows all exceptions because "the host may not be available at this point".

Test: EventTest now runs in a non-inlined helper and forces a collection afterwards, so the RCW and its sink container are reclaimed at a deterministic point, with a WeakReference proving the RCW is actually gone. Previously hitting this depended on GC timing.

This also reverts #133037, which had disabled the test with [ActiveIssue] for this same issue.

Notes for reviewers

src/libraries/Common/src/System/Runtime/InteropServices/ComEventsSink.cs is shared: it compiles into both Microsoft.CSharp and CoreCLR's System.Private.CoreLib. Only the Microsoft.CSharp path reuses sinks. The ComEventsHelper path removes a sink from the linked list before unadvising and never clears _iidSourceItf, so it never double-unadvises and is unaffected. NativeAOT's ComEventsHelper throws PlatformNotSupportedException.

The assert is [Conditional("DEBUG")], so there is no release behavior change. The repro only manifests with Debug-configuration libraries, which is what PR legs use (debugOnPrReleaseOnRolling); rolling builds use Release libraries and compile the assert out.

Validation

Windows x64, checked CoreCLR + Debug libraries:

• Before the fix: Dynamic.cmd failed 3/3 with 0x80131623 and the stack above. The unmodified test passed 3/3, confirming the test change is what makes it deterministic.
• After the fix: Dynamic.cmd passes 3/3, and Assert.False(rcwReference.IsAlive) is now reached and passes.
• Full Interop/COM tree: 24 of 30 ran and passed. The 6 NativeClients/* tests exit 9009 ("program not found") locally because they need the out-of-process harness, so CI is the first real check of NativeClients/Events, which is the only coverage of the ComEventsHelper path in this shared file.

Unrelated defect noticed

FromRuntimeCallableWrapper in ComEventsSink.Extended.cs has no break after reusing a sink, so with two emptied sinks it advises both to the same iid and returns the last one. A later -= then finds the first one via the _iidSourceItf == sourceIid fast path, whose _methods is null, and returns early without removing the handler. Not touched here.

Fixes #132947

Note

This pull request description was generated by GitHub Copilot.

[azure-pipelines]

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

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

[Copilot]

Copilot review overview

🟢 Approval recommended

The change removes a debug-only assertion that was not a valid invariant and updates the test to deterministically validate the intended finalizer behavior without altering release semantics.

Review tier: Lite
Findings: None

What changed in this PR

This PR addresses an intermittent fail-fast on the finalizer thread in the Interop/COM/Dynamic test scenario by making ComEventsSink.Unadvise() tolerant of already-unadvised (reused) sinks, and it updates the Dynamic COM event test to deterministically collect/finalize the RCW so the regression is reliably exercised (and the prior skip can be removed).

Changes:

• Remove the debug-only assertion in ComEventsSink.Unadvise() and rely on the existing _connectionPoint == null early-return.
• Refactor EventTest to run via a non-inlined helper that returns a WeakReference, then force GC/finalization and assert the RCW is reclaimed.
• Re-enable the Dynamic test by removing the [ActiveIssue] skip and switching to the new static EventTest.Run() entrypoint.

File	Description
src/​tests/​Interop/​COM/​Dynamic/​Program.cs	Removes the CoreCLR [ActiveIssue] skip and invokes the updated static event test entrypoint.
src/​tests/​Interop/​COM/​Dynamic/​EventTest.cs	Ensures deterministic RCW reclamation/finalizer execution via NoInlining helper + forced GC and WeakReference assertion.
src/​libraries/​Common/​src/​System/​Runtime/​InteropServices/​ComEventsSink.cs	Drops the debug assert that could fail-fast on the finalizer thread when a sink is already unadvised, preserving idempotent behavior.

[Copilot]

Copilot review overview

🟢 Approval recommended

The change removes a debug-only assert that contradicts existing null-handling in Unadvise and adds a targeted, deterministic test to cover the finalizer scenario.

Review tier: Lite
Findings: None