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

[net7.0] [windows] fix memory leak when CollectionView.ItemsSource changes #13648

Merged
merged 3 commits into from
Mar 2, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Mar 2, 2023

Backport of #13530 to net7.0

/cc @PureWeen @jonathanpeppers

jonathanpeppers and others added 3 commits March 2, 2023 15:41
Fixes: #13393
Context: https://github.com/ivan-todorov-progress/maui-collection-view-memory-leak-bug

Debugging the above sample, I found that `ItemsView._logicalChildren`
grew indefinitely in size if you replace a `CollectionView`'s
`ItemsSource` over and over.

I could fix this by creating a new `internal`
`ItemsView.ClearLogicalChildren()` method and call it from the Windows
`ItemsViewHandler`.

I could reproduce this issue in a Device Test running on Windows.

It appears the problem does not occur on Android or iOS due to the
recycling behavior of the underlying platforms.

For example, Android calls `OnViewRecycled()` which calls
`RemoveLogicalChild(view)`:

* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/Adapters/ItemsViewAdapter.cs#L52-L57
* https://github.com/dotnet/maui/blob/53f6e393750a3df05b12fcde442daf3616c216f8/src/Controls/src/Core/Handlers/Items/Android/TemplatedItemViewHolder.cs#L37-L45

On Windows, we merely have a `ItemContentControl` and there is no
callback for when things are recycled. We *do* get a callback for when
the `FormsDataContext` value changes, so an option is to clear the
list in this case.

After this change, my test passes -- but it was already passing on iOS
and Android.
Apparently iOS is only creating 1:

    Assert.Equal() Failure\nExpected: 3\nActual:   1
@PureWeen
Copy link
Member

PureWeen commented Mar 2, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jsuarezruiz jsuarezruiz added t/housekeeping ♻︎ area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Mar 2, 2023
@PureWeen PureWeen enabled auto-merge March 2, 2023 16:56
@PureWeen PureWeen merged commit d6d21b2 into net7.0 Mar 2, 2023
@PureWeen PureWeen deleted the backport/pr-13530-to-net7.0 branch March 2, 2023 22:24
@rmarinho
Copy link
Member

/backport to release/7.0.2xx

@github-actions
Copy link
Contributor Author

Started backporting to release/7.0.2xx: https://github.com/dotnet/maui/actions/runs/4469316800

@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
@samhouts samhouts added the fixed-in-7.0.92 Look for this fix in 7.0.92! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView fixed-in-7.0.92 Look for this fix in 7.0.92! platform/windows 🪟 t/housekeeping ♻︎
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants