Fix 13963: Ensure ScaleSmallIconToDpi Always Returns a New Icon to Prevent Resource Mismanagement

[LeafShi1]

Fixes #13963

Root Cause

The issue was introduced in PR https://github.com/dotnet/winforms/pull/13787/files, which changed the Form.UpdateWindowIcon implementation from _smallIcon = new Icon(icon, SystemInformation.SmallIconSize); to _smallIcon = ScaleHelper.ScaleSmallIconToDpi(icon, dpi);

The problem arises because ScaleSmallIconToDpi returns the original icon instance when its size already matches the target DPI. This caused _smallIcon and the external icon to reference the same object. When UpdateWindowIcon later disposed _smallIcon, it unintentionally disposed the original icon as well, leading to ObjectDisposedException when the original icon was reused.

Proposed changes

• Update ScaleSmallIconToDpi to always return a new Icon instance, even when the original icon already matches the target DPI size.

Customer Impact

• Apps that reuse the same Icon for multiple forms may throw ObjectDisposedException after closing a form, because the original icon gets disposed. This fix ensures the original icon remains valid and reusable, preventing these exceptions and improving reliability.

Regression?

• Yes

Risk

• Minimal

Screenshots

Before

With the demo code,

    public partial class Form1 : Form
    {
        private Icon ico;

        public Form1()
        {
            InitializeComponent();
            ico = new Icon("D:\\TestWinformIcon\\exception.size32.ico");  // change the path on your machine
        }

        private void button1_Click(object sender, EventArgs e)
        {
            var form = new Form2();
            form.Icon = ico;
            form.Show();
            form.Close();
        }

        private void button2_Click(object sender, EventArgs e)
        {
            MessageBox.Show(ico.Handle.ToString());
        }
    }

Click button1 and then click button2, exception dialog pops.

After

Click button1 and then click button2, the _ico.Handle displays normally.

Test methodology

• Manually

Test environment(s)

• .net 10.0.0-rc.1.25405.103

Microsoft Reviewers: Open in CodeFlow

[Copilot]

Pull Request Overview

This PR fixes a critical resource management bug where Form.UpdateWindowIcon could inadvertently dispose of externally-owned icon instances. The fix ensures that ScaleSmallIconToDpi always returns a new Icon instance, preventing the form from disposing icons that are still referenced by the caller.

Key Changes:

• Modified ScaleHelper.ScaleSmallIconToDpi to always create and return a new Icon instance instead of conditionally returning the original icon when dimensions match

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.18829%. Comparing base (7add703) to head (3d6a639).
⚠️ Report is 104 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #13971         +/-   ##
===================================================
+ Coverage   77.11724%   77.18829%   +0.07104%     
===================================================
  Files           3271        3276          +5     
  Lines         644354      645116        +762     
  Branches       47658       47704         +46     
===================================================
+ Hits          496908      497954       +1046     
+ Misses        143766      143473        -293     
- Partials        3680        3689          +9     

Flag	Coverage Δ
Debug	77.18829% <100.00000%> (+0.07104%)	⬆️
integration	19.07021% <100.00000%> (-0.03472%)	⬇️
production	52.09731% <100.00000%> (+0.13405%)	⬆️
test	97.40947% <ø> (-0.00662%)	⬇️
unit	49.43590% <100.00000%> (+0.14602%)	⬆️

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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kirsan31]

I think that in particular case this is a right approach - always to create a new icon. But as I can see ScaleSmallIconToDpi is used in 3 other places and it was designed to not create a new icon if not needed (from method description):

Get X, Y metrics at DPI, IF icon is not already that size, create and return a new one.

So if we go this approach we need carefully examine all 3 other places and check - did we break anything?

May be is safer (and a bit effectively) to modify

winforms/src/System.Windows.Forms/System/Windows/Forms/Form.cs

Line 6432 in 7e1617f

private unsafe void UpdateWindowIcon(bool redrawFrame, int dpi = 0)

with something like this (not update _smallIcon if new icon not created):

if (icon is not null)
{
    Icon? newSmallIcon;

    try
    {
        newSmallIcon = ScaleHelper.ScaleSmallIconToDpi(icon, dpi);
    }
    catch
    {
        newSmallIcon = _smallIcon;
    }

    if (newSmallIcon is not null)
    {
        PInvokeCore.SendMessage(this, PInvokeCore.WM_SETICON, (WPARAM)PInvoke.ICON_SMALL, (LPARAM)newSmallIcon.Handle);
        if (_smallIcon is not null && newSmallIcon != _smallIcon) // We will use a new icon - we need to dispose the old one.
        {
            _smallIcon.Dispose();
            _smallIcon = null;
        }
        
        if (newSmallIcon != icon) // We don't need to save this icon unless we create a new instance.
            _smallIcon = newSmallIcon;
    }

    PInvokeCore.SendMessage(this, PInvokeCore.WM_SETICON, (WPARAM)PInvoke.ICON_BIG, (LPARAM)icon.Handle);
}

Also may be it will be useful to check other 3 ScaleSmallIconToDpi places for same dispose problem as here too...

[LeafShi1]

I think that in particular case this is a right approach - always to create a new icon. But as I can see ScaleSmallIconToDpi is used in 3 other places and it was designed to not create a new icon if not needed (from method description):

You are right， however, as a helper method, ScaleSmallIconToDpi has the following benefits after this modification:

• The caller does not need to determine whether the reference is shared; it can safely dispose of it.
• Consistent behavior: Each call returns a new object, avoiding inconsistencies where the original object is sometimes returned and a new one is sometimes returned.
• No side effects: The passed icon is not modified.

But this is indeed a breaking change, @JeremyKuhne @KlausLoeffelmann what do you think?

[JeremyKuhne]

We cannot change this API to fix this regression. It has to be a targeted fix.

[JeremyKuhne]

There are limited GDI handles available, making this sort of change is risky.

[JeremyKuhne]

@LeafShi1 as we're running out of time I've created a PR myself. Sorry I didn't see your ping on this sooner, I was on vacation last week. #13983