Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dinamicaly adding/removing MenuItem-s in Menu leads to memory leaks #15990

Closed
ds1709 opened this issue Jun 11, 2024 · 8 comments · Fixed by #15998
Closed

Dinamicaly adding/removing MenuItem-s in Menu leads to memory leaks #15990

ds1709 opened this issue Jun 11, 2024 · 8 comments · Fixed by #15998
Labels

Comments

@ds1709
Copy link
Contributor

ds1709 commented Jun 11, 2024

Describe the bug

When adding MenuItem-s into Menu and then removing them, Menu keep holding reference to removed MenuItem-s.
Here screenshots from memory profiler.
image
and quick watch of MenuItem (Parent is null)
image
and also quick watch of Menu (Items collection is empty)
image
As you can see, Menu keep holding reference on removed MenuItem throught chain of strong event subscriptions.

To Reproduce

            var w = new Window1(); // Custom window with Menu.

            var mi = new MenuItem { Header = "FooBar" };
            mi.Click += (_, _) => w.MainMenu.Items.Remove(mi);

            w.MainMenu.Items.Add(mi);

Expected behavior

No response

Avalonia version

11.0.10

OS

No response

Additional context

No response

@ds1709 ds1709 added the bug label Jun 11, 2024
@ds1709
Copy link
Contributor Author

ds1709 commented Jun 11, 2024

After a little researche, I found that subscription is cached in _innerSerialDisposable field of class Switch<T>._. When MenuItem is added or removed, VisualParentProperty of it changes. Each change of property calls OnNext method of Switch<T>._. Each call creates new instance of InnerObserver and stores it in _innerSerialDisposable, but do nothing with old value. I just simply added _innerSerialDisposable.Dispose call befo setting new value, and now memory leak is gone.

        public override void OnNext(IObservable<TSource> value)
        {
            ulong id;

            lock (_gate)
            {
                id = unchecked(++_latest);
                _hasLatest = true;
            }

            var innerObserver = new InnerObserver(this, id);

            _innerSerialDisposable?.Dispose();  // <== Added Dispose call before setting new value.
            _innerSerialDisposable = innerObserver;
            innerObserver.Disposable = value.Subscribe(innerObserver);
        }

@timunie
Copy link
Contributor

timunie commented Jun 11, 2024

@ds1709 probably worth to file a PR

@ds1709
Copy link
Contributor Author

ds1709 commented Jun 11, 2024

@timunie I need this fix in version 11.0.*. So, do I need to create two PR, one in branch 11.0 and another in develop?

@timunie
Copy link
Contributor

timunie commented Jun 11, 2024

@ds1709 for 11.0 this requires probably a support agreement as we are only backporting for upcomming 11.1 at this point of time.

@ds1709
Copy link
Contributor Author

ds1709 commented Jun 11, 2024

@timunie is there any problem with support 11.0 version? This fix is not complex, so release 11.0.11 should be easy.

@timunie
Copy link
Contributor

timunie commented Jun 11, 2024

Limited resources. Nothing else.

@ds1709
Copy link
Contributor Author

ds1709 commented Jun 11, 2024

Is there any chance to get this fix in version 11.0? And another question, when is it planned to release version 11.1 (no beta)? According to the commot graph, there are a big amount of improvements from release 11.1.0-beta2. Same for 11.0.10, there are improvements, that wasn't released.

@timunie
Copy link
Contributor

timunie commented Jun 11, 2024

  1. You can request 11.0 backports if you have paid support. Reach out via website if you are interested in general
  2. 11.0.10 is actually relesed. You may need to pull templates again
  3. 11.1 should drop soonish if there's no regression found while rc testings. No actual due date

Thanks for your understanding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants