fix: Fix possible collection changed while enumerating in EVP propaga…

…tion
unoplatform · Feb 17, 2023 · 5bc413a · 5bc413a
1 parent 8b8abd4
commit 5bc413a
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 4 deletions.
diff --git a/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml/Given_FrameworkElement_EffectiveViewport.cs b/src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml/Given_FrameworkElement_EffectiveViewport.cs
@@ -1056,6 +1056,80 @@ await RetryAssert(() =>
 		}
 #endif
 
+		[TestMethod]
+		[RunsOnUIThread]
+		public async Task EVP_When_RemoveHandlerWhileRaisingEvent()
+		{
+			const bool canHorizontallyScroll = true, canVerticallyScroll = true;
+			Border sut;
+			ScrollViewer sv;
+			var root = new Border
+			{
+				HorizontalAlignment = HorizontalAlignment.Left,
+				VerticalAlignment = VerticalAlignment.Top,
+				Child = sv = new ScrollViewer
+				{
+					Width = 512,
+					Height = 512,
+					HorizontalAlignment = HorizontalAlignment.Left,
+					VerticalAlignment = VerticalAlignment.Top,
+					HorizontalScrollBarVisibility = canHorizontallyScroll ? ScrollBarVisibility.Auto : ScrollBarVisibility.Disabled,
+					VerticalScrollBarVisibility = canVerticallyScroll ? ScrollBarVisibility.Auto : ScrollBarVisibility.Disabled,
+					HorizontalScrollMode = canHorizontallyScroll ? ScrollMode.Enabled : ScrollMode.Disabled,
+					VerticalScrollMode = canVerticallyScroll ? ScrollMode.Enabled : ScrollMode.Disabled,
+					Content = new Grid
+					{
+						Height = 1024,
+						Children =
+						{
+							(sut = new Border
+							{
+								HorizontalAlignment = HorizontalAlignment.Left,
+								VerticalAlignment = VerticalAlignment.Top,
+								Background = new SolidColorBrush(Color.FromArgb(0xFF, 0x00, 0x00, 0xF9)),
+								Width = 100,
+								Height = 100,
+								RenderTransform = new CompositeTransform { TranslateX = 50, TranslateY = 25, ScaleY = 2 }
+							})
+						}
+					}
+				}
+			};
+
+			var result = new TaskCompletionSource<object?>();
+			var allowDetach = false;
+			sut.EffectiveViewportChanged += OnSutEVPChanged;
+
+			WindowContent = root;
+
+			// First make sure to be loaded (and actually ignore them)
+			await WaitForIdle();
+			allowDetach = true;
+
+			// Then cause a layout update
+			sv.ChangeView(null, 512, null, disableAnimation: true);
+
+			await result.Task;
+
+			void OnSutEVPChanged(FrameworkElement sender, EffectiveViewportChangedEventArgs args)
+			{
+				try
+				{
+					if (!allowDetach)
+					{
+						return;
+					}
+
+					sut.EffectiveViewportChanged -= OnSutEVPChanged;
+					result.TrySetResult(default);
+				}
+				catch (Exception e)
+				{
+					result.TrySetException(e);
+				}
+			}
+		}
+
 		private async Task RetryAssert(Action assertion)
 		{
 			var attempt = 0;

diff --git a/src/Uno.UI/UI/Xaml/FrameworkElement.EffectiveViewport.cs b/src/Uno.UI/UI/Xaml/FrameworkElement.EffectiveViewport.cs
@@ -43,6 +43,7 @@ partial class FrameworkElement : IFrameworkElement_EffectiveViewport
 		private event TypedEventHandler<_This, EffectiveViewportChangedEventArgs>? _effectiveViewportChanged;
 		private bool _hasNewHandler;
 		private List<IFrameworkElement_EffectiveViewport>? _childrenInterestedInViewportUpdates;
+		private bool _isEnumeratingChildrenInterestedInViewportUpdates;
 		private IDisposable? _parentViewportUpdatesSubscription;
 		private ViewportInfo _parentViewport = ViewportInfo.Empty; // WARNING: Stored in parent's coordinates space, use GetParentViewport()
 		private ViewportInfo _lastEffectiveViewport;
@@ -153,12 +154,21 @@ IDisposable IFrameworkElement_EffectiveViewport.RequestViewportUpdates(bool isIn
 				Uno.UI.Extensions.DependencyObjectExtensions.GetChildren(this).OfType<IFrameworkElement_EffectiveViewport>().Contains(child)
 				|| (child as _View)?.FindFirstAncestor<IFrameworkElement_EffectiveViewport>() == this);
 
-			(_childrenInterestedInViewportUpdates ??= new()).Add(child);
+			var childrenInterestedInViewportUpdates = _childrenInterestedInViewportUpdates switch
+			{
+				null => (_childrenInterestedInViewportUpdates = new()),
+				_ when _isEnumeratingChildrenInterestedInViewportUpdates => (_childrenInterestedInViewportUpdates = new(_childrenInterestedInViewportUpdates)),
+				_ => _childrenInterestedInViewportUpdates,
+			};
+			childrenInterestedInViewportUpdates.Add(child);
 			ReconfigureViewportPropagation(isInternalUpdate, child);
 
 			return Disposable.Create(() =>
 			{
-				_childrenInterestedInViewportUpdates!.Remove(child);
+				var childrenInterestedInViewportUpdates = _isEnumeratingChildrenInterestedInViewportUpdates
+						? (_childrenInterestedInViewportUpdates = new(_childrenInterestedInViewportUpdates))
+						: _childrenInterestedInViewportUpdates!;
+				childrenInterestedInViewportUpdates.Remove(child);
 				ReconfigureViewportPropagation();
 			});
 		}
@@ -351,9 +361,19 @@ private void PropagateEffectiveViewportChange(
 
 			if (_childrenInterestedInViewportUpdates is { Count: > 0 } && (isInitial || viewportUpdated))
 			{
-				foreach (var child in _childrenInterestedInViewportUpdates)
+				_isEnumeratingChildrenInterestedInViewportUpdates = true;
+				var enumerator = _childrenInterestedInViewportUpdates.GetEnumerator();
+				try
+				{
+					while (enumerator.MoveNext())
+					{
+						enumerator.Current!.OnParentViewportChanged(isInitial, isInternal, this, viewport);
+					}
+				}
+				finally
 				{
-					child.OnParentViewportChanged(isInitial, isInternal, this, viewport);
+					_isEnumeratingChildrenInterestedInViewportUpdates = false;
+					enumerator.Dispose();
 				}
 			}
 		}