Skip to content

Commit

Permalink
Update tests to make GC issues with ListProxy consistent (dotnet#13973
Browse files Browse the repository at this point in the history 
)

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.
  • Loading branch information
@hartez
hartez committed Mar 16, 2023
1 parent 7e2ee17 commit 082743d
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 2 deletions.
6 changes: 5 additions & 1 deletion src/Controls/src/Core/ListProxy.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ internal sealed class ListProxy : IReadOnlyList<object>, IListProxy, INotifyColl
readonly IList _list;
readonly int _windowSize;
readonly WeakNotifyCollectionChangedProxy _proxy;
readonly NotifyCollectionChangedEventHandler _collectionChangedDelegate;

IEnumerator _enumerator;
int _enumeratorIndex;
Expand Down Expand Up @@ -44,7 +45,10 @@ internal ListProxy(IEnumerable enumerable, int windowSize = int.MaxValue, IDispa
_list = new ReadOnlyListAdapter((IReadOnlyList<object>)enumerable);

if (enumerable is INotifyCollectionChanged changed)
_proxy = new WeakNotifyCollectionChangedProxy(changed, OnCollectionChanged);
{
_collectionChangedDelegate = OnCollectionChanged;
_proxy = new WeakNotifyCollectionChangedProxy(changed, _collectionChangedDelegate);
}
}

~ListProxy() => _proxy?.Unsubscribe();
Expand Down
56 changes: 55 additions & 1 deletion src/Controls/tests/Core.UnitTests/MultiPageTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ public void TestSetChildren()
pagesAdded++;
};

SurfaceGCBugs();

container.Children.Add(CreateContainedPage());
container.Children.Add(CreateContainedPage());

Expand All @@ -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);

Expand All @@ -76,6 +80,8 @@ public void CurrentPageSetAfterAdd()
property = true;
};

SurfaceGCBugs();

page.Children.Add(child);

Assert.Same(page.CurrentPage, child);
Expand All @@ -98,6 +104,8 @@ public void CurrentPageChangedAfterRemove()
property = true;
};

SurfaceGCBugs();

page.Children.Remove(child);

Assert.Same(page.CurrentPage, child2);
Expand All @@ -118,6 +126,8 @@ public void CurrentPageNullAfterRemove()
property = true;
};

SurfaceGCBugs();

page.Children.Remove(child);

Assert.Null(page.CurrentPage);
Expand All @@ -137,6 +147,8 @@ public void TemplatedPage()
return p;
});

SurfaceGCBugs();

page.ItemsSource = new[] { "Foo", "Bar" };

Action<Page, string> assertPage = (p, s) =>
Expand Down Expand Up @@ -174,6 +186,8 @@ public void SelectedItemSetAfterAdd()
selected = true;
};

SurfaceGCBugs();

items.Add("foo");

Assert.Same(page.SelectedItem, items.First());
Expand Down Expand Up @@ -201,6 +215,8 @@ public void SelectedItemNullAfterRemove()
selected = true;
};

SurfaceGCBugs();

items.Remove("foo");

Assert.Null(page.SelectedItem);
Expand All @@ -224,6 +240,8 @@ public void SelectedItemSetAfterItemsSourceSet()
selected = true;
};

SurfaceGCBugs();

page.ItemsSource = new[] { "foo" };

Assert.Same(page.SelectedItem, ((string[])page.ItemsSource)[0]);
Expand Down Expand Up @@ -309,6 +327,8 @@ public void TemplatePagesAdded()
Assert.Equal(((Label)cp.Content).Text, s);
};

SurfaceGCBugs();

items.Add("Baz");

var pages = page.Children.ToArray();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -394,6 +416,8 @@ public void TemplatePagesInserted()
Assert.Equal(((Label)cp.Content).Text, s);
};

SurfaceGCBugs();

items.Insert(1, "Baz");

var pages = page.Children.ToArray();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -467,6 +493,8 @@ public void TemplatePagesRemoved()
Assert.Equal(((Label)cp.Content).Text, s);
};

SurfaceGCBugs();

items.Remove("Foo");

var pages = page.Children.ToArray();
Expand Down Expand Up @@ -501,6 +529,8 @@ public void TemplatePagesRangeRemoved()
Assert.Equal(((Label)cp.Content).Text, s);
};

SurfaceGCBugs();

items.RemoveAt(1, 2);

var pages = page.Children.ToArray();
Expand Down Expand Up @@ -537,6 +567,8 @@ public void TemplatePagesReordered()
Assert.Equal(((Label)cp.Content).Text, s);
};

SurfaceGCBugs();

items.Move(0, 1);

var pages = page.Children.ToArray();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -651,6 +687,8 @@ public void TemplatePagesReplaced()
Assert.Equal(((Label)cp.Content).Text, s);
};

SurfaceGCBugs();

items[0] = "Baz";

var pages = page.Children.ToArray();
Expand Down Expand Up @@ -685,6 +723,8 @@ public void TemplatedPagesSourceReplaced()

page.ItemsSource = new ObservableCollection<string> { "Baz", "Bar" };

SurfaceGCBugs();

var pages = page.Children.ToArray();
Assert.Equal(2, pages.Length);
assertPage((Page)pages[0], "Baz");
Expand Down Expand Up @@ -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"
Expand All @@ -748,6 +790,8 @@ public void PagesChangedOnTemplateChange()
fail++;
};

SurfaceGCBugs();

page.ItemTemplate = new DataTemplate(() => new ContentPage
{
Content = new Label { Text = "Content" }
Expand Down Expand Up @@ -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();
}
}
}

0 comments on commit 082743d

Please sign in to comment.