Merged PR 34696: [8.0] Prevent delivery of events to disposed components (second attempt) (#52057)

Prevents delivery of events to disposed components.

Co-authored-by: Steve Sanderson <stevesa@microsoft.com>

Author: wtgodbe

src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs

@@ -978,7 +978,7 @@ private static void InitializeNewAttributeFrame(ref DiffContext diffContext, ref
             newFrame.AttributeNameField.Length >= 3 &&
             newFrame.AttributeNameField.StartsWith("on", StringComparison.Ordinal))
         {
-            diffContext.Renderer.AssignEventHandlerId(ref newFrame);
+            diffContext.Renderer.AssignEventHandlerId(diffContext.ComponentId, ref newFrame);
         }
     }
 

src/Components/Components/src/RenderTree/Renderer.Log.cs

@@ -63,5 +63,16 @@ public static void HandlingEvent(ILogger logger, ulong eventHandlerId, EventArgs
                 HandlingEvent(logger, eventHandlerId, eventArgs?.GetType().Name ?? "null");
             }
         }
+
+        [LoggerMessage(6, LogLevel.Debug, "Skipping attempt to raise event {EventId} of type '{EventType}' because the component ID {ComponentId} was already disposed", EventName = "SkippingEventOnDisposedComponent", SkipEnabledCheck = true)]
+        public static partial void SkippingEventOnDisposedComponent(ILogger logger, int componentId, ulong eventId, string eventType);
+
+        public static void SkippingEventOnDisposedComponent(ILogger logger, int componentId, ulong eventHandlerId, EventArgs? eventArgs)
+        {
+            if (logger.IsEnabled(LogLevel.Debug)) // This is almost always false, so skip the evaluations
+            {
+                SkippingEventOnDisposedComponent(logger, componentId, eventHandlerId, eventArgs?.GetType().Name ?? "null");
+            }
+        }
     }
 }

src/Components/Components/src/RenderTree/Renderer.cs

@@ -28,7 +28,7 @@ public abstract partial class Renderer : IDisposable, IAsyncDisposable
     private readonly Dictionary<int, ComponentState> _componentStateById = new Dictionary<int, ComponentState>();
     private readonly Dictionary<IComponent, ComponentState> _componentStateByComponent = new Dictionary<IComponent, ComponentState>();
     private readonly RenderBatchBuilder _batchBuilder = new RenderBatchBuilder();
-    private readonly Dictionary<ulong, EventCallback> _eventBindings = new Dictionary<ulong, EventCallback>();
+    private readonly Dictionary<ulong, (int RenderedByComponentId, EventCallback Callback)> _eventBindings = new();
     private readonly Dictionary<ulong, ulong> _eventHandlerIdReplacements = new Dictionary<ulong, ulong>();
     private readonly ILogger _logger;
     private readonly ComponentFactory _componentFactory;
@@ -416,7 +416,22 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie
             _pendingTasks ??= new();
         }
 
-        var callback = GetRequiredEventCallback(eventHandlerId);
+        var (renderedByComponentId, callback) = GetRequiredEventBindingEntry(eventHandlerId);
+
+        // If this event attribute was rendered by a component that's since been disposed, don't dispatch the event at all.
+        // This can occur because event handler disposal is deferred, so event handler IDs can outlive their components.
+        // The reason the following check is based on "which component rendered this frame" and not on "which component
+        // receives the callback" (i.e., callback.Receiver) is that if parent A passes a RenderFragment with events to child B,
+        // and then child B is disposed, we don't want to dispatch the events (because the developer considers them removed
+        // from the UI) even though the receiver A is still alive.
+        if (!_componentStateById.ContainsKey(renderedByComponentId))
+        {
+            // This is not an error since it can happen legitimately (in Blazor Server, the user might click a button at the same
+            // moment that the component is disposed remotely, and then the click event will arrive after disposal).
+            Log.SkippingEventOnDisposedComponent(_logger, renderedByComponentId, eventHandlerId, eventArgs);
+            return Task.CompletedTask;
+        }
+
         Log.HandlingEvent(_logger, eventHandlerId, eventArgs);
 
         // Try to match it up with a receiver so that, if the event handler later throws, we can route the error to the
@@ -480,7 +495,7 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie
     /// <returns>The parameter type expected by the event handler. Normally this is a subclass of <see cref="EventArgs"/>.</returns>
     public Type GetEventArgsType(ulong eventHandlerId)
     {
-        var methodInfo = GetRequiredEventCallback(eventHandlerId).Delegate?.Method;
+        var methodInfo = GetRequiredEventBindingEntry(eventHandlerId).Callback.Delegate?.Method;
 
         // The DispatchEventAsync code paths allow for the case where Delegate or its method
         // is null, and in this case the event receiver just receives null. This won't happen
@@ -581,7 +596,7 @@ protected virtual void AddPendingTask(ComponentState? componentState, Task task)
         _pendingTasks?.Add(task);
     }
 
-    internal void AssignEventHandlerId(ref RenderTreeFrame frame)
+    internal void AssignEventHandlerId(int renderedByComponentId, ref RenderTreeFrame frame)
     {
         var id = ++_lastEventHandlerId;
 
@@ -593,15 +608,15 @@ internal void AssignEventHandlerId(ref RenderTreeFrame frame)
             //
             // When that happens we intentionally box the EventCallback because we need to hold on to
             // the receiver.
-            _eventBindings.Add(id, callback);
+            _eventBindings.Add(id, (renderedByComponentId, callback));
         }
         else if (frame.AttributeValueField is MulticastDelegate @delegate)
         {
             // This is the common case for a delegate, where the receiver of the event
             // is the same as delegate.Target. In this case since the receiver is implicit we can
             // avoid boxing the EventCallback object and just re-hydrate it on the other side of the
             // render tree.
-            _eventBindings.Add(id, new EventCallback(@delegate.Target as IHandleEvent, @delegate));
+            _eventBindings.Add(id, (renderedByComponentId, new EventCallback(@delegate.Target as IHandleEvent, @delegate)));
         }
 
         // NOTE: we do not to handle EventCallback<T> here. EventCallback<T> is only used when passing
@@ -645,14 +660,14 @@ internal void TrackReplacedEventHandlerId(ulong oldEventHandlerId, ulong newEven
         _eventHandlerIdReplacements.Add(oldEventHandlerId, newEventHandlerId);
     }
 
-    private EventCallback GetRequiredEventCallback(ulong eventHandlerId)
+    private (int RenderedByComponentId, EventCallback Callback) GetRequiredEventBindingEntry(ulong eventHandlerId)
     {
-        if (!_eventBindings.TryGetValue(eventHandlerId, out var callback))
+        if (!_eventBindings.TryGetValue(eventHandlerId, out var entry))
         {
             throw new ArgumentException($"There is no event handler associated with this event. EventId: '{eventHandlerId}'.", nameof(eventHandlerId));
         }
 
-        return callback;
+        return entry;
     }
 
     private ulong FindLatestEventHandlerIdInChain(ulong eventHandlerId)

src/Components/Components/test/RendererTest.cs

@@ -2619,6 +2619,56 @@ public void RenderBatch_DoesNotDisposeComponentMultipleTimes()
         Assert.True(component.Disposed);
     }
 
+    [Fact]
+    public async Task DoesNotDispatchEventsAfterOwnerComponentIsDisposed()
+    {
+        // Arrange
+        var renderer = new TestRenderer();
+        var eventCount = 0;
+        Action<EventArgs> origEventHandler = args => { eventCount++; };
+        var component = new ConditionalParentComponent<EventComponent>
+        {
+            IncludeChild = true,
+            ChildParameters = new Dictionary<string, object>
+            {
+                { nameof(EventComponent.OnTest), origEventHandler }
+            }
+        };
+        var rootComponentId = renderer.AssignRootComponentId(component);
+        component.TriggerRender();
+        var batch = renderer.Batches.Single();
+        var rootComponentDiff = batch.DiffsByComponentId[rootComponentId].Single();
+        var rootComponentFrame = batch.ReferenceFrames[0];
+        var childComponentFrame = rootComponentDiff.Edits
+            .Select(e => batch.ReferenceFrames[e.ReferenceFrameIndex])
+            .Where(f => f.FrameType == RenderTreeFrameType.Component)
+            .Single();
+        var childComponentId = childComponentFrame.ComponentId;
+        var childComponentDiff = batch.DiffsByComponentId[childComponentFrame.ComponentId].Single();
+        var eventHandlerId = batch.ReferenceFrames
+            .Skip(childComponentDiff.Edits[0].ReferenceFrameIndex) // Search from where the child component frames start
+            .Where(f => f.FrameType == RenderTreeFrameType.Attribute)
+            .Single(f => f.AttributeEventHandlerId != 0)
+            .AttributeEventHandlerId;
+
+        // Act/Assert 1: Event handler fires when we trigger it
+        Assert.Equal(0, eventCount);
+        var renderTask = renderer.DispatchEventAsync(eventHandlerId, args: null);
+        Assert.True(renderTask.IsCompletedSuccessfully);
+        Assert.Equal(1, eventCount);
+        await renderTask;
+
+        // Now remove the EventComponent, but without ever acknowledging the renderbatch, so the event handler doesn't get disposed
+        var disposalBatchAcknowledgementTcs = new TaskCompletionSource();
+        component.IncludeChild = false;
+        renderer.NextRenderResultTask = disposalBatchAcknowledgementTcs.Task;
+        component.TriggerRender();
+
+        // Act/Assert 2: Can no longer fire the original event. It's not an error but the delegate was not invoked.
+        await renderer.DispatchEventAsync(eventHandlerId, args: null);
+        Assert.Equal(1, eventCount);
+    }
+
     [Fact]
     public async Task DisposesEventHandlersWhenAttributeValueChanged()
     {

src/Components/test/E2ETest/Tests/FormsTest.cs

@@ -990,6 +990,39 @@ public void CanHaveModelLevelValidationErrors()
         Browser.Collection(logEntries, x => Assert.Equal("OnValidSubmit", x));
     }
 
+    [Fact]
+    public async Task CannotSubmitEditFormSynchronouslyAfterItWasRemoved()
+    {
+        var appElement = MountSimpleValidationComponent();
+
+        var submitButtonFinder = By.CssSelector("button[type=submit]");
+        Browser.Exists(submitButtonFinder);
+
+        // Remove the form then immediately also submit it, so the server receives both
+        // the 'remove' and 'submit' commands (in that order) before it updates the UI
+        appElement.FindElement(By.Id("remove-form")).Click();
+
+        try
+        {
+            appElement.FindElement(submitButtonFinder).Click();
+        }
+        catch (NoSuchElementException)
+        {
+            // This should happen on WebAssembly because the form will be removed synchronously
+            // That means the test has passed
+            return;
+        }
+
+        // Wait for the removal to complete, which is intentionally delayed to ensure
+        // this test can submit a second instruction before the first is processed.
+        Browser.DoesNotExist(submitButtonFinder);
+
+        // Verify that the form submit event was not processed, even if we wait a while
+        // to be really sure the second instruction was processed.
+        await Task.Delay(1000);
+        Browser.DoesNotExist(By.Id("last-callback"));
+    }
+
     private Func<string[]> CreateValidationMessagesAccessor(IWebElement appElement, string messageSelector = ".validation-message")
     {
         return () => appElement.FindElements(By.CssSelector(messageSelector))

src/Components/test/testassets/BasicTestApp/FormsTest/SimpleValidationComponent.razor

@@ -1,37 +1,42 @@
 @using System.ComponentModel.DataAnnotations
 @using Microsoft.AspNetCore.Components.Forms
 
-<EditForm Model="@this" OnValidSubmit="@HandleValidSubmit" OnInvalidSubmit="@HandleInvalidSubmit" autocomplete="off">
-    <DataAnnotationsValidator />
-
-    <p class="user-name">
-        User name: <input @bind="UserName" class="@context.FieldCssClass(() => UserName)" />
-    </p>
-    <p class="accepts-terms">
-        Accept terms: <input type="checkbox" @bind="AcceptsTerms" class="@context.FieldCssClass(() => AcceptsTerms)" />
-    </p>
-
-    <button type="submit">Submit</button>
-
-    @* Could use <ValidationSummary /> instead, but this shows it can be done manually *@
-    <ul class="validation-errors">
-        @foreach (var message in context.GetValidationMessages())
-        {
-            <li class="validation-message">@message</li>
-        }
-    </ul>
-
-</EditForm>
+@if (!removeForm)
+{
+    <EditForm Model="@this" OnValidSubmit="@HandleValidSubmit" OnInvalidSubmit="@HandleInvalidSubmit" autocomplete="off">
+        <DataAnnotationsValidator />
+
+        <p class="user-name">
+            User name: <input @bind="UserName" class="@context.FieldCssClass(() => UserName)" />
+        </p>
+        <p class="accepts-terms">
+            Accept terms: <input type="checkbox" @bind="AcceptsTerms" class="@context.FieldCssClass(() => AcceptsTerms)" />
+        </p>
+
+        <button type="submit">Submit</button>
+
+        @* Could use <ValidationSummary /> instead, but this shows it can be done manually *@
+        <ul class="validation-errors">
+            @foreach (var message in context.GetValidationMessages())
+            {
+                <li class="validation-message">@message</li>
+            }
+        </ul>
+    </EditForm>
+}
 
 @if (lastCallback != null)
 {
     <span id="last-callback">@lastCallback</span>
 }
 
+<p><button id="remove-form" @onclick="RemoveForm">Remove form</button></p>
+
 @code {
     protected virtual bool UseExperimentalValidator => false;
 
     string lastCallback;
+    bool removeForm;
 
     [Required(ErrorMessage = "Please choose a username")]
     public string UserName { get; set; }
@@ -49,4 +54,10 @@
     {
         lastCallback = "OnInvalidSubmit";
     }
+
+    void RemoveForm()
+    {
+        removeForm = true;
+        Thread.Sleep(1000); // To ensure we can dispatch another event before this completes
+    }
 }