Fix resource leaks in WinForms native-object cleanup

[JeremyKuhne]

Customer Impact

Some WinForms operations leave Windows objects alive after the application has finished using them. Pictures still display, browser views close, and events still work, but repeated use can accumulate resources.

Windows keeps these objects alive while another component needs them. Several paths were missing the cleanup that tells Windows they are finished. Creating a .NET wrapper does not perform that cleanup on the caller's behalf.

Proposed changes

• Adds missing cleanup in the shared conversion helpers, ActiveX font conversion, accessibility navigation, and browser hosting.
• Disconnects old HTML event connections when a page unloads, while preserving connections for other active frames.
• Fixes a separate, test-only cleanup issue that caused the VB6 hosting test to crash during shutdown, and re-enables that test.

The production changes use existing cleanup patterns and introduce no public API changes. Most of the diff is regression coverage.

Regression?

No .NET 11-specific regression; the same defective code is also present in .NET 10.

Risk

Low to moderate. Most changes add missing cleanup; the shared conversion helper and the timing of HTML unload cleanup are the main review points.

Test methodology

• Fresh Arcade builds pass with no warnings or errors. All 66 targeted ownership scenarios and controls pass on x86 and x64.
• Tests verify both that unused resources are released and that objects still in use remain usable. Navigation tests verify old-page cleanup and continued event delivery in a sibling frame.
• The VB6 test passes on x86 with a clean process exit. Broader affected WinForms and shared infrastructure suites have passing results from the September 5 validation.
• WPF application workflows and third-party controls remain outside this validation.

Test environment(s)

Windows, x86 and x64; .NET 11.0.0-rc.1.26451.109, Debug configuration.

Microsoft Reviewers: Open in CodeFlow

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.84861%. Comparing base (9cf27ae) to head (277773a).
⚠️ Report is 24 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #15059         +/-   ##
===================================================
+ Coverage   37.24166%   37.84861%   +0.60694%     
===================================================
  Files            246         246                 
  Lines           9774        9789         +15     
  Branches        1029        1031          +2     
===================================================
+ Hits            3640        3705         +65     
+ Misses          5970        5920         -50     
  Partials         164         164                 

Flag	Coverage Δ
Debug	37.84861% <80.00000%> (+0.60694%)	⬆️
production	40.04608% <80.00000%> (+0.68082%)	⬆️
test	20.64923% <ø> (ø)
unit	40.04608% <80.00000%> (+0.68082%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🟡 Changes recommended

HtmlShimManager still retains unloaded-window native references through undisposed _associatedWindow pointers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes native COM/GDI resource leaks across WinForms interop, accessibility, ActiveX, and browser hosting.

Changes:

• Balances native COM references across shared helpers and callers.
• Cleans obsolete HTML event shims during navigation.
• Adds ownership regression tests and restores VB6 shutdown coverage.

File summaries

File	Description
src/test/unit/System.Windows.Forms/WebBrowserBaseTests.cs	Tests browser COM-reference cleanup.
src/test/unit/System.Windows.Forms/System/Windows/Forms/WebBrowserTests.cs	Tests browser shutdown and disposal.
src/test/unit/System.Windows.Forms/System/Windows/Forms/HtmlWindowTests.cs	Tests unload cleanup and sibling-frame preservation.
src/test/unit/System.Windows.Forms/System/Windows/Forms/HtmlElementTests.cs	Tests element event ownership.
src/test/unit/System.Windows.Forms/System/Windows/Forms/HtmlDocumentTests.cs	Tests DOM wrapper and document-event ownership.
src/test/unit/System.Windows.Forms/System/Windows/Forms/DataObjectTests.cs	Tests OLE adapter reference ownership.
src/test/unit/System.Windows.Forms/System/Windows/Forms/ComponentModel/Com2Interop/ComNativeDescriptorTests.cs	Verifies COM cleanup in descriptor tests.
src/test/unit/System.Windows.Forms/System/Windows/Forms/AxHostTests.cs	Tests font, picture, and ambient-property ownership.
src/test/unit/System.Windows.Forms/System/Windows/Forms/AxHost.VisualBasic6Tests.cs	Restores VB6 shutdown regression coverage.
src/test/unit/System.Windows.Forms/System/Windows/Forms/AccessibleObjects/AccessibleObjectTests.cs	Tests accessibility navigation cleanup.
src/System.Windows.Forms/System/Windows/Forms/Controls/WebBrowser/WebBrowserContainer.cs	Releases retrieved client-site references.
src/System.Windows.Forms/System/Windows/Forms/Controls/WebBrowser/WebBrowserBase.cs	Balances ActiveX instance references.
src/System.Windows.Forms/System/Windows/Forms/Controls/WebBrowser/HtmlShimManager.cs	Cleans unloaded-window shims, but leaves associated window pointers undisposed.
src/System.Windows.Forms/System/Windows/Forms/ActiveX/AxHost.cs	Releases temporary native font references.
src/System.Windows.Forms/System/Windows/Forms/Accessibility/AccessibleObject.cs	Releases accessibility relationship pointers.
src/System.Windows.Forms.Primitives/tests/UnitTests/Windows/Win32/ComHelpersTests.cs	Adds COM ownership contract tests.
src/System.Windows.Forms.Primitives/tests/UnitTests/Windows/Win32/AgileComPointerTests.cs	Tests GIT retrieval reference stability.
src/System.Windows.Forms.Primitives/tests/TestUtilities/Windows/Win32/System/Com/ComClassFactory.cs	Releases factory-created native references.
src/System.Private.Windows.Core/src/Windows/Win32/System/Com/ComHelpers.cs	Balances queried IUnknown references.

Review details

• Files reviewed: 19/19 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[KlausLoeffelmann]

@JeremyKuhne, can you take a look at the Copilot comment and assess how important that one is? Can you address it, if you feel it should be addressed? Thanks!

[JeremyKuhne]

@JeremyKuhne, can you take a look at the Copilot comment and assess how important that one is? Can you address it, if you feel it should be addressed? Thanks!

Sorry, missed this. Updated so the resource gets released right away, more details are in line with the review comment.

[Copilot]

🔵 Needs a closer look

ActiveX quick activation and attached HTML event handlers still retain native resources.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/System.Windows.Forms/System/Windows/Forms/ActiveX/AxHost.cs:3665

• This balances the reference only for GetIFontFromFont, but GetIFontPointerFromFont also returns an owning OleCreateFontIndirect reference to QuickActivate at line 3319, where it is never released after the call. The new test helper correctly wraps the same result in a ComScope (AxHostTests.cs:1577), confirming the ownership contract. Scope the production QACONTAINER.pFont pointer across iqa.QuickActivate as well, or repeated ActiveX activation still leaks one native font.
  src/System.Windows.Forms/System/Windows/Forms/Controls/WebBrowser/HtmlShimManager.cs:178
• This unload path still leaves AttachEventHandler subscriptions connected. shim.Dispose() dispatches to each derived DisconnectFromEvents, but the document, element, and window overrides only clear _cookie and never call HtmlShim.DisconnectFromEvents(), so _attachedEventList is not detached as the base contract requires. Add the base call to all three overrides and cover an attached-handler navigation case; otherwise the old page can continue retaining its proxy and handler after this match succeeds.

• Files reviewed: 21/21 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[JeremyKuhne]

Addressed both additional findings in f673d1d.

• Quick activation now releases the host's font reference on both success and failure.
• Document, element, and window shims now detach AttachEventHandler subscriptions during disposal. This cleanup deliberately runs in Dispose, not every DisconnectFromEvents call: removing the last ordinary event handler must not remove independently attached handlers.

Both omissions were pre-existing. The new regression checks reproduced the missing cleanup before the fixes and pass afterward, including native callback suppression, navigation cleanup, and preservation of independent handlers.

Validation: affected ActiveX and HTML suites pass on x86 and x64, with 825 passed and 2 skipped per architecture and clean process exits.

[Copilot]

🔵 Needs a closer look

The broad COM ownership and unload-timing changes across legacy ActiveX, MSHTML, and accessibility paths warrant final human validation.

Review details

• Files reviewed: 22/22 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[KlausLoeffelmann]

AI keeps flagging this - so I took an experimental Agent, which I came up with during work on MS Bench (different context original, but the approach could have applied here).

Question first:
You wrote "not a regression". Not from 10 to 11, alright. But was it a regression from 9 to 10?
Or does this come from NetFX.
If not - do you know from the top of your head, which PR introduced the regression?

So - to the PR done with said Agent-support:
I do not understand everything in detail (really not) what that Agent came up with, but FWIW to me it looks at least somewhat plausible on first glance.

I'll put it out in draft soon and have no intention to ever merge it - this is only to assess the usefulness/reliability of that approach in this field of expertise to gain experience.

That's why I would ask you to review and assess it, so we can see, if it is nonsense or actually useful. If the latter - feel free to just cherry-pick those approaches which make sense to you into your fix, particularly if they'd improve quality or actually prevent other leak risks.

Thanks!

[JeremyKuhne]

@KlausLoeffelmann I'm investigating.

You wrote "not a regression". Not from 10 to 11, alright. But was it a regression from 9 to 10?

It is almost certainly a fallout of the AOT related COM work. I didn't root cause to a specific PR as I was taking that as a given. I should have tweaked the description more.

I'm digging into your PR and will integrate whatever makes sense. I'll also try to root cause any holes the agent left (in either case). I suspect that mine was trying too hard to isolate fixes as it found them.

[JeremyKuhne]

Update

I have incorporated the product design and implementation work from #15087 into this PR. Credit to @KlausLoeffelmann and Copilot for identifying and implementing the broader ownership and lifecycle model. The shared product files now match #15087, while this PR retains its accessibility relationship cleanup, ComClassFactory and VB6 shutdown fixes, bitmap coverage, and additional ownership tests. I also added direct IDataObject::SetData transfer coverage for both current .NET and .NET Framework.

The earlier version of this PR did not cover the problem as completely as I expected. The tests were detailed, but several of them shared assumptions with the implementation instead of independently testing the product lifecycle and COM contracts:

• Every HTML navigation test subscribed to HtmlWindow.Unload. That application subscription incidentally established the native connection the manager itself did not establish. The tests therefore proved cleanup after the notification reached the manager, but did not prove that ordinary document-only or element-only subscriptions caused the manager to observe unload.
• Attached-event tests treated one managed delegate as one native proxy. That matched the dictionary representation and missed duplicate registrations, the same delegate under different event names, unmatched removal, multicast delegates, and the independent lifetimes of standard connection-point events and attached dispatch proxies.
• The ownership audit focused primarily on returned references rather than building a complete ledger across borrowed, retained, and transferred values. That left temporary caller-owned references at Advise, SetClientSite, and QuickActivate, and it missed the successful fRelease=true transfer contract for IDataObject::SetData.
• Follow-up fixes addressed the individual leaks being observed, but I did not step back and re-derive the complete activation, identity, transfer, reentrancy, and teardown invariants before considering the work complete.

The revised implementation now:

• observes HTML window unload independently of application subscriptions and preserves sibling-frame subscriptions;
• tracks every successful attached-event registration by native proxy identity and actual delegate multiplicity;
• removes owners from manager lookup state before native cleanup can reenter managed code;
• keeps removal non-creating after unload or disposal and attempts all independent cleanup when one detach fails;
• balances temporary QuickActivate, connection-point, SetClientSite, CoCreateInstance, and GetClientSite references; and
• transfers STGMEDIUM ownership only after a successful SetData call with fRelease=true, without reading or releasing a medium the recipient may already have destroyed.

Validation completed locally:

• Arcade Debug and Release solution builds succeeded, including Microsoft.Private.Windows.Core on net481 and the modern private-core targets.
• The complete affected HTML test classes passed 576 tests with two pre-existing skips.
• The remaining affected WinForms classes passed 1,483 tests.
• The shared ComHelpers and AgileComPointer classes passed 33 tests in both Debug and Release.
• The new private-core SetData matrix passed all 10 cases on current .NET and all 10 on net481, in both Debug and Release.
• Product parity checks found no differences from [Experimental Agent] Fix COM ownership and HTML subscription lifetimes/Alternative fix attempt #15087 in the imported product files and no differences from this PR's original accessibility, ComClassFactory, or VB6 fixes.

The main review focus should now be the manager-owned unload observation, attached-event registration model, SetData transfer boundary, and reentrant cleanup ordering.