WeakEvent subscription management thread race condition fix (#19383)

* Fixed thread-race condition when INotifyCollectionChanged event fired on non-ui thread

* Fixed WeakEvent thread-race condition (NullReferenceException) when event fired on non-ui thread

* Limited delegates creation in WeakEvent

* Avoid casting in WeakEvent

src/Avalonia.Base/Utilities/WeakEvent.cs

@@ -1,5 +1,7 @@
 using System;
 using System.Runtime.CompilerServices;
+using System.Threading;
+using Avalonia.Collections.Pooled;
 using Avalonia.Threading;
 
 namespace Avalonia.Utilities;
@@ -10,8 +12,8 @@ namespace Avalonia.Utilities;
 public sealed class WeakEvent<TSender, TEventArgs> : WeakEvent where TSender : class
 {
     private readonly Func<TSender, EventHandler<TEventArgs>, Action> _subscribe;
-
+    private readonly ConditionalWeakTable<TSender, Subscription> _subscriptions = new();
-    private readonly ConditionalWeakTable<object, Subscription> _subscriptions = new();
+    private readonly ConditionalWeakTable<TSender, Subscription>.CreateValueCallback _createSubscription;
 
     internal WeakEvent(
         Action<TSender, EventHandler<TEventArgs>> subscribe,
@@ -22,33 +24,43 @@ public sealed class WeakEvent<TSender, TEventArgs> : WeakEvent where TSender : c
             subscribe(t, s);
             return () => unsubscribe(t, s);
         };
+        _createSubscription = CreateSubscription;
     }
-    
+
     internal WeakEvent(Func<TSender, EventHandler<TEventArgs>, Action> subscribe)
     {
         _subscribe = subscribe;
+        _createSubscription = CreateSubscription;
     }
-    
+
     public void Subscribe(TSender target, IWeakEventSubscriber<TEventArgs> subscriber)
     {
-        if (!_subscriptions.TryGetValue(target, out var subscription))
+        var spinWait = default(SpinWait);
-            _subscriptions.Add(target, subscription = new Subscription(this, target));
+        while (true)
-        subscription.Add(subscriber);
+        {
+            var subscription = _subscriptions.GetValue(target, _createSubscription);
+            if (subscription.Add(subscriber))
+                break;
+            spinWait.SpinOnce();
+        }
     }
 
     public void Unsubscribe(TSender target, IWeakEventSubscriber<TEventArgs> subscriber)
     {
-        if (_subscriptions.TryGetValue(target, out var subscription)) 
+        if (_subscriptions.TryGetValue(target, out var subscription))
             subscription.Remove(subscriber);
     }
 
+    private Subscription CreateSubscription(TSender key) => new(this, key);
+
     private sealed class Subscription
     {
         private readonly WeakEvent<TSender, TEventArgs> _ev;
         private readonly TSender _target;
         private readonly Action _compact;
-        private readonly Action _unsubscribe;
         private readonly WeakHashList<IWeakEventSubscriber<TEventArgs>> _list = new();
+        private readonly object _lock = new();
+        private Action? _unsubscribe;
         private bool _compactScheduled;
         private bool _destroyed;
 
@@ -57,7 +69,6 @@ public sealed class WeakEvent<TSender, TEventArgs> : WeakEvent where TSender : c
             _ev = ev;
             _target = target;
             _compact = Compact;
-            _unsubscribe = ev._subscribe(target, OnEvent);
         }
 
         private void Destroy()
@@ -65,19 +76,42 @@ public sealed class WeakEvent<TSender, TEventArgs> : WeakEvent where TSender : c
             if(_destroyed)
                 return;
             _destroyed = true;
-            _unsubscribe();
+            _unsubscribe?.Invoke();
             _ev._subscriptions.Remove(_target);
         }
 
-        public void Add(IWeakEventSubscriber<TEventArgs> s) => _list.Add(s);
+        public bool Add(IWeakEventSubscriber<TEventArgs> s)
+        {
+            if (_destroyed)
+                return false;
+
+            lock (_lock)
+            {
+                if (_destroyed)
+                    return false;
+
+                _unsubscribe ??= _ev._subscribe(_target, OnEvent);
+                _list.Add(s);
+                return true;
+            }
+        }
 
         public void Remove(IWeakEventSubscriber<TEventArgs> s)
         {
-            _list.Remove(s);
+            if (_destroyed)
-            if(_list.IsEmpty)
+                return;
-                Destroy();
+
-            else if(_list.NeedCompact && _compactScheduled)
+            lock (_lock)
-                ScheduleCompact();
+            {
+                if (_destroyed)
+                    return;
+
+                _list.Remove(s);
+                if(_list.IsEmpty)
+                    Destroy();
+                else if(_list.NeedCompact && _compactScheduled)
+                    ScheduleCompact();
+            }
         }
 
         private void ScheduleCompact()
@@ -90,23 +124,40 @@ public sealed class WeakEvent<TSender, TEventArgs> : WeakEvent where TSender : c
 
         private void Compact()
         {
-            if(!_compactScheduled)
+            if (_destroyed)
                 return;
-            _compactScheduled = false;
+
-            _list.Compact();
+            lock (_lock)
-            if (_list.IsEmpty)
+            {
-                Destroy();
+                if (_destroyed)
+                    return;
+                if(!_compactScheduled)
+                    return;
+                _compactScheduled = false;
+                _list.Compact();
+                if (_list.IsEmpty)
+                    Destroy();
+            }
         }
 
         private void OnEvent(object? sender, TEventArgs eventArgs)
         {
-            var alive = _list.GetAlive();
+            PooledList<IWeakEventSubscriber<TEventArgs>>? alive;
-            if(alive == null)
+            lock (_lock)
-                Destroy();
+            {
-            else
+                alive = _list.GetAlive();
+                if (alive == null)
+                {
+                    Destroy();
+                    return;
+                }
+            }
+
+            foreach(var item in alive.Span)
+                item.OnEvent(_target, _ev, eventArgs);
+
+            lock (_lock)
             {
-                foreach(var item in alive.Span)
-                    item.OnEvent(_target, _ev, eventArgs);
                 WeakHashList<IWeakEventSubscriber<TEventArgs>>.ReturnToSharedPool(alive);
                 if(_list.NeedCompact && !_compactScheduled)
                     ScheduleCompact();
@@ -124,13 +175,13 @@ public class WeakEvent
     {
         return new WeakEvent<TSender, TEventArgs>(subscribe, unsubscribe);
     }
-    
+
     public static WeakEvent<TSender, TEventArgs> Register<TSender, TEventArgs>(
         Func<TSender, EventHandler<TEventArgs>, Action> subscribe) where TSender : class where TEventArgs : EventArgs
     {
         return new WeakEvent<TSender, TEventArgs>(subscribe);
     }
-    
+
     public static WeakEvent<TSender, EventArgs> Register<TSender>(
         Action<TSender, EventHandler> subscribe,
         Action<TSender, EventHandler> unsubscribe) where TSender : class

src/Avalonia.Controls/Utils/CollectionChangedEventManager.cs

@@ -132,18 +132,22 @@ namespace Avalonia.Controls.Utils
                     }
                 }
 
-                var l = Listeners.ToArray();
-
                 if (Dispatcher.UIThread.CheckAccess())
                 {
+                    var l = Listeners.ToArray();
                     Notify(_collection, e, l);
                 }
                 else
                 {
                     var eCapture = e;
-                    Dispatcher.UIThread.Post(() => Notify(_collection, eCapture, l), DispatcherPriority.Send);
+                    Dispatcher.UIThread.Post(() =>
+                        {
+                            var l = Listeners.ToArray();
+                            Notify(_collection, eCapture, l);
+                        },
+                        DispatcherPriority.Send);
                 }
             }
         }
     }
-}
+}