diff --git a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts index 1fe50dc9e8..fa6b5aa5f3 100644 --- a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts +++ b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts @@ -6,9 +6,6 @@ let raiseEventMethod: MethodHandle; let renderComponentMethod: MethodHandle; export class BrowserRenderer { - // TODO: To avoid leaking memory, automatically remove entries from this dict as soon - // as the corresponding DOM nodes are removed (or maybe when the associated component - // is disposed, assuming we can guarantee that always happens). private childComponentLocations: { [componentId: number]: Element } = {}; constructor(private browserRendererId: number) { @@ -27,6 +24,10 @@ export class BrowserRenderer { this.applyEdits(componentId, element, 0, edits, editsLength, referenceTree); } + public disposeComponent(componentId: number) { + delete this.childComponentLocations[componentId]; + } + applyEdits(componentId: number, parent: Element, childIndex: number, edits: System_Array, editsLength: number, referenceTree: System_Array) { let currentDepth = 0; let childIndexAtCurrentDepth = childIndex; diff --git a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch.ts b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch.ts index 0326242d9c..fd4f728a65 100644 --- a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch.ts +++ b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/RenderBatch.ts @@ -7,6 +7,7 @@ import { RenderTreeEditPointer } from './RenderTreeEdit'; export const renderBatch = { updatedComponents: (obj: RenderBatchPointer) => platform.readStructField>(obj, 0), + disposedComponentIds: (obj: RenderBatchPointer) => platform.readStructField>(obj, arrayRangeStructLength), }; const arrayRangeStructLength = 8; diff --git a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/Renderer.ts b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/Renderer.ts index 5968ddf726..9c1818ca44 100644 --- a/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/Renderer.ts +++ b/src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/Renderer.ts @@ -32,7 +32,7 @@ export function renderBatch(browserRendererId: number, batch: RenderBatchPointer const updatedComponents = renderBatchStruct.updatedComponents(batch); const updatedComponentsLength = arrayRange.count(updatedComponents); const updatedComponentsArray = arrayRange.array(updatedComponents); - for (var i = 0; i < updatedComponentsLength; i++) { + for (let i = 0; i < updatedComponentsLength; i++) { const diff = platform.getArrayEntryPtr(updatedComponentsArray, i, renderTreeDiffStructLength); const componentId = renderTreeDiff.componentId(diff); @@ -44,6 +44,15 @@ export function renderBatch(browserRendererId: number, batch: RenderBatchPointer const tree = arrayRange.array(currentStateArrayRange); browserRenderer.updateComponent(componentId, edits, editsLength, tree); } + + const disposedComponentIds = renderBatchStruct.disposedComponentIds(batch); + const disposedComponentIdsLength = arrayRange.count(disposedComponentIds); + const disposedComponentIdsArray = arrayRange.array(disposedComponentIds); + for (let i = 0; i < disposedComponentIdsLength; i++) { + const componentIdPtr = platform.getArrayEntryPtr(disposedComponentIdsArray, i, 4); + const componentId = platform.readInt32Field(componentIdPtr); + browserRenderer.disposeComponent(componentId); + } } function clearElement(element: Element) { diff --git a/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeDiffComputer.cs b/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeDiffComputer.cs index f91b52d2e0..5c0903ee38 100644 --- a/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeDiffComputer.cs +++ b/src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeDiffComputer.cs @@ -157,13 +157,19 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree else { ref var oldNode = ref oldTree[oldStartIndex]; - if (oldNode.NodeType == RenderTreeNodeType.Attribute) + var oldNodeType = oldNode.NodeType; + if (oldNodeType == RenderTreeNodeType.Attribute) { Append(RenderTreeEdit.RemoveAttribute(siblingIndex, oldNode.AttributeName)); oldStartIndex++; } else { + if (oldNodeType == RenderTreeNodeType.Element || oldNodeType == RenderTreeNodeType.Component) + { + DisposeChildComponents(batchBuilder, oldTree, oldStartIndex); + } + Append(RenderTreeEdit.RemoveNode(siblingIndex)); oldStartIndex = NextSiblingIndex(oldNode, oldStartIndex); } @@ -381,6 +387,8 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree else { // Elements with different names are treated as completely unrelated + InstantiateChildComponents(batchBuilder, newTree, newNodeIndex); + DisposeChildComponents(batchBuilder, oldTree, oldNodeIndex); Append(RenderTreeEdit.PrependNode(siblingIndex, newNodeIndex)); siblingIndex++; Append(RenderTreeEdit.RemoveNode(siblingIndex)); @@ -404,6 +412,8 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree else { // Child components of different types are treated as completely unrelated + InstantiateChildComponents(batchBuilder, newTree, newNodeIndex); + DisposeChildComponents(batchBuilder, oldTree, oldNodeIndex); Append(RenderTreeEdit.PrependNode(siblingIndex, newNodeIndex)); siblingIndex++; Append(RenderTreeEdit.RemoveNode(siblingIndex)); @@ -502,5 +512,18 @@ namespace Microsoft.AspNetCore.Blazor.RenderTree } } } + + private void DisposeChildComponents(RenderBatchBuilder batchBuilder, RenderTreeNode[] nodes, int elementOrComponentIndex) + { + var endIndex = nodes[elementOrComponentIndex].ElementDescendantsEndIndex; + for (var i = elementOrComponentIndex; i <= endIndex; i++) + { + ref var node = ref nodes[i]; + if (node.NodeType == RenderTreeNodeType.Component) + { + _renderer.DisposeInExistingBatch(batchBuilder, node.ComponentId); + } + } + } } } diff --git a/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs b/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs index 020df06189..c38b050d38 100644 --- a/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs +++ b/src/Microsoft.AspNetCore.Blazor/Rendering/ComponentState.cs @@ -80,5 +80,17 @@ namespace Microsoft.AspNetCore.Blazor.Rendering // developers don't need to call Render() on their components explicitly. _renderer.RenderNewBatch(_componentId); } + + /// + /// Notifies the component that it is being disposed. + /// + public void NotifyDisposed() + { + // TODO: Handle components throwing during dispose. Shouldn't break the whole render batch. + if (_component is IDisposable disposable) + { + disposable.Dispose(); + } + } } } diff --git a/src/Microsoft.AspNetCore.Blazor/Rendering/RenderBatch.cs b/src/Microsoft.AspNetCore.Blazor/Rendering/RenderBatch.cs index 389a38a7e6..ad7fd9dd74 100644 --- a/src/Microsoft.AspNetCore.Blazor/Rendering/RenderBatch.cs +++ b/src/Microsoft.AspNetCore.Blazor/Rendering/RenderBatch.cs @@ -15,9 +15,17 @@ namespace Microsoft.AspNetCore.Blazor.Rendering /// public ArrayRange UpdatedComponents { get; } - internal RenderBatch(ArrayRange updatedComponents) + /// + /// Gets the IDs of the components that were disposed. + /// + public ArrayRange DisposedComponentIDs { get; } + + internal RenderBatch( + ArrayRange updatedComponents, + ArrayRange disposedComponentIDs) { UpdatedComponents = updatedComponents; + DisposedComponentIDs = disposedComponentIDs; } } } diff --git a/src/Microsoft.AspNetCore.Blazor/Rendering/RenderBatchBuilder.cs b/src/Microsoft.AspNetCore.Blazor/Rendering/RenderBatchBuilder.cs index 259a0c2074..9018c9e5fe 100644 --- a/src/Microsoft.AspNetCore.Blazor/Rendering/RenderBatchBuilder.cs +++ b/src/Microsoft.AspNetCore.Blazor/Rendering/RenderBatchBuilder.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using Microsoft.AspNetCore.Blazor.RenderTree; namespace Microsoft.AspNetCore.Blazor.Rendering @@ -8,6 +9,7 @@ namespace Microsoft.AspNetCore.Blazor.Rendering internal class RenderBatchBuilder { private ArrayBuilder _updatedComponentDiffs = new ArrayBuilder(); + private ArrayBuilder _disposedComponentIds = new ArrayBuilder(); public int ReserveUpdatedComponentSlotId() { @@ -20,10 +22,17 @@ namespace Microsoft.AspNetCore.Blazor.Rendering => _updatedComponentDiffs.Overwrite(updatedComponentSlotId, diff); public void Clear() - => _updatedComponentDiffs.Clear(); + { + _updatedComponentDiffs.Clear(); + _disposedComponentIds.Clear(); + } public RenderBatch ToBatch() => new RenderBatch( - _updatedComponentDiffs.ToRange()); + _updatedComponentDiffs.ToRange(), + _disposedComponentIds.ToRange()); + + public void AddDisposedComponent(int componentId) + => _disposedComponentIds.Append(componentId); } } diff --git a/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs b/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs index af34be7513..d9626a881d 100644 --- a/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs +++ b/src/Microsoft.AspNetCore.Blazor/Rendering/Renderer.cs @@ -96,6 +96,12 @@ namespace Microsoft.AspNetCore.Blazor.Rendering GetRequiredComponentState(componentId).Render(batchBuilder); } + internal void DisposeInExistingBatch(RenderBatchBuilder batchBuilder, int componentId) + { + GetRequiredComponentState(componentId).NotifyDisposed(); + batchBuilder.AddDisposedComponent(componentId); + } + /// /// Notifies the specified component that an event has occurred. /// diff --git a/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffComputerTest.cs b/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffComputerTest.cs index e8afb908b5..c8446af5c9 100644 --- a/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffComputerTest.cs +++ b/test/Microsoft.AspNetCore.Blazor.Test/RenderTreeDiffComputerTest.cs @@ -316,16 +316,34 @@ namespace Microsoft.AspNetCore.Blazor.Test newTree.OpenComponentElement(123); // Act - var result = GetSingleUpdatedComponent(); + var renderBatch = GetRenderedBatch(); - // Assert - Assert.Collection(result.Edits, + // Assert: Even though we didn't assign IDs to the components, this + // shows that FakeComponent was disposed + Assert.Collection(renderBatch.DisposedComponentIDs, + disposedComponentId => Assert.Equal(0, disposedComponentId)); + + // Assert: First updated component is the root with one child being + // prepended, and its earlier incarnation being removed + Assert.Equal(2, renderBatch.UpdatedComponents.Count); + var updatedComponent1 = renderBatch.UpdatedComponents.Array[0]; + Assert.Collection(updatedComponent1.Edits, entry => { AssertEdit(entry, RenderTreeEditType.PrependNode, 0); Assert.Equal(0, entry.NewTreeIndex); + Assert.IsType(updatedComponent1.CurrentState.Array[0].Component); }, entry => AssertEdit(entry, RenderTreeEditType.RemoveNode, 1)); + + // Assert: Second updated component is the new FakeComponent2 + var updatedComponent2 = renderBatch.UpdatedComponents.Array[1]; + Assert.Collection(updatedComponent2.Edits, + entry => + { + AssertEdit(entry, RenderTreeEditType.PrependNode, 0); + Assert.Equal(0, entry.NewTreeIndex); + }); } [Fact] @@ -744,6 +762,37 @@ namespace Microsoft.AspNetCore.Blazor.Test Assert.Null(newComponentInstance.ObjectProperty); // To observe that the property wasn't even written, we nulled it out on the original } + [Fact] + public void CallsDisposeOnlyOnRemovedChildComponents() + { + // Arrange + oldTree.OpenComponentElement(10); // + oldTree.CloseElement(); // + oldTree.OpenComponentElement(20); // + oldTree.CloseElement(); // + oldTree.OpenComponentElement(30); // + oldTree.CloseElement(); // + newTree.OpenComponentElement(30); // + newTree.CloseElement(); // + + diff.ApplyNewRenderTreeVersion(new RenderBatchBuilder(), 0, new RenderTreeBuilder(renderer).GetNodes(), oldTree.GetNodes()); + var disposableComponent1 = (DisposableComponent)oldTree.GetNodes().Array[0].Component; + var nonDisposableComponent = (NonDisposableComponent)oldTree.GetNodes().Array[1].Component; + var disposableComponent2 = (DisposableComponent)oldTree.GetNodes().Array[2].Component; + + // Act + var renderedBatch = GetRenderedBatch(); + + // Assert: We track NonDisposableComponent was disposed even though it's not IDisposable + Assert.Equal(renderedBatch.DisposedComponentIDs, new[] { 0, 1 }); + + // Assert: We did call Dispose on the disposed DisposableComponent + Assert.Equal(1, disposableComponent1.DisposalCount); + + // Assert: We didn't dispose the retained component + Assert.Equal(0, disposableComponent2.DisposalCount); + } + private RenderTreeDiff GetSingleUpdatedComponent() { var diffsInBatch = GetRenderedBatch().UpdatedComponents; @@ -786,6 +835,19 @@ namespace Microsoft.AspNetCore.Blazor.Test } } + private class DisposableComponent : IComponent, IDisposable + { + public int DisposalCount { get; private set; } + public void Dispose() => DisposalCount++; + public void BuildRenderTree(RenderTreeBuilder builder) { } + } + + private class NonDisposableComponent : IComponent + { + public void BuildRenderTree(RenderTreeBuilder builder) { } + } + + private static void AssertEdit( RenderTreeEdit edit, RenderTreeEditType type, diff --git a/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs b/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs index cafbb69964..9ef2eb0235 100644 --- a/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs +++ b/test/Microsoft.AspNetCore.Blazor.Test/RendererTest.cs @@ -402,6 +402,45 @@ namespace Microsoft.AspNetCore.Blazor.Test node => AssertNode.Text(node, "second")); } + [Fact] + public void RenderBatchIncludesListOfDisposedComponents() + { + // Arrange + var renderer = new TestRenderer(); + var firstRender = true; + var component = new TestComponent(builder => + { + builder.OpenElement(7, "some element"); + if (firstRender) + { + builder.OpenComponentElement(100); + builder.CloseElement(); + builder.OpenComponentElement(150); + builder.CloseElement(); + } + builder.OpenComponentElement(200); + builder.CloseElement(); + builder.CloseElement(); + }); + + var rootComponentId = renderer.AssignComponentId(component); + + // Act/Assert 1: First render, capturing child component IDs + renderer.RenderNewBatch(rootComponentId); + var childComponentIds = renderer.Batches.Single().RenderTreesByComponentId[rootComponentId] + .Where(node => node.NodeType == RenderTreeNodeType.Component) + .Select(node => node.ComponentId) + .ToList(); + Assert.Equal(childComponentIds, new[] { 1, 2, 3 }); + + // Act: Second render + firstRender = false; + renderer.RenderNewBatch(rootComponentId); + + // Assert: Applicable children are included in disposal list + Assert.Equal(renderer.Batches[1].DisposedComponentIDs, new[] { 1, 2 }); + } + private class NoOpRenderer : Renderer { public new int AssignComponentId(IComponent component) @@ -440,6 +479,8 @@ namespace Microsoft.AspNetCore.Blazor.Test capturedBatch.RenderTreesByComponentId[renderTreeDiff.ComponentId] = renderTreeDiff.CurrentState.ToArray(); } + + capturedBatch.DisposedComponentIDs = renderBatch.DisposedComponentIDs.ToList(); } } @@ -447,6 +488,8 @@ namespace Microsoft.AspNetCore.Blazor.Test { public IDictionary RenderTreesByComponentId { get; } = new Dictionary(); + + public IList DisposedComponentIDs { get; set; } } private class TestComponent : IComponent