dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

Memory leak in ContextMenuStrip with ToolStripMenuItem submenus #93520

Closed romanzimin22 closed 1 year ago

romanzimin22 commented 1 year ago

.NET version

8.0 Preview 8.0.100-rc.1.23455.8

Did it work in .NET Framework?

Yes

Did it work in any of the earlier releases of .NET Core or .NET 5+?

Worked in net6 and seems to work if built for net7

Issue description

Creating context menu with submenus causes memory usage growing at a rate 1GB per sec without any client actions

Steps to reproduce

WinFormsApp1.zip

  1. Start attached app
  2. Click the button - it will show Form2
  3. Watch the memory. If it doesn't start growing, close the second form and repeat step 2
  4. May take 3-5 retries until memory starts growing by itself without any actions

In a complex UI it takes 1 click to repro, so the issue isn't in disposing. And the menu is much smaller. Could be 12 items with the same result of uncontrolled memory leaking

elachlan commented 1 year ago

@Olina-Zhang could your team please confirm?

Olina-Zhang commented 1 year ago

Repro it in .Net 8.0 the latest build, cannot repro in .Net 7.0 and .Net 6.0. .Net 8.0 result: image MemoryLeak

elachlan commented 1 year ago

I tried with a repo in WinformsControlTest. But was unsuccessful. Any suggestions?

internal class ContextMenuTestForm : Form
{
    public ContextMenuTestForm()
    {
        Button button = new();
        button.Text = "Click Me";
        button.Click += button1_Click;
        Controls.Add(button);
    }

    private void button1_Click(object sender, EventArgs e)
    {
        ContextMenuForm contextMenuForm = new();
        contextMenuForm.Show(this);
    }

    public class ContextMenuForm : Form
    {
        private ContextMenuStrip m_Menu;

        protected override void Dispose(bool disposing)
        {
            if (disposing)
            {
                if (m_Menu is not null)
                {
                    m_Menu.Dispose();
                }
            }

            base.Dispose(disposing);
        }

        public ContextMenuForm()
        {
            m_Menu = new ContextMenuStrip();
            for (int i = 0; i < 100; i++)
            {
                var menu = new ToolStripMenuItem("Menu" + i.ToString());
                m_Menu.Items.Add(menu);
                var subMenu = new ToolStripMenuItem("SubMenu1");
                menu.DropDownItems.Add(subMenu);
                subMenu = new ToolStripMenuItem("SubMenu2");
                menu.DropDownItems.Add(subMenu);
            }
        }
    }
}
elachlan commented 1 year ago

This also reminds me a bit of dotnet/winforms#530 Does the issue repro if you remove the this in the form.show() call?

Olina-Zhang commented 1 year ago

Does the issue repro if you remove the this in the form.show() call?

Yes, it also repro with removing the this in the form.show() call in that provided application. And waiting for several minutes, the memory usage is down. image

elachlan commented 1 year ago

@Olina-Zhang does it repo on main (.NET9)?

Olina-Zhang commented 1 year ago

@Olina-Zhang does it repo on main (.NET9)?

Yes, I copy that testing code into ScratchProject for main branch of Winforms repo, it has the same result.

elachlan commented 1 year ago

Yay! I reproduced it. Only happens in release build. Was unable to reproduce in debug. Narrowing it down now.

elachlan commented 1 year ago

Minimum viable reproduction. must be in release build

internal class ContextMenuTestForm : Form
{
    private ContextMenuStrip m_Menu;

    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
            m_Menu?.Dispose();
        }

        base.Dispose(disposing);
    }

    public ContextMenuTestForm()
    {
        m_Menu = new ContextMenuStrip();
        for (int i = 0; i < 170; i++)
        {
            var menu = new ToolStripMenuItem($"Menu{i}");
            m_Menu.Items.Add(menu);
            var subMenu = new ToolStripMenuItem("SubMenu1");
            menu.DropDownItems.Add(subMenu);
            subMenu = new ToolStripMenuItem("SubMenu2");
            menu.DropDownItems.Add(subMenu);
        }
    }
}
elachlan commented 1 year ago

I have bisected it to dotnet/winforms#9538

Which was the switch from .NET8 Preview 7 to .NET8 RC1.

@JeremyKuhne I am unsure how to proceed with this?

qqpeter99 commented 1 year ago

Can this be fixed soon in the official .Net8 release? Our product depends on .Net8 Winforms for the upcoming release. This impacts our product quality in a big way.

elachlan commented 1 year ago

@KlausLoeffelmann can you please have a quick look at this?

KlausLoeffelmann commented 1 year ago

Jeremy will be OOF for the next 3 weeks.

I could reproduce it both in Release and Debug and both with and without an attached Debugger.

I am not sure, though, if Context Menu or other components being involved here are a red herring. I wouldn't be surprised, if this has fundamentally different other root causes.

@ericstj: Anything which would come to mind when you hear the symptoms? What really makes me wonder is, that if you close the 2nd form, which seems to start triggering the enormous memory consumption, the memory continues to be gotten consumed. If it reaches the limit of available memory, it just starts to free-up memory, until it comes down to the previous level of used memory.

ericstj commented 1 year ago

My guess is large objects that are not disposed. That would explain why it will correct itself once limit is approached - GC will kick in and free up those objects on the LOH. If they were actually rooted or if it was a native leak I wouldn't expect it to correct itself. This doc has some good pointers: https://learn.microsoft.com/en-us/dotnet/core/diagnostics/debug-memory-leak

Look for what objects are taking up the most space on the heap in that dump - then go see what should be responsible for disposing them.

elachlan commented 1 year ago

@ericstj Diagnostic tools in VS doesn't show any difference in the memory usage even though the process memory keeps increasing. Could it be a leak in the OS based components? image

image

ericstj commented 1 year ago

It could be that the memory is on the OS side, but the managed side is responsible for lifetime and manages that through a handle (as a SafeHandle or IntPtr). For example: a Font would be backed with a handle to a native object representing that font. I'm not sure how to read this type list with ValueTuples -- could that mean that you have 25K Font objects rooted with those ValueTuple- or are those just boxed ValueTuples to single Font object. I'm also not sure what this is showing - is this after the memory has gone down as shown in the first screenshot? If so then it's too late to be looking at object counts since the memory spike was already resolved.

merriemcgaw commented 1 year ago

Can this be fixed soon in the official .Net8 release? Our product depends on .Net8 Winforms for the upcoming release. This impacts our product quality in a big way.

We are on top of investigating it and will try to have a fix as soon as possible. We lock down very soon for .NET 8 RTM so I can't make a firm promise. Just know that we recognize the importance and are prioritizing accordingly.

romanzimin22 commented 1 year ago

If I attach a debugger to the running app in mixed mode instead of starting it, it gives me Heap Profiling button, and looks like all the memory is there, native memory: image

Tanya-Solyanik commented 1 year ago

Handle and GDI object counts are stable when active private working set is growing - image

kirsan31 commented 1 year ago

I was trying to investigate but with SDK 8.0.100-preview.6 (the latest available for 17.7.5 VS) the issue doesn't repro :(

elachlan commented 1 year ago

@kirsan31 I bisected the issue to .NET 8 RC1

kirsan31 commented 1 year ago

I reproduce it with SDK 8.0.100-rc.2.

  1. this is not direct managed object leak.
  2. this is not direct handle leak.
  3. after some time the problem is goes away, and this is not related to GC and memory limits.
  4. Net Virtual Alloc Stacks in perfView surprised me: image

And yes disabling Tiered Compilation (<TieredCompilation>false</TieredCompilation>) fix the problem... So in point 3 I think problem is goes away after Tiered Compilation completed.

elachlan commented 1 year ago

This explains why it doesn't reproduce after you let it settle! Good find!!

@ericstj that makes this a runtime issue if it's tiered compilation, right?

elachlan commented 1 year ago

@jkotas you might be interested in this. Tiered compilation causing massive memory usage.

ghost commented 1 year ago

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch See info in area-owners.md if you want to be subscribed.

Issue Details
### .NET version 8.0 Preview 8.0.100-rc.1.23455.8 ### Did it work in .NET Framework? Yes ### Did it work in any of the earlier releases of .NET Core or .NET 5+? Worked in net6 and seems to work if built for net7 ### Issue description Creating context menu with submenus causes memory usage growing at a rate 1GB per sec without any client actions ### Steps to reproduce [WinFormsApp1.zip](https://github.com/dotnet/winforms/files/12832438/WinFormsApp1.zip) 1. Start attached app 2. Click the button - it will show Form2 3. Watch the memory. If it doesn't start growing, close the second form and repeat step 2 4. May take 3-5 retries until memory starts growing by itself without any actions In a complex UI it takes 1 click to repro, so the issue isn't in disposing. And the menu is much smaller. Could be 12 items with the same result of uncontrolled memory leaking
Author: romanzimin22
Assignees: -
Labels: `area-CodeGen-coreclr`, `untriaged`, `needs-area-label`
Milestone: -
jkotas commented 1 year ago

@jakobbotsch Is this duplicate of https://github.com/dotnet/runtime/issues/93342 ?

jakobbotsch commented 1 year ago

Yes, this is the same problem. We hit the issue while compiling System.Windows.Forms.ToolStripControlHost.OnSubscribeControlEvents(System.Windows.Forms.Control) with PGO data.

Thanks for reporting this @romanzimin22, and thanks everyone who helped investigate. I've confirmed this issue is fixed for the provided repro case. The fix will be part of the GA release of .NET 8.

elachlan commented 1 year ago

Has this fix been included in .NET 9 as well?

jakobbotsch commented 1 year ago

Has this fix been included in .NET 9 as well?

The fix is in main, but it looks like the current .NET 9 build on dotnet/installer is very outdated (from Sep 20), so the fix hasn't propagated there yet.