Fix resource leaks in WinForms native-object cleanup - #15059
Conversation
Codecov Report❌ Patch coverage is 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
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 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.
|
@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. |
There was a problem hiding this comment.
🔵 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, butGetIFontPointerFromFontalso returns an owningOleCreateFontIndirectreference toQuickActivateat line 3319, where it is never released after the call. The new test helper correctly wraps the same result in aComScope(AxHostTests.cs:1577), confirming the ownership contract. Scope the productionQACONTAINER.pFontpointer acrossiqa.QuickActivateas 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
AttachEventHandlersubscriptions connected.shim.Dispose()dispatches to each derivedDisconnectFromEvents, but the document, element, and window overrides only clear_cookieand never callHtmlShim.DisconnectFromEvents(), so_attachedEventListis 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
|
Addressed both additional findings in f673d1d.
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. |
There was a problem hiding this comment.
🔵 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
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
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
Test environment(s)
Windows, x86 and x64; .NET
11.0.0-rc.1.26451.109, Debug configuration.Microsoft Reviewers: Open in CodeFlow