Bug 56771 - Multi-item add in INotifyCollectionChanged causes a NSInternalInconsistencyException in bindings on iOS
Summary: Multi-item add in INotifyCollectionChanged causes a NSInternalInconsistencyEx...
Status: RESOLVED FIXED
Alias: None
Product: Forms
Classification: Xamarin
Component: Forms ()
Version: 2.3.4
Hardware: All All
: Normal major
Target Milestone: ---
Assignee: Chris King
URL:
Depends on:
Blocks:
 
Reported: 2017-05-23 17:56 UTC by Morten Nielsen
Modified: 2017-05-30 22:37 UTC (History)
5 users (show)

Tags: inotifycollectionchanged ac
Is this bug a regression?: ---
Last known good build:


Attachments
Repro sample (191.88 KB, application/x-zip-compressed)
2017-05-23 17:56 UTC, Morten Nielsen
Details
Fixed reproduction (237.16 KB, application/x-zip-compressed)
2017-05-30 19:02 UTC, Chris King
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Morten Nielsen 2017-05-23 17:56:19 UTC
Created attachment 22395 [details]
Repro sample

When building an optimized ObservableCollection that only raises a single Add event for a multi-item change, the following exception is thrown: 

Foundation.MonoTouchException: Objective-C exception thrown.  Name: NSInternalInconsistencyException Reason: attempt to insert row 2 into section 0, but there are only 2 rows in section 0 after the update

When adding a large amount of items to a list, it's beneficial to only raise a single multi-item change event to avoid too many layout cycles and events, and can be done by subclassing ObservableCollection or implementing INotifyCollectionChanged, and add an "AddRange" method. Once the items has been added to the internal collection, a single event is raised for each property changed like this:

OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, changedItems: items.ToList(), startingIndex: startIndex));
OnPropertyChanged(new PropertyChangedEventArgs(nameof(Count)));
OnPropertyChanged(new PropertyChangedEventArgs("Item[]"));

Note the list of 'changedItems'.

Android handles this ok. Also note that a multi-item remove seems to work ok. 


To reproduce, simply run the attached sample on iOS and Android, and click the "Add" button to add two items. Notice Android handles this ok, but iOS only handles the remove/clear, but will throw the mentioned exception on AddRange.
Comment 1 Morten Nielsen 2017-05-23 18:28:35 UTC
FYI Replace also seems to be handled ok, so it's only "Add". 


//List ReplaceMany implementation 
        public void ReplaceMany(int startIndex, params T[] items)
        {
            if(items.Length > 0)
            {
                List<T> removedItems = new List<T>(items.Length);
                for (int i = 0; i < items.Length; i++)
                {
                    removedItems.Add(base.Items[i + startIndex]);
                    base.Items[i + startIndex] = items[i];
                    OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, oldItems: removedItems,  newItems: items.ToList(), startingIndex: startIndex));
                }
            }
        }

Usage:
 data.ReplaceMany(0, $"Item {++i}", $"Item {++i}");
Comment 2 Morten Nielsen 2017-05-23 18:37:30 UTC
...actually Android is broken too, which might be caused by the same problem.

Steps: 
1. Launch on Android. 
2. Click "Add 2" to add "Item 5" and "Item 6".
3. Click "Remove 2" to remove "Item 1" and "Item 2".
4. Click "Add 2" to add "Item 7" and "Item 8".

Note that 7 and 8 wasn't added, but 5 and 6 was added again. Stepping through the code confirms 7 and 8 was added to the collection, but the layout seems to always be showing two items off after step '3' happened.
Comment 3 Paul DiPietro [MSFT] 2017-05-24 18:40:44 UTC
The reproduction appears to occur for iOS as originally mentioned, despite different steps for Android, so this will be set to confirmed for now until further investigation.
Comment 4 Morten Nielsen 2017-05-24 19:54:29 UTC
@Paul Do you want me to log a separate issue for the Android behavior?
Comment 6 Chris King 2017-05-30 19:02:48 UTC
Created attachment 22562 [details]
Fixed reproduction
Comment 7 Morten Nielsen 2017-05-30 22:37:11 UTC
D'oh! I can't believe I missed that increment bug! My original issue is a lot more advanced and not as obvious, but naturally I thought my simpler repro now was reproducing it successfully.

This led me back to double-check my own implementation, and it seems Xamarin.Forms instantly reacts to remove and add events, whereas WPF/UWP delays the update, and thus any briefly-out-of-sync underlying collection isn't affected, which had led me to believe my original implementation was OK.

I'm so sorry for wasting your time.