Better handling of render-after-disposed scenarios

src/Microsoft.AspNetCore.Blazor/Components/RenderHandle.cs

@@ -39,7 +39,7 @@ namespace Microsoft.AspNetCore.Blazor.Components
                 throw new InvalidOperationException("The render handle is not yet assigned.");
             }
 
-            _renderer.AddToRenderQueue(new RenderQueueEntry(_componentId, renderFragment));
+            _renderer.AddToRenderQueue(_componentId, renderFragment);
         }
     }
 }

src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs

@@ -19,6 +19,7 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
         private readonly Renderer _renderer;
         private RenderTreeBuilder _renderTreeBuilderCurrent;
         private RenderTreeBuilder _renderTreeBuilderPrevious;
+        private bool _componentWasDisposed;
 
         /// <summary>
         /// Constructs an instance of <see cref="ComponentState"/>.
@@ -37,6 +38,13 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
 
         public void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment renderFragment)
         {
+            // A component might be in the render queue already before getting disposed by an
+            // earlier entry in the render queue. In that case, rendering is a no-op.
+            if (_componentWasDisposed)
+            {
+                return;
+            }
+
             // Swap the old and new tree builders
             (_renderTreeBuilderCurrent, _renderTreeBuilderPrevious) = (_renderTreeBuilderPrevious, _renderTreeBuilderCurrent);
 
@@ -54,6 +62,8 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
 
         public void DisposeInBatch(RenderBatchBuilder batchBuilder)
         {
+            _componentWasDisposed = true;
+ 
             // TODO: Handle components throwing during dispose. Shouldn't break the whole render batch.
             if (_component is IDisposable disposable)
             {

src/Microsoft.AspNetCore.Blazor/Rendering/RenderQueueEntry.cs

@@ -8,12 +8,12 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
 {
     internal readonly struct RenderQueueEntry
     {
-        public readonly int ComponentId;
+        public readonly ComponentState ComponentState;
         public readonly RenderFragment RenderFragment;
 
-        public RenderQueueEntry(int componentId, RenderFragment renderFragment)
+        public RenderQueueEntry(ComponentState componentState, RenderFragment renderFragment)
         {
-            ComponentId = componentId;
+            ComponentState = componentState;
             RenderFragment = renderFragment ?? throw new ArgumentNullException(nameof(renderFragment));
         }
     }

src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs

@@ -71,7 +71,18 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
         {
             if (_eventHandlersById.TryGetValue(eventHandlerId, out var handler))
             {
-                GetRequiredComponentState(componentId).DispatchEvent(handler, eventArgs);
+                // The event handler might request multiple renders in sequence. Capture them
+                // all in a single batch.
+                try
+                {
+                    _isBatchInProgress = true;
+                    GetRequiredComponentState(componentId).DispatchEvent(handler, eventArgs);
+                }
+                finally
+                {
+                    _isBatchInProgress = false;
+                    ProcessRenderQueue();
+                }
             }
             else
             {
@@ -103,9 +114,18 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
             frame = frame.WithAttributeEventHandlerId(id);
         }
 
-        internal void AddToRenderQueue(RenderQueueEntry renderQueueEntry)
+        internal void AddToRenderQueue(int componentId, RenderFragment renderFragment)
         {
-            _batchBuilder.ComponentRenderQueue.Enqueue(renderQueueEntry);
+            var componentState = GetOptionalComponentState(componentId);
+            if (componentState == null)
+            {
+                // If the component was already disposed, then its render handle trying to
+                // queue a render is a no-op.
+                return;
+            }
+
+            _batchBuilder.ComponentRenderQueue.Enqueue(
+                new RenderQueueEntry(componentState, renderFragment));
 
             if (!_isBatchInProgress)
             {
@@ -118,6 +138,11 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
                 ? componentState
                 : throw new ArgumentException($"The renderer does not have a component with ID {componentId}.");
 
+        private ComponentState GetOptionalComponentState(int componentId)
+            => _componentStateById.TryGetValue(componentId, out var componentState)
+                ? componentState
+                : null;
+
         private void ProcessRenderQueue()
         {
             _isBatchInProgress = true;
@@ -143,8 +168,7 @@ namespace Microsoft.AspNetCore.Blazor.Rendering
 
         private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry)
         {
-            var componentId = renderQueueEntry.ComponentId;
+            renderQueueEntry.ComponentState
-            GetRequiredComponentState(componentId)
                 .RenderIntoBatch(_batchBuilder, renderQueueEntry.RenderFragment);
 
             // Process disposal queue now in case it causes further component renders to be enqueued

test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs

@@ -450,14 +450,23 @@ namespace Microsoft.AspNetCore.Blazor.Test
                 .Where(frame => frame.FrameType == RenderTreeFrameType.Component)
                 .Select(frame => frame.ComponentId)
                 .ToList();
+            var childComponent3 = batch.ReferenceFrames.Where(f => f.ComponentId == 3)
+                .Single().Component;
             Assert.Equal(new[] { 1, 2 }, childComponentIds);
+            Assert.IsType<FakeComponent>(childComponent3);
 
             // Act: Second render
             firstRender = false;
             component.TriggerRender();
 
             // Assert: Applicable children are included in disposal list
+            Assert.Equal(2, renderer.Batches.Count);
             Assert.Equal(new[] { 1, 3 }, renderer.Batches[1].DisposedComponentIDs);
+
+            // Act/Assert: If a disposed component requests a render, it's a no-op
+            ((FakeComponent)childComponent3).RenderHandle.Render(builder
+                => throw new NotImplementedException("Should not be invoked"));
+            Assert.Equal(2, renderer.Batches.Count);
         }
 
         [Fact]
@@ -612,6 +621,85 @@ namespace Microsoft.AspNetCore.Blazor.Test
             Assert.Equal(1, eventCount);
         }
 
+        [Fact]
+        public void AllRendersTriggeredSynchronouslyDuringEventHandlerAreHandledAsSingleBatch()
+        {
+            // Arrange: A root component with a child whose event handler explicitly queues
+            // a re-render of both the root component and the child
+            var renderer = new TestRenderer();
+            var eventCount = 0;
+            TestComponent rootComponent = null;
+            EventComponent childComponent = null;
+            rootComponent = new TestComponent(builder =>
+            {
+                builder.AddContent(0, "Child event count: " + eventCount);
+                builder.OpenComponent<EventComponent>(1);
+                builder.AddAttribute(2, nameof(EventComponent.Handler), args =>
+                {
+                    eventCount++;
+                    rootComponent.TriggerRender();
+                    childComponent.TriggerRender();
+                });
+                builder.CloseComponent();
+            });
+            var rootComponentId = renderer.AssignComponentId(rootComponent);
+            rootComponent.TriggerRender();
+            var origBatchReferenceFrames = renderer.Batches.Single().ReferenceFrames;
+            var childComponentFrame = origBatchReferenceFrames
+                .Single(f => f.Component is EventComponent);
+            var childComponentId = childComponentFrame.ComponentId;
+            childComponent = (EventComponent)childComponentFrame.Component;
+            var origEventHandlerId = origBatchReferenceFrames
+                .Where(f => f.FrameType == RenderTreeFrameType.Attribute)
+                .Last(f => f.AttributeEventHandlerId != 0)
+                .AttributeEventHandlerId;
+            Assert.Single(renderer.Batches);
+
+            // Act
+            renderer.DispatchEvent(childComponentId, origEventHandlerId, args: null);
+
+            // Assert
+            Assert.Equal(2, renderer.Batches.Count);
+            var batch = renderer.Batches.Last();
+            Assert.Collection(batch.DiffsInOrder,
+                diff =>
+                {
+                    // First we triggered the root component to re-render
+                    Assert.Equal(rootComponentId, diff.ComponentId);
+                    Assert.Collection(diff.Edits, edit =>
+                    {
+                        Assert.Equal(RenderTreeEditType.UpdateText, edit.Type);
+                        AssertFrame.Text(
+                            batch.ReferenceFrames[edit.ReferenceFrameIndex],
+                            "Child event count: 1");
+                    });
+                },
+                diff =>
+                {
+                    // Then the root re-render will have triggered an update to the child
+                    Assert.Equal(childComponentId, diff.ComponentId);
+                    Assert.Collection(diff.Edits, edit =>
+                    {
+                        Assert.Equal(RenderTreeEditType.UpdateText, edit.Type);
+                        AssertFrame.Text(
+                            batch.ReferenceFrames[edit.ReferenceFrameIndex],
+                            "Render count: 2");
+                    });
+                },
+                diff =>
+                {
+                    // Finally we explicitly requested a re-render of the child
+                    Assert.Equal(childComponentId, diff.ComponentId);
+                    Assert.Collection(diff.Edits, edit =>
+                    {
+                        Assert.Equal(RenderTreeEditType.UpdateText, edit.Type);
+                        AssertFrame.Text(
+                            batch.ReferenceFrames[edit.ReferenceFrameIndex],
+                            "Render count: 3");
+                    });
+                });
+        }
+
         [Fact]
         public void ComponentCannotTriggerRenderBeforeRenderHandleAssigned()
         {
@@ -721,6 +809,60 @@ namespace Microsoft.AspNetCore.Blazor.Test
             Assert.Empty(diff4.Edits);
         }
 
+        [Fact]
+        public void QueuedRenderIsSkippedIfComponentWasAlreadyDisposedInSameBatch()
+        {
+            // Arrange
+            var renderer = new TestRenderer();
+            var shouldRenderChild = true;
+            TestComponent component = null;
+            component = new TestComponent(builder =>
+            {
+                builder.AddContent(0, "Some frame so the child isn't at position zero");
+                if (shouldRenderChild)
+                {
+                    builder.OpenComponent<RendersSelfAfterEventComponent>(1);
+                    builder.AddAttribute(2, nameof(RendersSelfAfterEventComponent.OnClick), (Action)(() =>
+                    {
+                        // First we queue (1) a re-render of the root component, then the child component
+                        // will queue (2) its own re-render. But by the time (1) completes, the child will
+                        // have been disposed, even though (2) is still in the queue
+                        shouldRenderChild = false;
+                        component.TriggerRender();
+                    }));
+                    builder.CloseComponent();
+                }
+            });
+
+            var componentId = renderer.AssignComponentId(component);
+            component.TriggerRender();
+            var childComponentId = renderer.Batches.Single()
+                .ReferenceFrames
+                .Where(f => f.ComponentId != 0)
+                .Single()
+                .ComponentId;
+            var origEventHandlerId = renderer.Batches.Single()
+                .ReferenceFrames
+                .Where(f => f.FrameType == RenderTreeFrameType.Attribute)
+                .Single(f => f.AttributeEventHandlerId != 0)
+                .AttributeEventHandlerId;
+
+            // Act
+            // The fact that there's no error here is the main thing we're testing
+            renderer.DispatchEvent(childComponentId, origEventHandlerId, args: null);
+
+            // Assert: correct render result
+            var newBatch = renderer.Batches.Skip(1).Single();
+            Assert.Equal(1, newBatch.DisposedComponentIDs.Count);
+            Assert.Equal(1, newBatch.DiffsByComponentId.Count);
+            Assert.Collection(newBatch.DiffsByComponentId[componentId].Single().Edits,
+                edit =>
+                {
+                    Assert.Equal(RenderTreeEditType.RemoveFrame, edit.Type);
+                    Assert.Equal(1, edit.SiblingIndex);
+                });
+        }
+
         private class NoOpRenderer : Renderer
         {
             public new int AssignComponentId(IComponent component)
@@ -824,10 +966,10 @@ namespace Microsoft.AspNetCore.Blazor.Test
             public int IntProperty { get; set; }
             public string StringProperty { get; set; }
             public object ObjectProperty { get; set; }
+            public RenderHandle RenderHandle { get; private set; }
 
             public void Init(RenderHandle renderHandle)
-            {
+                => RenderHandle = renderHandle;
-            }
 
             public void SetParameters(ParameterCollection parameters)
                 => parameters.AssignToProperties(this);
@@ -837,6 +979,7 @@ namespace Microsoft.AspNetCore.Blazor.Test
         {
             public UIEventHandler Handler { get; set; }
             public bool SkipElement { get; set; }
+            private int renderCount = 0;
 
             protected override void BuildRenderTree(RenderTreeBuilder builder)
             {
@@ -853,6 +996,7 @@ namespace Microsoft.AspNetCore.Blazor.Test
                     builder.CloseElement();
                 }
                 builder.CloseElement();
+                builder.AddContent(4, $"Render count: {++renderCount}");
             }
 
             public void HandleEvent(UIEventHandler handler, UIEventArgs args)
@@ -901,6 +1045,36 @@ namespace Microsoft.AspNetCore.Blazor.Test
             }
         }
 
+        private class RendersSelfAfterEventComponent : IComponent, IHandleEvent
+        {
+            public Action OnClick { get; set; }
+
+            private RenderHandle _renderHandle;
+
+            public void Init(RenderHandle renderHandle)
+                => _renderHandle = renderHandle;
+
+            public void SetParameters(ParameterCollection parameters)
+            {
+                parameters.AssignToProperties(this);
+                Render();
+            }
+
+            public void HandleEvent(UIEventHandler handler, UIEventArgs args)
+            {
+                handler(args);
+                Render();
+            }
+
+            private void Render()
+                => _renderHandle.Render(builder =>
+                {
+                    builder.OpenElement(0, "my button");
+                    builder.AddAttribute(1, "my click handler", eventArgs => OnClick());
+                    builder.CloseElement();
+                });
+        }
+
         private class MultiRendererComponent : IComponent
         {
             private readonly List<RenderHandle> _renderHandles