Properly check if operation is pending when executing/aborting (#19132)

Co-authored-by: Julien Lebosquain <julien@lebosquain.net>

src/Avalonia.Base/Threading/Dispatcher.Invoke.cs

@@ -445,7 +445,8 @@ public partial class Dispatcher
             // so it is safe to modify the operation outside of the lock.
             // Just mark the operation as aborted, which we can safely
             // return to the user.
-            operation.DoAbort();
+            operation.Status = DispatcherOperationStatus.Aborted;
+            operation.CallAbortCallbacks();
         }
     }
 

src/Avalonia.Base/Threading/Dispatcher.Queue.cs

@@ -134,7 +134,14 @@ public partial class Dispatcher
     private void ExecuteJob(DispatcherOperation job)
     {
         lock (InstanceLock)
+        {
+            if(job.Status != DispatcherOperationStatus.Pending)
+                return;
+            
             _queue.RemoveItem(job);
+            job.Status = DispatcherOperationStatus.Executing;
+        }
+
         job.Execute();
         // The backend might be firing timers with a low priority,
         // so we manually check if our high priority timers are due for execution
@@ -236,11 +243,16 @@ public partial class Dispatcher
         }
     }
 
-    internal void Abort(DispatcherOperation operation)
+    internal bool Abort(DispatcherOperation operation)
     {
         lock (InstanceLock)
+        {
+            if (operation.Status != DispatcherOperationStatus.Pending)
+                return false;
             _queue.RemoveItem(operation);
-        operation.DoAbort();
+            operation.Status = DispatcherOperationStatus.Aborted;
+        }
+        return true;
     }
     
     // Returns whether or not the priority was set.

src/Avalonia.Base/Threading/DispatcherOperation.cs

@@ -11,7 +11,7 @@ namespace Avalonia.Threading;
 public class DispatcherOperation
 {
     protected readonly bool ThrowOnUiThread;
-    public DispatcherOperationStatus Status { get; protected set; }
+    public DispatcherOperationStatus Status { get; internal set; }
     public Dispatcher Dispatcher { get; }
 
     public DispatcherPriority Priority
@@ -115,13 +115,13 @@ public class DispatcherOperation
     
     public bool Abort()
     {
-        lock (Dispatcher.InstanceLock)
+        if (Dispatcher.Abort(this))
         {
-            if (Status != DispatcherOperationStatus.Pending)
+            CallAbortCallbacks();
-                return false;
-            Dispatcher.Abort(this);
             return true;
         }
+
+        return false;
     }
 
     /// <summary>
@@ -254,20 +254,15 @@ public class DispatcherOperation
         return GetTask().GetAwaiter();
     }
 
-    internal void DoAbort()
+    internal void CallAbortCallbacks()
     {
-        Status = DispatcherOperationStatus.Aborted;
         AbortTask();
         _aborted?.Invoke(this, EventArgs.Empty);
     }
     
     internal void Execute()
     {
-        lock (Dispatcher.InstanceLock)
+        Debug.Assert(Status == DispatcherOperationStatus.Executing);
-        {
-            Status = DispatcherOperationStatus.Executing;
-        }
-
         try
         {
             using (AvaloniaSynchronizationContext.Ensure(Dispatcher, Priority))
@@ -311,7 +306,19 @@ public class DispatcherOperation
 
     internal virtual object? GetResult() => null;
     
-    protected virtual void AbortTask() => (TaskSource as TaskCompletionSource<object?>)?.SetCanceled();
+    protected virtual void AbortTask()
+    {
+        object? taskSource;
+        lock (Dispatcher.InstanceLock)
+        {
+            Debug.Assert(Status == DispatcherOperationStatus.Aborted);
+            // There is no way for TaskSource to become not-null after being null with aborted tasks,
+            // so it's safe to save it here and use after exiting the lock
+            taskSource = TaskSource;
+        }
+
+        (taskSource as TaskCompletionSource<object?>)?.SetCanceled();
+    }
 
     private static CancellationToken CreateCancelledToken()
     {