[CollectionView] Fixed crash modifying an ObservableCollection from non UI thread (#8568)

jsuarezruiz · samhouts · commit 50612ef5944f · 2019-12-16T16:13:41.000-08:00
* Fixed crash modifying an ObservableCollection from non UI thread * Fixed mistake on UWP * Replaced SynchronizationContext by Device.BeginInvokeOnMainThread * Removed unnecessary namespaces * Check IsInvokeRequired in CollectionChanged fixes #8392
diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8392.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8392.cs
@@ -0,0 +1,142 @@
+﻿using System.Collections.ObjectModel;
+using System.Threading.Tasks;
+using System.Windows.Input;
+using Xamarin.Forms.CustomAttributes;
+using Xamarin.Forms.Internals;
+
+#if UITEST
+using Xamarin.Forms.Core.UITests;
+using Xamarin.UITest;
+using NUnit.Framework;
+#endif
+
+namespace Xamarin.Forms.Controls.Issues
+{
+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.Bugzilla, 8392, "[Bug] CollectionView does not marshal ObservableCollection.CollectionChanged to MainThread")]
+	public class Issue8392 : TestContentPage
+	{
+		public Issue8392()
+		{
+			Title = "Issue 8392";
+
+			var collectionView = new CollectionView
+			{
+				ItemTemplate = new Issue8392DataTemplate()
+			};
+
+			collectionView.SetBinding(ItemsView.ItemsSourceProperty, nameof(Issue8392ViewModel.DataList));
+
+			var refreshView = new RefreshView
+			{
+				Content = collectionView
+			};
+
+			refreshView.SetBinding(RefreshView.IsRefreshingProperty, nameof(Issue8392ViewModel.IsRefreshing));
+			refreshView.SetBinding(RefreshView.CommandProperty, nameof(Issue8392ViewModel.PullToRefreshCommand));
+
+			Content = refreshView;
+
+			BindingContext = new Issue8392ViewModel();
+		}
+
+		protected override void Init()
+		{
+
+		}
+
+		protected override void OnAppearing()
+		{
+			base.OnAppearing();
+
+			if (Content is RefreshView refreshView
+				&& refreshView.Content is CollectionView)
+			{
+				refreshView.IsRefreshing = true;
+			}
+		}
+	}
+
+	[Preserve(AllMembers = true)]
+	public class Issue8392DataTemplate : DataTemplate
+	{
+		public Issue8392DataTemplate() : base(CreateTemplate)
+		{
+		}
+
+		static Layout CreateTemplate()
+		{
+			var numberLabel = new Label
+			{
+				FontSize = 14,
+				HorizontalOptions = LayoutOptions.Start,
+				VerticalOptions = LayoutOptions.Center,
+				HorizontalTextAlignment = TextAlignment.Start,
+				VerticalTextAlignment = TextAlignment.Center,
+			};
+			numberLabel.SetBinding(Label.TextProperty, nameof(Issue8392Model.Number));
+
+			var grid = new Grid
+			{
+				RowDefinitions =
+				{
+					new RowDefinition { Height = new GridLength(20,GridUnitType.Absolute) }
+				},
+				ColumnDefinitions =
+				{
+					new ColumnDefinition { Width = new GridLength(1, GridUnitType.Star) }
+				}
+			};
+
+			grid.Children.Add(numberLabel, 0, 0);
+
+			return grid;
+		}
+	}
+
+	public class Issue8392Model
+	{
+		public Issue8392Model(int number) => Number = number;
+
+		public int Number { get; }
+	}
+
+	[Preserve(AllMembers = true)]
+	public class Issue8392ViewModel : BindableObject
+	{
+		bool _isRefreshing;
+
+		public ICommand PullToRefreshCommand => new Command(async () => await ExecutePullToRefreshCommand());
+
+		public bool IsRefreshing
+		{
+			get { return _isRefreshing; }
+			set
+			{
+				_isRefreshing = value;
+				OnPropertyChanged();
+			}
+		}
+
+		public ObservableCollection<Issue8392Model> DataList { get; } = new ObservableCollection<Issue8392Model>();
+
+		async Task ExecutePullToRefreshCommand()
+		{
+			DataList.Clear();
+
+			try
+			{
+				// If the items load without exception, the test has passed.
+				for (int i = 0; i < 30; i++)
+				{
+					await Task.Delay(200).ConfigureAwait(false);
+					DataList.Add(new Issue8392Model(i + 1));
+				}
+			}
+			finally
+			{
+				IsRefreshing = false;
+			}
+		}
+	}
+}
diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
@@ -1143,6 +1143,7 @@
     </Compile>
     <Compile Include="$(MSBuildThisFileDirectory)Issue7898.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue7510.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Issue8392.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue8672.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue8326.xaml.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Issue8449.xaml.cs" />
