Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,19 @@ void CollectionChanged(NotifyCollectionChangedEventArgs args)
if (!_collectionViewController.TryGetTarget(out var controller))
return;

// Force UICollectionView to get the internal accounting straight
var collectionView = controller.CollectionView;
UpdateSection(collectionView);

// Force UICollectionView to get the internal accounting straight.
// Skip for Remove: standard INotifyCollectionChanged semantics require the item to be
// already removed from the backing collection before CollectionChanged fires. This means
// _groupSource has N-1 items when we receive Remove, but UIKit still has N sections.
// Calling UpdateSection here triggers GetGroupCount with UIKit's stale section index
// (N-1), which is out of range on the already-mutated _groupSource → ArgumentOutOfRangeException.
// The post-processing UpdateSection call below handles reconciliation after DeleteSections.
if (args.Action != NotifyCollectionChangedAction.Remove)
{
UpdateSection(collectionView);
}

switch (args.Action)
{
Expand Down Expand Up @@ -360,6 +370,12 @@ void Move(NotifyCollectionChangedEventArgs args)

int GetGroupCount(int groupIndex)
{
// Defensive bounds check: UIKit can call back synchronously during DeleteSections,
// and other re-entrancy scenarios may present a stale groupIndex. Return 0 instead
// of throwing ArgumentOutOfRangeException.
if ((uint)groupIndex >= (uint)_groupSource.Count)
return 0;

switch (_groupSource[groupIndex])
{
case IList list:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,114 @@ await CreateHandlerAndAddToWindow<CollectionViewHandler>(collectionView, async h
});
}

// Regression test for https://github.com/dotnet/maui/issues/34691
// ObservableGroupedSource.CollectionChanged called UpdateSection() before processing
// Remove, causing GetGroupCount(N-1) to be invoked when _groupSource already had N-1
// items → ArgumentOutOfRangeException.
[Fact(DisplayName = "Removing last section from grouped CollectionView does not crash")]
public async Task ItemsSourceGroupedRemoveLastSectionDoesNotCrash()
{
SetupBuilder();

var data = new List<string> { "item 1", "item 2", "item 3" };
var groupData = new ObservableCollection<CollectionViewStringGroup>
{
new("Header 1", data),
new("Header 2", data),
new("Header 3", data),
};

var collectionView = new CollectionView
{
IsGrouped = true,
ItemsSource = groupData,
ItemTemplate = new DataTemplate(() => new Label()),
};

await CreateHandlerAndAddToWindow<CollectionViewHandler>(collectionView, async handler =>
{
await Task.Delay(1000);

// Removing the last section was the classic crash: UIKit still had N sections
// when UpdateSection() was called before Remove(), so GetGroupCount(N-1) was
// invoked against a _groupSource that already had N-1 items → out of range.
groupData.RemoveAt(groupData.Count - 1);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[major] Regression Prevention — This removes the backing group before verifying that the native UICollectionView has realized the original sections. If uiCollectionView.NumberOfSections() is still 0, ObservableGroupedSource.Remove takes the ReloadRequired() path and the test passes without exercising the stale-section DeleteSections/GetGroupCount path that caused the crash. Please force layout and assert the precondition (NumberOfSections() == groupData.Count) before each removal; the same issue applies to the removals at lines 120 and 161.


await Task.Delay(500);

// Verify the CollectionView is still in a consistent state
Assert.Equal(2, groupData.Count);
});
}

// Regression test for https://github.com/dotnet/maui/issues/34691
[Fact(DisplayName = "Removing first section from grouped CollectionView does not crash")]
public async Task ItemsSourceGroupedRemoveFirstSectionDoesNotCrash()
{
SetupBuilder();

var data = new List<string> { "item 1", "item 2" };
var groupData = new ObservableCollection<CollectionViewStringGroup>
{
new("Header 1", data),
new("Header 2", data),
new("Header 3", data),
};

var collectionView = new CollectionView
{
IsGrouped = true,
ItemsSource = groupData,
ItemTemplate = new DataTemplate(() => new Label()),
};

await CreateHandlerAndAddToWindow<CollectionViewHandler>(collectionView, async handler =>
{
await Task.Delay(1000);

groupData.RemoveAt(0);
await Task.Delay(500);
groupData.RemoveAt(0);

Assert.Equal(1, groupData.Count);
});
}

// Regression test for https://github.com/dotnet/maui/issues/34691
[Fact(DisplayName = "Removing sections one by one from grouped CollectionView does not crash")]
public async Task ItemsSourceGroupedRemoveAllSectionsOneByOneDoesNotCrash()
{
SetupBuilder();

var data = new List<string> { "item 1", "item 2" };
var groupData = new ObservableCollection<CollectionViewStringGroup>
{
new("Header 1", data),
new("Header 2", data),
new("Header 3", data),
};

var collectionView = new CollectionView
{
IsGrouped = true,
ItemsSource = groupData,
ItemTemplate = new DataTemplate(() => new Label()),
};

await CreateHandlerAndAddToWindow<CollectionViewHandler>(collectionView, async handler =>
{
await Task.Delay(1000);

while (groupData.Count > 0)
{
groupData.RemoveAt(groupData.Count - 1);
await Task.Delay(300);
}

Assert.Empty(groupData);
});
}

class CollectionViewStringGroup : List<string>
{
public string GroupHeader { get; private set; }
Expand Down
Loading