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

Android CollectionView leaks if given a header or footer #16961

Closed
jknaudt21 opened this issue Aug 23, 2023 · 2 comments · Fixed by #17012
Closed

Android CollectionView leaks if given a header or footer #16961

jknaudt21 opened this issue Aug 23, 2023 · 2 comments · Fixed by #17012
Assignees
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 memory-leak 💦 Memory usage grows / objects live forever platform/android 🤖 t/bug Something isn't working
Milestone

Comments

@jknaudt21
Copy link
Contributor

Description

This bug was discovered when modifying the tests for this PR: #16870

Turns out that when you create a CollectionView and give it a header or footer, it begins to leak.

I was expecting to see 5 logical children, but instead saw that there were 9.

Steps to Reproduce

  1. Add a header and footer to the collection view on the ItemsSourceDoesNotLeak test. If Fix CollectionView not displaying header or footers #16870 has already been merged, then delete the preprocessor directives that were added
			var collectionView = new CollectionView
			{
				Header = new Label { Text = "Header" },
				Footer = new Label { Text = "Footer" },
				ItemTemplate = new DataTemplate(() => new Label())
			};
  1. Run the test and observe the failure: the collection view is leaking memory and adding too many logical children

Link to public reproduction project repository

Maui main

Version with bug

8.0.0-preview.7.8842

Is this a regression from previous behavior?

No, this is something new

Last version that worked well

Unknown/Other

Affected platforms

Android

Affected platform versions

No response

Did you find any workaround?

No response

Relevant log output

No response

@jknaudt21 jknaudt21 added t/bug Something isn't working area-controls-collectionview CollectionView, CarouselView, IndicatorView memory-leak 💦 Memory usage grows / objects live forever labels Aug 23, 2023
@jknaudt21 jknaudt21 self-assigned this Aug 23, 2023
@rachelkang
Copy link
Member

fyi @jonathanpeppers

@rachelkang rachelkang added this to the Backlog milestone Aug 23, 2023
@ghost
Copy link

ghost commented Aug 23, 2023

We've added this issue to our backlog, and we will work to address it as time and resources allow. If you have any additional information or questions about this issue, please leave a comment. For additional info about issue management, please read our Triage Process.

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Aug 25, 2023
Fixes: dotnet#16961

Debugging the `ItemsSourceDoesNotLeak()` test with a new `Header` and
`Footer` on Android, there was some very odd behavior...

At the end of the test, `_logicalChildren` contained 9 items!

    Header
    Footer
    Header
    Footer
    Header
    null
    null
    null
    Footer

Where I would expect it to contain 5?!?

It appears the problem is:

* `CollectionView.ItemsSource` changes

* We notify Android's `RecyclerView` to refresh

* `RecyclerView` calls `OnCreateViewHolder()` for the header, footer,
  and all rows.

* `CreateHeaderFooterViewHolder()` is called again for the header &
  footer.

* Duplicate `viewHolder.View` items are added as logical children.

To fix this, an idea:

* Create a new `internal ContainsLogicalChild()` method

* Call this before adding the same `View` in
  `CreateHeaderFooterViewHolder()`

This isn't the *best*, because it will iterate over the
`_logicalChildren` to decide if the list contains the view. But this is
better than what strange behavior might result from this?
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Aug 28, 2023
Fixes: dotnet#16961

Debugging the `ItemsSourceDoesNotLeak()` test with a new `Header` and
`Footer` on Android, there was some very odd behavior...

At the end of the test, `_logicalChildren` contained 9 items!

    Header
    Footer
    Header
    Footer
    Header
    null
    null
    null
    Footer

Where I would expect it to contain 5?!?

It appears the problem is:

* `CollectionView.ItemsSource` changes

* We notify Android's `RecyclerView` to refresh

* `RecyclerView` calls `OnCreateViewHolder()` for the header, footer,
  and all rows.

* `CreateHeaderFooterViewHolder()` is called again for the header &
  footer.

* Duplicate `viewHolder.View` items are added as logical children.

To fix this, an idea:

* Create a new `internal ContainsLogicalChild()` method

* Call this before adding the same `View` in
  `CreateHeaderFooterViewHolder()`

This isn't the *best*, because it will iterate over the
`_logicalChildren` to decide if the list contains the view. But this is
better than what strange behavior might result from this?
PureWeen pushed a commit that referenced this issue Aug 28, 2023
…17012)

* [android] CollectionView logical children grows with Header/Footer

Fixes: #16961

Debugging the `ItemsSourceDoesNotLeak()` test with a new `Header` and
`Footer` on Android, there was some very odd behavior...

At the end of the test, `_logicalChildren` contained 9 items!

    Header
    Footer
    Header
    Footer
    Header
    null
    null
    null
    Footer

Where I would expect it to contain 5?!?

It appears the problem is:

* `CollectionView.ItemsSource` changes

* We notify Android's `RecyclerView` to refresh

* `RecyclerView` calls `OnCreateViewHolder()` for the header, footer,
  and all rows.

* `CreateHeaderFooterViewHolder()` is called again for the header &
  footer.

* Duplicate `viewHolder.View` items are added as logical children.

To fix this, an idea:

* Create a new `internal ContainsLogicalChild()` method

* Call this before adding the same `View` in
  `CreateHeaderFooterViewHolder()`

This isn't the *best*, because it will iterate over the
`_logicalChildren` to decide if the list contains the view. But this is
better than what strange behavior might result from this?

* Check `viewHolder.View.Parent != ItemsView`
@samhouts samhouts modified the milestones: Backlog, .NET 8 GA Aug 29, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 28, 2023
@samhouts samhouts added the fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 label Jan 31, 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-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 memory-leak 💦 Memory usage grows / objects live forever platform/android 🤖 t/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants