Skip to content

Fix resource leaks in WinForms native-object cleanup - #15059

Open
JeremyKuhne wants to merge 3 commits into
dotnet:mainfrom
JeremyKuhne:fix/native-resource-cleanup
Open

Fix resource leaks in WinForms native-object cleanup#15059
JeremyKuhne wants to merge 3 commits into
dotnet:mainfrom
JeremyKuhne:fix/native-resource-cleanup

Conversation

@JeremyKuhne

@JeremyKuhne JeremyKuhne commented Sep 8, 2026

Copy link
Copy Markdown
Member

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 Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.23785%. Comparing base (9cf27ae) to head (f673d1d).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #15059         +/-   ##
===================================================
- Coverage   37.24166%   37.23785%   -0.00382%     
===================================================
  Files            246         246                 
  Lines           9774        9775          +1     
  Branches        1029        1029                 
===================================================
  Hits            3640        3640                 
- Misses          5970        5971          +1     
  Partials         164         164                 
Flag Coverage Δ
Debug 37.23785% <0.00000%> (-0.00382%) ⬇️
production 39.36072% <0.00000%> (-0.00454%) ⬇️
test 20.64923% <ø> (ø)
unit 39.36072% <0.00000%> (-0.00454%) ⬇️

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member Author

@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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

Copy link
Copy Markdown
Member Author

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants