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

[windows] fix memory leak in TabbedPage #23281

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jun 26, 2024

Context: #23166

This expands upon #23166 (iOS/Catalyst memory leaks for TabbedPage),
fixing the same problem on Windows. I could reproduce the problem in
MemoryTests.cs.

I found the NavigationViewItemViewModel.Data property held the
ContentPage that held onto the TabbedPage. I made the backing field
a WeakReference and the test in MemoryTests.cs now passes on Windows.

@jonathanpeppers jonathanpeppers added the memory-leak 💦 Memory usage grows / objects live forever (sub: perf) label Jun 26, 2024
@jonathanpeppers
Copy link
Member Author

Update: I just reverted my changes related to the events, and it seems like maybe only NavigationViewItemViewModel.Data is a problem?

Will know for sure when CI is working again.

@jonathanpeppers jonathanpeppers force-pushed the WindowsTabbedPageLeaks branch 2 times, most recently from 2b9de8b to 75325f1 Compare June 28, 2024 13:37
@jonathanpeppers jonathanpeppers marked this pull request as ready for review June 28, 2024 13:37
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner June 28, 2024 13:37

Verified

This commit was signed with the committer’s verified signature.
eddumelendez Eddú Meléndez Gonzales
Context: dotnet#23166

This expands upon dotnet#23166 (iOS/Catalyst memory leaks for `TabbedPage`),
fixing the same problem on Windows. I could reproduce the problem in
`MemoryTests.cs`.

I found the `NavigationViewItemViewModel.Data` property held the
`ContentPage` that held onto the `TabbedPage`. I made the backing field
a `WeakReference` and the test in `MemoryTests.cs` now passes on Windows.
@jonathanpeppers jonathanpeppers force-pushed the WindowsTabbedPageLeaks branch from 75325f1 to 8bd1226 Compare June 28, 2024 18:05
@jonathanpeppers
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen merged commit 01408ca into dotnet:main Jul 2, 2024
47 checks passed
@jonathanpeppers jonathanpeppers deleted the WindowsTabbedPageLeaks branch July 2, 2024 19:20
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2024
@samhouts samhouts added fixed-in-8.0.70 fixed-in-net9.0-nightly This may be available in a nightly release! labels Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-tabbedpage TabbedPage fixed-in-8.0.70 fixed-in-net9.0-nightly This may be available in a nightly release! memory-leak 💦 Memory usage grows / objects live forever (sub: perf) platform/windows 🪟
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants