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

NavigationViewItemRevokers use WeakRef this pointers #6803

Closed

Conversation

StephenLPeters
Copy link
Contributor

Rather than having navigation view track which items have revoker objects attached to them and ensure those get cleared during navigation view's destructor, we can simply have the callback object keep a weak ref to the navigation view rather than a raw pointer.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Mar 8, 2022
@StephenLPeters StephenLPeters marked this pull request as draft March 8, 2022 22:22
@StephenLPeters
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor Author

/azp run

@StephenLPeters StephenLPeters marked this pull request as ready for review March 9, 2022 19:51
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters added area-NavigationView NavView control team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Mar 9, 2022
@StephenLPeters
Copy link
Contributor Author

@lhecker FYI

@lhecker
Copy link
Member

lhecker commented Mar 9, 2022

Love it! 🙂 I'm a bit concerned about the supposed memory leak mentioned in the other PR. Some users tend to leave their computer running for up to multiple weeks after all, but I can't judge how to solve that, or how important solving it is.

@DHowett
Copy link
Member

DHowett commented Mar 10, 2022

I am also quite concerned about introducing a leak. Terminal is not structured like other applications, as it uses transient NavViews that it expects to be destroyed when the user is done with them.
Each one costs a pretty significant amount of memory (building our settings UI raises our commit by about 15 megabytes...) and being able to reclaim that would be ideal.

@lhecker
Copy link
Member

lhecker commented Mar 10, 2022

@DHowett I might've misread the exchange between Stephen and Keith in #6797. A clarification about this new approach would be nice, since it lacks the "synchronous" cleanup that was happening before. I'm assuming there must've been a reason it was done that way... Testing for leaks in Windows Terminal will be a bit hard, since WinUI is leaking a lot of memory anyways already (~300kB every time we open the settings tab, which makes it hard to test for regressions).

@kmahone
Copy link
Member

kmahone commented Mar 10, 2022

I believe that this should function correctly without any leaks. Stephen, can you verify in the debugger that the appropriate destructors are getting called as expected?

With this change, my expectation is that NavView will not keep any NavViewItems alive that are not its children. When a NavViewItem is no longer referenced anywhere, it should get destroyed, and as part of the destruction its event handlers should get unregistered.

@kmahone
Copy link
Member

kmahone commented May 23, 2022

We went with this fix instead:
Fix issue with exception being thrown during cleanup of NavigationView. #6797

@kmahone kmahone closed this May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NavigationView NavView control team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants