From 082743d7e48ed06f18be850f287c8077eb613e39 Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Wed, 15 Mar 2023 23:45:29 -0600 Subject: [PATCH] Update tests to make GC issues with `ListProxy` consistent (#13973) Updates MultiPage tests to ensure that ListProxy collection changed handler issues show up consistently (rather than only showing up intermittently on release builds). Fixes weak reference issue with ListProxy's collection changed handler. The ListProxy is creating a WeakNotifyCollectionChangedProxy using a method group; there's no other reference to the generated delegate, so if the GC kicks in the delegate is collected and the collection changed handler no longer fires. This PR creates a reference to the delegate in ListProxy so the delegate isn't collected. --- src/Controls/src/Core/ListProxy.cs | 6 +- .../tests/Core.UnitTests/MultiPageTests.cs | 56 ++++++++++++++++++- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/Controls/src/Core/ListProxy.cs b/src/Controls/src/Core/ListProxy.cs index c9f58dfd46b8..8d3a5bd461b1 100644 --- a/src/Controls/src/Core/ListProxy.cs +++ b/src/Controls/src/Core/ListProxy.cs @@ -16,6 +16,7 @@ internal sealed class ListProxy : IReadOnlyList, IListProxy, INotifyColl readonly IList _list; readonly int _windowSize; readonly WeakNotifyCollectionChangedProxy _proxy; + readonly NotifyCollectionChangedEventHandler _collectionChangedDelegate; IEnumerator _enumerator; int _enumeratorIndex; @@ -44,7 +45,10 @@ internal ListProxy(IEnumerable enumerable, int windowSize = int.MaxValue, IDispa _list = new ReadOnlyListAdapter((IReadOnlyList)enumerable); if (enumerable is INotifyCollectionChanged changed) - _proxy = new WeakNotifyCollectionChangedProxy(changed, OnCollectionChanged); + { + _collectionChangedDelegate = OnCollectionChanged; + _proxy = new WeakNotifyCollectionChangedProxy(changed, _collectionChangedDelegate); + } } ~ListProxy() => _proxy?.Unsubscribe(); diff --git a/src/Controls/tests/Core.UnitTests/MultiPageTests.cs b/src/Controls/tests/Core.UnitTests/MultiPageTests.cs index 0771df7d4ed3..816cedff72ce 100644 --- a/src/Controls/tests/Core.UnitTests/MultiPageTests.cs +++ b/src/Controls/tests/Core.UnitTests/MultiPageTests.cs @@ -30,6 +30,8 @@ public void TestSetChildren() pagesAdded++; }; + SurfaceGCBugs(); + container.Children.Add(CreateContainedPage()); container.Children.Add(CreateContainedPage()); @@ -50,6 +52,8 @@ public void TestOverwriteChildren() page.ChildAdded += (sender, args) => childCount++; page.ChildRemoved += (sender, args) => removeCount++; + SurfaceGCBugs(); + foreach (var child in page.Children.ToArray()) page.Children.Remove((T)child); @@ -76,6 +80,8 @@ public void CurrentPageSetAfterAdd() property = true; }; + SurfaceGCBugs(); + page.Children.Add(child); Assert.Same(page.CurrentPage, child); @@ -98,6 +104,8 @@ public void CurrentPageChangedAfterRemove() property = true; }; + SurfaceGCBugs(); + page.Children.Remove(child); Assert.Same(page.CurrentPage, child2); @@ -118,6 +126,8 @@ public void CurrentPageNullAfterRemove() property = true; }; + SurfaceGCBugs(); + page.Children.Remove(child); Assert.Null(page.CurrentPage); @@ -137,6 +147,8 @@ public void TemplatedPage() return p; }); + SurfaceGCBugs(); + page.ItemsSource = new[] { "Foo", "Bar" }; Action assertPage = (p, s) => @@ -174,6 +186,8 @@ public void SelectedItemSetAfterAdd() selected = true; }; + SurfaceGCBugs(); + items.Add("foo"); Assert.Same(page.SelectedItem, items.First()); @@ -201,6 +215,8 @@ public void SelectedItemNullAfterRemove() selected = true; }; + SurfaceGCBugs(); + items.Remove("foo"); Assert.Null(page.SelectedItem); @@ -224,6 +240,8 @@ public void SelectedItemSetAfterItemsSourceSet() selected = true; }; + SurfaceGCBugs(); + page.ItemsSource = new[] { "foo" }; Assert.Same(page.SelectedItem, ((string[])page.ItemsSource)[0]); @@ -309,6 +327,8 @@ public void TemplatePagesAdded() Assert.Equal(((Label)cp.Content).Text, s); }; + SurfaceGCBugs(); + items.Add("Baz"); var pages = page.Children.ToArray(); @@ -351,10 +371,12 @@ public void TemplatePagesRangeAdded() if (e.Action != NotifyCollectionChangedAction.Add) return; - addedCount++; + addedCount += 1; Assert.Equal(2, e.NewItems.Count); }; + SurfaceGCBugs(); + items.AddRange(new[] { "Baz", "Bam" }); Assert.Equal(1, addedCount); @@ -394,6 +416,8 @@ public void TemplatePagesInserted() Assert.Equal(((Label)cp.Content).Text, s); }; + SurfaceGCBugs(); + items.Insert(1, "Baz"); var pages = page.Children.ToArray(); @@ -430,6 +454,8 @@ public void TemplatePagesRangeInserted() Assert.Equal(((Label)cp.Content).Text, s); }; + SurfaceGCBugs(); + items.InsertRange(1, new[] { "Baz", "Bam" }); var pages = page.Children.ToArray(); @@ -467,6 +493,8 @@ public void TemplatePagesRemoved() Assert.Equal(((Label)cp.Content).Text, s); }; + SurfaceGCBugs(); + items.Remove("Foo"); var pages = page.Children.ToArray(); @@ -501,6 +529,8 @@ public void TemplatePagesRangeRemoved() Assert.Equal(((Label)cp.Content).Text, s); }; + SurfaceGCBugs(); + items.RemoveAt(1, 2); var pages = page.Children.ToArray(); @@ -537,6 +567,8 @@ public void TemplatePagesReordered() Assert.Equal(((Label)cp.Content).Text, s); }; + SurfaceGCBugs(); + items.Move(0, 1); var pages = page.Children.ToArray(); @@ -572,6 +604,8 @@ public void TemplatePagesRangeReorderedForward() Assert.Equal(((Label)cp.Content).Text, s); }; + SurfaceGCBugs(); + items.Move(1, 4, 2); var pages = page.Children.ToArray(); @@ -611,6 +645,8 @@ public void TemplatePagesRangeReorderedBackward() Assert.Equal(((Label)cp.Content).Text, s); }; + SurfaceGCBugs(); + items.Move(4, 1, 2); var pages = page.Children.ToArray(); @@ -651,6 +687,8 @@ public void TemplatePagesReplaced() Assert.Equal(((Label)cp.Content).Text, s); }; + SurfaceGCBugs(); + items[0] = "Baz"; var pages = page.Children.ToArray(); @@ -685,6 +723,8 @@ public void TemplatedPagesSourceReplaced() page.ItemsSource = new ObservableCollection { "Baz", "Bar" }; + SurfaceGCBugs(); + var pages = page.Children.ToArray(); Assert.Equal(2, pages.Length); assertPage((Page)pages[0], "Baz"); @@ -725,6 +765,8 @@ public void PagesChangedOnItemsSourceChange() fail++; }; + SurfaceGCBugs(); + page.ItemsSource = new[] { "Foo", "Bar" }; Assert.Equal(1, reset); // "PagesChanged wasn't raised or was raised too many times for Reset" @@ -748,6 +790,8 @@ public void PagesChangedOnTemplateChange() fail++; }; + SurfaceGCBugs(); + page.ItemTemplate = new DataTemplate(() => new ContentPage { Content = new Label { Text = "Content" } @@ -823,5 +867,15 @@ public void CurrentPageChanged() Assert.True(raised); } + + static void SurfaceGCBugs() + { + // Ensure a GC happens here to make sure we don't have any missing references to + // the collection changed handlers in ListProxy; without the GC calls if we introduce + // a reference bug the tests will still usually pass on Debug builds and only intermittently + // fail on Release builds. We don't want these bugs to accidentally slip by. + GC.Collect(); + GC.WaitForPendingFinalizers(); + } } }