diff --git a/Xamarin.Forms.Platform.Android/CollectionView/ObservableGroupedSource.cs b/Xamarin.Forms.Platform.Android/CollectionView/ObservableGroupedSource.cs
@@ -10,11 +10,10 @@ internal class ObservableGroupedSource : IGroupableItemsViewSource, ICollectionC
 		readonly ICollectionChangedNotifier _notifier;
 		readonly IList _groupSource;
 		List<IItemsViewSource> _groups = new List<IItemsViewSource>();
+		readonly bool _hasGroupHeaders;
+		readonly bool _hasGroupFooters;
 		bool _disposed;
 
-		bool _hasGroupHeaders;
-		bool _hasGroupFooters;
-
 		public int Count
 		{
 			get
@@ -42,14 +41,14 @@ public ObservableGroupedSource(GroupableItemsView groupableItemsView, ICollectio
 			_notifier = notifier;
 			_groupSource = groupSource as IList ?? new ListSource(groupSource);
 
+			_hasGroupFooters = groupableItemsView.GroupFooterTemplate != null;
+			_hasGroupHeaders = groupableItemsView.GroupHeaderTemplate != null;
+
 			if (_groupSource is INotifyCollectionChanged incc)
 			{
 				incc.CollectionChanged += CollectionChanged;
 			}
 
-			_hasGroupFooters = groupableItemsView.GroupFooterTemplate != null;
-			_hasGroupHeaders = groupableItemsView.GroupHeaderTemplate != null;
-
 			UpdateGroupTracking();
 		}
 
@@ -231,6 +230,18 @@ void ClearGroupTracking()
 		}
 
 		void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
+		{
+			if (Device.IsInvokeRequired)
+			{
+				Device.BeginInvokeOnMainThread(() => CollectionChanged(args));
+			}
+			else
+			{
+				CollectionChanged(args);
+			}
+		}
+
+		void CollectionChanged(NotifyCollectionChangedEventArgs args)
 		{
 			switch (args.Action)
 			{
diff --git a/Xamarin.Forms.Platform.Android/CollectionView/ObservableItemsSource.cs b/Xamarin.Forms.Platform.Android/CollectionView/ObservableItemsSource.cs
@@ -14,6 +14,7 @@ public ObservableItemsSource(IList itemSource, ICollectionChangedNotifier notifi
 		{
 			_itemsSource = itemSource;
 			_notifier = notifier;
+
 			((INotifyCollectionChanged)itemSource).CollectionChanged += CollectionChanged;
 		}
 
@@ -81,6 +82,18 @@ int AdjustPositionForHeader(int position)
 		}
 
 		void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
+		{
+			if (Device.IsInvokeRequired)
+			{
+				Device.BeginInvokeOnMainThread(() => CollectionChanged(args));
+			}
+			else
+			{
+				CollectionChanged(args);
+			}
+		}
+
+		void CollectionChanged(NotifyCollectionChangedEventArgs args)
 		{
 			switch (args.Action)
 			{
diff --git a/Xamarin.Forms.Platform.UAP/CollectionView/GroupedItemTemplateCollection.cs b/Xamarin.Forms.Platform.UAP/CollectionView/GroupedItemTemplateCollection.cs
@@ -1,6 +1,7 @@
 ﻿using System.Collections;
 using System.Collections.ObjectModel;
 using System.Collections.Specialized;
+using System.Threading;
 
 namespace Xamarin.Forms.Platform.UWP
 {
@@ -50,6 +51,18 @@ GroupTemplateContext CreateGroupTemplateContext(object group)
 		}
 
 		void GroupsChanged(object sender, NotifyCollectionChangedEventArgs args)
+		{
+			if (Device.IsInvokeRequired)
+			{
+				Device.BeginInvokeOnMainThread(() => GroupsChanged(args));
+			}
+			else
+			{
+				GroupsChanged(args);
+			}
+		}
+
+		void GroupsChanged(NotifyCollectionChangedEventArgs args)
 		{
 			switch (args.Action)
 			{
diff --git a/Xamarin.Forms.Platform.UAP/CollectionView/ObservableItemTemplateCollection.cs b/Xamarin.Forms.Platform.UAP/CollectionView/ObservableItemTemplateCollection.cs
@@ -2,6 +2,7 @@
 using System.Collections;
 using System.Collections.ObjectModel;
 using System.Collections.Specialized;
+using System.Threading;
 
 namespace Xamarin.Forms.Platform.UWP
 {
@@ -55,6 +56,18 @@ public void CleanUp()
 		}
 
 		void InnerCollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
+		{
+			if (Device.IsInvokeRequired)
+			{
+				Device.BeginInvokeOnMainThread(() => InnerCollectionChanged(args));
+			}
+			else
+			{
+				InnerCollectionChanged(args);
+			}
+		}
+
+		void InnerCollectionChanged(NotifyCollectionChangedEventArgs args)
 		{
 			switch (args.Action)
 			{
diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableGroupedSource.cs
@@ -4,14 +4,13 @@
 using System.Collections.Specialized;
 using Foundation;
 using UIKit;
-using Xamarin.Forms.Internals;
 
 namespace Xamarin.Forms.Platform.iOS
 {
 	internal class ObservableGroupedSource : IItemsViewSource
 	{
 		readonly UICollectionView _collectionView;
-		UICollectionViewController _collectionViewController;
+		readonly UICollectionViewController _collectionViewController;
 		readonly IList _groupSource;
 		bool _disposed;
 		List<ObservableItemsSource> _groups = new List<ObservableItemsSource>();
@@ -129,6 +128,18 @@ void ResetGroupTracking()
 		}
 
 		void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
+		{
+			if (Device.IsInvokeRequired)
+			{
+				Device.BeginInvokeOnMainThread(() => CollectionChanged(args));
+			}
+			else
+			{
+				CollectionChanged(args);
+			}
+		}
+
+		void CollectionChanged(NotifyCollectionChangedEventArgs args)
 		{
 			switch (args.Action)
 			{
diff --git a/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs b/Xamarin.Forms.Platform.iOS/CollectionView/ObservableItemsSource.cs
@@ -93,6 +93,18 @@ public object this[NSIndexPath indexPath]
 		}
 
 		void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
+		{
+			if (Device.IsInvokeRequired)
+			{
+				Device.BeginInvokeOnMainThread(() => CollectionChanged(args));
+			}
+			else
+			{
+				CollectionChanged(args);
+			}
+		}
+
+		void CollectionChanged(NotifyCollectionChangedEventArgs args)
 		{
 			switch (args.Action)
 			{

Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,7 @@ public ObservableItemsSource(IList itemSource, ICollectionChangedNotifier notifi`
`14`	`14`	`{`
`15`	`15`	`_itemsSource = itemSource;`
`16`	`16`	`_notifier = notifier;`
	`17`	`+`
`17`	`18`	`((INotifyCollectionChanged)itemSource).CollectionChanged += CollectionChanged;`
`18`	`19`	`}`
`19`	`20`
`@@ -81,6 +82,18 @@ int AdjustPositionForHeader(int position)`
`81`	`82`	`}`
`82`	`83`
`83`	`84`	`void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)`
	`85`	`+ {`
	`86`	`+ if (Device.IsInvokeRequired)`
	`87`	`+ {`
	`88`	`+ Device.BeginInvokeOnMainThread(() => CollectionChanged(args));`
	`89`	`+ }`
	`90`	`+ else`
	`91`	`+ {`
	`92`	`+ CollectionChanged(args);`
	`93`	`+ }`
	`94`	`+ }`
	`95`	`+`
	`96`	`+ void CollectionChanged(NotifyCollectionChangedEventArgs args)`
`84`	`97`	`{`
`85`	`98`	`switch (args.Action)`
`86`	`99`	`{`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,7 @@`
`1`	`1`	`using System.Collections;`
`2`	`2`	`using System.Collections.ObjectModel;`
`3`	`3`	`using System.Collections.Specialized;`
	`4`	`+using System.Threading;`
`4`	`5`
`5`	`6`	`namespace Xamarin.Forms.Platform.UWP`
`6`	`7`	`{`
`@@ -50,6 +51,18 @@ GroupTemplateContext CreateGroupTemplateContext(object group)`
`50`	`51`	`}`
`51`	`52`
`52`	`53`	`void GroupsChanged(object sender, NotifyCollectionChangedEventArgs args)`
	`54`	`+ {`
	`55`	`+ if (Device.IsInvokeRequired)`
	`56`	`+ {`
	`57`	`+ Device.BeginInvokeOnMainThread(() => GroupsChanged(args));`
	`58`	`+ }`
	`59`	`+ else`
	`60`	`+ {`
	`61`	`+ GroupsChanged(args);`
	`62`	`+ }`
	`63`	`+ }`
	`64`	`+`
	`65`	`+ void GroupsChanged(NotifyCollectionChangedEventArgs args)`
`53`	`66`	`{`
`54`	`67`	`switch (args.Action)`
`55`	`68`	`{`
Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,7 @@`
`2`	`2`	`using System.Collections;`
`3`	`3`	`using System.Collections.ObjectModel;`
`4`	`4`	`using System.Collections.Specialized;`
	`5`	`+using System.Threading;`
`5`	`6`
`6`	`7`	`namespace Xamarin.Forms.Platform.UWP`
`7`	`8`	`{`
`@@ -55,6 +56,18 @@ public void CleanUp()`
`55`	`56`	`}`
`56`	`57`
`57`	`58`	`void InnerCollectionChanged(object sender, NotifyCollectionChangedEventArgs args)`
	`59`	`+ {`
	`60`	`+ if (Device.IsInvokeRequired)`
	`61`	`+ {`
	`62`	`+ Device.BeginInvokeOnMainThread(() => InnerCollectionChanged(args));`
	`63`	`+ }`
	`64`	`+ else`
	`65`	`+ {`
	`66`	`+ InnerCollectionChanged(args);`
	`67`	`+ }`
	`68`	`+ }`
	`69`	`+`
	`70`	`+ void InnerCollectionChanged(NotifyCollectionChangedEventArgs args)`
`58`	`71`	`{`
`59`	`72`	`switch (args.Action)`
`60`	`73`	`{`
Original file line number	Diff line number	Diff line change
`@@ -93,6 +93,18 @@ public object this[NSIndexPath indexPath]`
`93`	`93`	`}`
`94`	`94`
`95`	`95`	`void CollectionChanged(object sender, NotifyCollectionChangedEventArgs args)`
	`96`	`+ {`
	`97`	`+ if (Device.IsInvokeRequired)`
	`98`	`+ {`
	`99`	`+ Device.BeginInvokeOnMainThread(() => CollectionChanged(args));`
	`100`	`+ }`
	`101`	`+ else`
	`102`	`+ {`
	`103`	`+ CollectionChanged(args);`
	`104`	`+ }`
	`105`	`+ }`
	`106`	`+`
	`107`	`+ void CollectionChanged(NotifyCollectionChangedEventArgs args)`
`96`	`108`	`{`
`97`	`109`	`switch (args.Action)`
`98`	`110`	`{`