Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[UWP] CollectionView Memory Leak #14780

Merged
merged 10 commits into from
Dec 30, 2021
Merged

[UWP] CollectionView Memory Leak #14780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ac0d7cf
unsubscribe from oldElement on TearDownOldElement
YZahringer Oct 22, 2021
6956a00
Cleanup ItemContentControl.DataContext on CleanUpCollectionViewSource
YZahringer Oct 23, 2021
e5d250c
Merge branch 'xamarin:5.0.0' into bug-uwp-collectionview-memory-leak
YZahringer Nov 4, 2021
bf47939
Merge branch 'xamarin:5.0.0' into bug-uwp-collectionview-memory-leak
YZahringer Nov 23, 2021
caf1e1e
Merge branch 'xamarin:5.0.0' into bug-uwp-collectionview-memory-leak
YZahringer Nov 26, 2021
8fd6809
Merge branch 'xamarin:5.0.0' into bug-uwp-collectionview-memory-leak
YZahringer Dec 1, 2021
9f4d7c8
Merge branch 'xamarin:5.0.0' into bug-uwp-collectionview-memory-leak
YZahringer Dec 9, 2021
6547b5c
Merge branch 'xamarin:5.0.0' into bug-uwp-collectionview-memory-leak
YZahringer Dec 20, 2021
cce4293
Merge branch 'xamarin:5.0.0' into bug-uwp-collectionview-memory-leak
YZahringer Dec 26, 2021
8510674
Merge branch 'xamarin:5.0.0' into bug-uwp-collectionview-memory-leak
YZahringer Dec 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions Xamarin.Forms.Platform.UAP/CollectionView/ItemsViewRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,13 @@ protected virtual void CleanUpCollectionViewSource()
CollectionViewSource = null;
}

var itemContentControls = ListViewBase.GetChildren<ItemContentControl>();
foreach (ItemContentControl itemContentControl in itemContentControls)
{
itemContentControl.FormsDataContext = null;
itemContentControl.DataContext = null;
}

if (Element?.ItemsSource == null)
{
ListViewBase.ItemsSource = null;
Expand Down Expand Up @@ -213,10 +220,11 @@ protected virtual void TearDownOldElement(ItemsView oldElement)
return;
}

if (Layout != null)
if (oldElement is StructuredItemsView oldStructuredItemsView)
{
// Stop tracking the old layout
Layout.PropertyChanged -= LayoutPropertyChanged;
oldStructuredItemsView.ItemsLayout.PropertyChanged -= LayoutPropertyChanged;
oldStructuredItemsView.ItemsLayout = null;
Copy link
Contributor Author

@YZahringer YZahringer Oct 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this null assignment, but if we don't do it, the memory leak persists.
I also tried calling SetInheritedBindingContext(oldStructuredItemsView.ItemsLayout, null) instead, without success.

@jsuarezruiz maybe you have an idea or can validate that this should not produce side effects?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Yann! Sorry I only got to this now. I will try and think how we can validate this. I assume you tested this yourself? Are you using this code now in any of your apps by any chance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Gerald, I use the simple project available here with VS Diagnostics Tools to reproduce/check the fix.

For the moment, on production we use an extended renderer as workaround on many apps. Not exactly the same code, because there are private/internal on the base renderer, but the result is the same:

public class ExtendedCollectionViewRenderer : Xamarin.Forms.Platform.UWP.CollectionViewRenderer
{
    protected override void Dispose(bool disposing)
    {
        base.TearDownOldElement(Element);

        if (Element.ItemsLayout is LinearItemsLayout linearItemsLayout)
        {
            Element.ItemsLayout = new LinearItemsLayout(linearItemsLayout.Orientation);
        }
        else if (Element.ItemsLayout is GridItemsLayout gridItemsLayout)
        {
            Element.ItemsLayout = new GridItemsLayout(gridItemsLayout.Span, gridItemsLayout.Orientation);
        }
        else
        {
            Element.ItemsLayout = null;
        }

        base.Dispose(disposing);
    }

    protected override void CleanUpCollectionViewSource()
    {
        var itemContentControls = ListViewBase.GetChildren<ItemContentControl>();
        foreach (var itemContentControl in itemContentControls)
        {
            itemContentControl.FormsDataContext = null;
            itemContentControl.DataContext = null;
        }

        base.CleanUpCollectionViewSource();
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the build completes there should be NuGets produced, would you maybe be able to try them out on projects where you have this problem and see if this works as expected and doesn't break anything else? The instructions can be found here. That would be very helpful, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some tests with the nuget produced on an application with multiple CollectionView on different views, the Memory Leak is solved.
By the way, I also updated the test project.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cooperation @YZahringer !

}

// Stop listening for ScrollTo requests
Expand Down Expand Up @@ -604,7 +612,7 @@ bool IsElementVisibleInContainer(FrameworkElement element, FrameworkElement cont

default:
return elementBounds.Left < containerBounds.Right && elementBounds.Right > containerBounds.Left;
};
}
}

void OnScrollViewChanged(object sender, ScrollViewerViewChangedEventArgs e)
Expand Down
4 changes: 2 additions & 2 deletions Xamarin.Forms.Platform.UAP/CollectionView/SelectableItemsViewRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ protected override void TearDownOldElement(ItemsView oldElement)
oldListViewBase.SelectionChanged -= NativeSelectionChanged;
}

if (ItemsView != null)
if (oldElement is TItemsView oldItemsView)
{
ItemsView.SelectionChanged -= FormsSelectionChanged;
oldItemsView.SelectionChanged -= FormsSelectionChanged;
}

base.TearDownOldElement(oldElement);
Expand Down