Skip to content

Commit e8917fc

Browse files
SteveSandersonMSjaviercn
authored andcommitted
[Blazor][Fixes #13056] Renderer use-after-disposal tweaks
* Improves Renderer handling use after disposal. * Ensures RemoteRenderer skips resuming the render queue after ACK if it was since disposed
1 parent 9bd027a commit e8917fc

File tree

4 files changed

+58
-3
lines changed

4 files changed

+58
-3
lines changed

src/Components/Components/src/Rendering/Renderer.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ public abstract partial class Renderer : IDisposable
2828
private bool _isBatchInProgress;
2929
private ulong _lastEventHandlerId;
3030
private List<Task> _pendingTasks;
31+
private bool _disposed;
3132

3233
/// <summary>
3334
/// Allows the caller to handle exceptions from the SynchronizationContext when one is available.
@@ -403,6 +404,11 @@ private ComponentState GetOptionalComponentState(int componentId)
403404
/// </summary>
404405
protected virtual void ProcessPendingRender()
405406
{
407+
if (_disposed)
408+
{
409+
throw new ObjectDisposedException(nameof(Renderer), "Cannot process pending renders after the renderer has been disposed.");
410+
}
411+
406412
ProcessRenderQueue();
407413
}
408414

@@ -696,6 +702,8 @@ private void UpdateRenderTreeToMatchClientState(ulong eventHandlerId, EventField
696702
/// <param name="disposing"><see langword="true"/> if this method is being invoked by <see cref="IDisposable.Dispose"/>, otherwise <see langword="false"/>.</param>
697703
protected virtual void Dispose(bool disposing)
698704
{
705+
_disposed = true;
706+
699707
// It's important that we handle all exceptions here before reporting any of them.
700708
// This way we can dispose all components before an error handler kicks in.
701709
List<Exception> exceptions = null;
@@ -717,6 +725,7 @@ protected virtual void Dispose(bool disposing)
717725
}
718726
}
719727

728+
_componentStateById.Clear(); // So we know they were all disposed
720729
_batchBuilder.Dispose();
721730

722731
if (exceptions?.Count > 1)

src/Components/Components/test/RendererTest.cs

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2552,7 +2552,7 @@ public void QueuedRenderIsSkippedIfComponentWasAlreadyDisposedInSameBatch()
25522552
.ComponentId;
25532553
var origEventHandlerId = renderer.Batches.Single()
25542554
.ReferenceFrames
2555-
.Where(f => f.FrameType == RenderTreeFrameType.Attribute && f.AttributeName == "onclick")
2555+
.Where(f => f.FrameType == RenderTreeFrameType.Attribute && f.AttributeName == "onmycustomevent")
25562556
.Single(f => f.AttributeEventHandlerId != 0)
25572557
.AttributeEventHandlerId;
25582558

@@ -3490,6 +3490,41 @@ public void DisposingRenderer_DisposesTopLevelComponents()
34903490
Assert.True(component.Disposed);
34913491
}
34923492

3493+
[Fact]
3494+
public void DisposingRenderer_RejectsAttemptsToStartMoreRenderBatches()
3495+
{
3496+
// Arrange
3497+
var renderer = new TestRenderer();
3498+
renderer.Dispose();
3499+
3500+
// Act/Assert
3501+
var ex = Assert.Throws<ObjectDisposedException>(() => renderer.ProcessPendingRender());
3502+
Assert.Contains("Cannot process pending renders after the renderer has been disposed.", ex.Message);
3503+
}
3504+
3505+
[Fact]
3506+
public void WhenRendererIsDisposed_ComponentRenderRequestsAreSkipped()
3507+
{
3508+
// The important point of this is that user code in components may continue to call
3509+
// StateHasChanged (e.g., after an async task completion), and we don't want that to
3510+
// show up as an error. In general, components should skip rendering after disposal.
3511+
// This test shows that we don't add any new entries to the render queue after disposal.
3512+
// There's a different test showing that if the render queue entry was already added
3513+
// before a component got individually disposed, that render queue entry gets skipped.
3514+
3515+
// Arrange
3516+
var renderer = new TestRenderer();
3517+
var component = new DisposableComponent();
3518+
renderer.AssignRootComponentId(component);
3519+
3520+
// Act
3521+
renderer.Dispose();
3522+
component.TriggerRender();
3523+
3524+
// Assert: no exception, no batch produced
3525+
Assert.Empty(renderer.Batches);
3526+
}
3527+
34933528
[Fact]
34943529
public void DisposingRenderer_DisposesNestedComponents()
34953530
{
@@ -3936,7 +3971,7 @@ private void Render()
39363971
=> _renderHandle.Render(builder =>
39373972
{
39383973
builder.OpenElement(0, "my button");
3939-
builder.AddAttribute(1, "my click handler", new Action<EventArgs>(eventArgs => OnClick(eventArgs)));
3974+
builder.AddAttribute(1, "onmycustomevent", EventCallback.Factory.Create(this, eventArgs => OnClick(eventArgs)));
39403975
builder.CloseElement();
39413976
});
39423977
}

src/Components/Server/src/Circuits/RemoteRenderer.cs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,15 @@ public Task OnRenderCompleted(long incomingBatchId, string errorMessageOrNull)
284284
// missing.
285285

286286
// We return the task in here, but the caller doesn't await it.
287-
return Dispatcher.InvokeAsync(() => ProcessPendingRender());
287+
return Dispatcher.InvokeAsync(() =>
288+
{
289+
// Now we're on the sync context, check again whether we got disposed since this
290+
// work item was queued. If so there's nothing to do.
291+
if (!_disposing)
292+
{
293+
ProcessPendingRender();
294+
}
295+
});
288296
}
289297
}
290298

src/Components/Shared/test/TestRenderer.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,5 +125,8 @@ protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
125125
OnUpdateDisplayComplete?.Invoke();
126126
return NextRenderResultTask;
127127
}
128+
129+
public new void ProcessPendingRender()
130+
=> base.ProcessPendingRender();
128131
}
129132
}

0 commit comments

Comments
 (0)