diff --git a/src/Components/Components/src/CascadingValue.cs b/src/Components/Components/src/CascadingValue.cs index 564ca99def..626dc89833 100644 --- a/src/Components/Components/src/CascadingValue.cs +++ b/src/Components/Components/src/CascadingValue.cs @@ -127,7 +127,7 @@ namespace Microsoft.AspNetCore.Components if (_subscribers != null && ChangeDetection.MayHaveChanged(previousValue, Value)) { - NotifySubscribers(); + NotifySubscribers(parameters.Lifetime); } return Task.CompletedTask; @@ -168,11 +168,11 @@ namespace Microsoft.AspNetCore.Components _subscribers.Remove(subscriber); } - private void NotifySubscribers() + private void NotifySubscribers(ParameterViewLifetime lifetime) { foreach (var subscriber in _subscribers) { - subscriber.NotifyCascadingValueChanged(); + subscriber.NotifyCascadingValueChanged(lifetime); } } diff --git a/src/Components/Components/src/ParameterView.cs b/src/Components/Components/src/ParameterView.cs index 7ecbade21e..8dafe9c2c5 100644 --- a/src/Components/Components/src/ParameterView.cs +++ b/src/Components/Components/src/ParameterView.cs @@ -21,24 +21,21 @@ namespace Microsoft.AspNetCore.Components RenderTreeFrame.Element(0, string.Empty).WithComponentSubtreeLength(1) }; - private static readonly ParameterView _empty = new ParameterView(null, _emptyFrames, 0, null); - - // If a value is provided for this field, then the ParameterView instance is only - // valid for as long as the lifetime owner says it is - private readonly RenderBatchBuilder _lifetimeOwner; + private static readonly ParameterView _empty = new ParameterView(ParameterViewLifetime.Unbound, _emptyFrames, 0, null); + private readonly ParameterViewLifetime _lifetime; private readonly RenderTreeFrame[] _frames; private readonly int _ownerIndex; private readonly IReadOnlyList _cascadingParametersOrNull; - internal ParameterView(RenderBatchBuilder lifetimeOwnerOrNull, RenderTreeFrame[] frames, int ownerIndex) - : this(lifetimeOwnerOrNull, frames, ownerIndex, null) + internal ParameterView(ParameterViewLifetime lifetime, RenderTreeFrame[] frames, int ownerIndex) + : this(lifetime, frames, ownerIndex, null) { } - private ParameterView(RenderBatchBuilder lifetimeOwnerOrNull, RenderTreeFrame[] frames, int ownerIndex, IReadOnlyList cascadingParametersOrNull) + private ParameterView(ParameterViewLifetime lifetime, RenderTreeFrame[] frames, int ownerIndex, IReadOnlyList cascadingParametersOrNull) { - _lifetimeOwner = lifetimeOwnerOrNull; + _lifetime = lifetime; _frames = frames; _ownerIndex = ownerIndex; _cascadingParametersOrNull = cascadingParametersOrNull; @@ -49,12 +46,17 @@ namespace Microsoft.AspNetCore.Components /// public static ParameterView Empty => _empty; + internal ParameterViewLifetime Lifetime => _lifetime; + /// /// Returns an enumerator that iterates through the . /// /// The enumerator. public Enumerator GetEnumerator() - => new Enumerator(_frames, _ownerIndex, _cascadingParametersOrNull); + { + _lifetime.AssertNotExpired(); + return new Enumerator(_frames, _ownerIndex, _cascadingParametersOrNull); + } /// /// Gets the value of the parameter with the specified name. @@ -114,7 +116,7 @@ namespace Microsoft.AspNetCore.Components } internal ParameterView WithCascadingParameters(IReadOnlyList cascadingParameters) - => new ParameterView(_lifetimeOwner, _frames, _ownerIndex, cascadingParameters); + => new ParameterView(_lifetime, _frames, _ownerIndex, cascadingParameters); // It's internal because there isn't a known use case for user code comparing // ParameterView instances, and even if there was, it's unlikely it should @@ -221,7 +223,7 @@ namespace Microsoft.AspNetCore.Components frames[++i] = RenderTreeFrame.Attribute(i, kvp.Key, kvp.Value); } - return new ParameterView(null, frames, 0); + return new ParameterView(ParameterViewLifetime.Unbound, frames, 0); } /// diff --git a/src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs b/src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs index ee40692430..11f5540f4c 100644 --- a/src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs +++ b/src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs @@ -519,8 +519,9 @@ namespace Microsoft.AspNetCore.Components.RenderTree // comparisons it wants with the old values. Later we could choose to pass the // old parameter values if we wanted. By default, components always rerender // after any SetParameters call, which is safe but now always optimal for perf. - var oldParameters = new ParameterView(null, oldTree, oldComponentIndex); - var newParameters = new ParameterView(diffContext.BatchBuilder, newTree, newComponentIndex); + var oldParameters = new ParameterView(ParameterViewLifetime.Unbound, oldTree, oldComponentIndex); + var newParametersLifetime = new ParameterViewLifetime(diffContext.BatchBuilder); + var newParameters = new ParameterView(newParametersLifetime, newTree, newComponentIndex); if (!newParameters.DefinitelyEquals(oldParameters)) { componentState.SetDirectParameters(newParameters); @@ -893,7 +894,8 @@ namespace Microsoft.AspNetCore.Components.RenderTree var childComponentState = frame.ComponentState; // Set initial parameters - var initialParameters = new ParameterView(diffContext.BatchBuilder, frames, frameIndex); + var initialParametersLifetime = new ParameterViewLifetime(diffContext.BatchBuilder); + var initialParameters = new ParameterView(initialParametersLifetime, frames, frameIndex); childComponentState.SetDirectParameters(initialParameters); } diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index e94ddc370d..08ff8cafc4 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -73,6 +73,7 @@ namespace Microsoft.AspNetCore.Components.Rendering _renderTreeBuilderPrevious.GetFrames(), CurrentRenderTree.GetFrames()); batchBuilder.UpdatedComponentDiffs.Append(diff); + batchBuilder.InvalidateParameterViews(); } public bool TryDisposeInBatch(RenderBatchBuilder batchBuilder, out Exception exception) @@ -156,10 +157,10 @@ namespace Microsoft.AspNetCore.Components.Rendering _renderer.AddToPendingTasks(Component.SetParametersAsync(parameters)); } - public void NotifyCascadingValueChanged() + public void NotifyCascadingValueChanged(ParameterViewLifetime lifetime) { var directParams = _latestDirectParametersSnapshot != null - ? new ParameterView(null, _latestDirectParametersSnapshot.Buffer, 0) + ? new ParameterView(lifetime, _latestDirectParametersSnapshot.Buffer, 0) : ParameterView.Empty; var allParams = directParams.WithCascadingParameters(_cascadingParameters); var task = Component.SetParametersAsync(allParams); diff --git a/src/Components/Components/src/Rendering/ParameterViewLifetime.cs b/src/Components/Components/src/Rendering/ParameterViewLifetime.cs new file mode 100644 index 0000000000..7fe5a244a9 --- /dev/null +++ b/src/Components/Components/src/Rendering/ParameterViewLifetime.cs @@ -0,0 +1,30 @@ +// 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; + +namespace Microsoft.AspNetCore.Components.Rendering +{ + internal readonly struct ParameterViewLifetime + { + private readonly RenderBatchBuilder _owner; + private readonly int _stamp; + + public static readonly ParameterViewLifetime Unbound = default; + + public ParameterViewLifetime(RenderBatchBuilder owner) + { + _owner = owner; + _stamp = owner.ParameterViewValidityStamp; + } + + public void AssertNotExpired() + { + // _owner will be null if this instance is default(ParameterViewLifetime) + if (_owner != null && _owner.ParameterViewValidityStamp != _stamp) + { + throw new InvalidOperationException($"The {nameof(ParameterView)} instance can no longer be read because it has expired. {nameof(ParameterView)} can only be read synchronously and must not be stored for later use."); + } + } + } +} diff --git a/src/Components/Components/src/Rendering/RenderBatchBuilder.cs b/src/Components/Components/src/Rendering/RenderBatchBuilder.cs index f26a248fd8..fcb118e09a 100644 --- a/src/Components/Components/src/Rendering/RenderBatchBuilder.cs +++ b/src/Components/Components/src/Rendering/RenderBatchBuilder.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Threading; using Microsoft.AspNetCore.Components.RenderTree; namespace Microsoft.AspNetCore.Components.Rendering @@ -15,6 +16,11 @@ namespace Microsoft.AspNetCore.Components.Rendering /// internal class RenderBatchBuilder : IDisposable { + // A value that, if changed, causes expiry of all ParameterView instances issued + // for this RenderBatchBuilder. This is to prevent invalid reads from arrays that + // may have been returned to the shared pool. + private int _parameterViewValidityStamp; + // Primary result data public ArrayBuilder UpdatedComponentDiffs { get; } = new ArrayBuilder(); public ArrayBuilder DisposedComponentIds { get; } = new ArrayBuilder(); @@ -31,6 +37,8 @@ namespace Microsoft.AspNetCore.Components.Rendering // Scratch data structure for understanding attribute diffs. public Dictionary AttributeDiffSet { get; } = new Dictionary(); + public int ParameterViewValidityStamp => _parameterViewValidityStamp; + internal StackObjectPool> KeyedItemInfoDictionaryPool { get; } = new StackObjectPool>(maxPreservedItems: 10, () => new Dictionary()); @@ -58,6 +66,22 @@ namespace Microsoft.AspNetCore.Components.Rendering DisposedComponentIds.ToRange(), DisposedEventHandlerIds.ToRange()); + public void InvalidateParameterViews() + { + // Wrapping is fine because all that matters is whether a snapshotted value matches + // the current one. There's no plausible case where it wraps around and happens to + // increment all the way back to a previously-snapshotted value on the exact same + // call that's checking the value. + if (_parameterViewValidityStamp == int.MaxValue) + { + _parameterViewValidityStamp = int.MinValue; + } + else + { + _parameterViewValidityStamp++; + } + } + public void Dispose() { EditsBuffer.Dispose(); diff --git a/src/Components/Components/test/CascadingParameterTest.cs b/src/Components/Components/test/CascadingParameterTest.cs index 0716474c30..a906664741 100644 --- a/src/Components/Components/test/CascadingParameterTest.cs +++ b/src/Components/Components/test/CascadingParameterTest.cs @@ -385,7 +385,7 @@ namespace Microsoft.AspNetCore.Components.Test // It's no longer able to access anything in the ParameterView it just received var ex = Assert.Throws(nestedComponent.AttemptIllegalAccessToLastParameterView); - Assert.Equal("blah", ex.Message); + Assert.Equal($"The {nameof(ParameterView)} instance can no longer be read because it has expired. {nameof(ParameterView)} can only be read synchronously and must not be stored for later use.", ex.Message); } private static T FindComponent(CapturedBatch batch, out int componentId) diff --git a/src/Components/Components/test/ParameterViewTest.Assignment.cs b/src/Components/Components/test/ParameterViewTest.Assignment.cs index f5d0d7a257..9df9feab9f 100644 --- a/src/Components/Components/test/ParameterViewTest.Assignment.cs +++ b/src/Components/Components/test/ParameterViewTest.Assignment.cs @@ -673,7 +673,7 @@ namespace Microsoft.AspNetCore.Components } builder.CloseComponent(); - var view = new ParameterView(null, builder.GetFrames().Array, ownerIndex: 0); + var view = new ParameterView(ParameterViewLifetime.Unbound, builder.GetFrames().Array, ownerIndex: 0); var cascadingParameters = new List(); foreach (var kvp in _keyValuePairs) diff --git a/src/Components/Components/test/ParameterViewTest.cs b/src/Components/Components/test/ParameterViewTest.cs index 96072a2dad..8aad6c9140 100644 --- a/src/Components/Components/test/ParameterViewTest.cs +++ b/src/Components/Components/test/ParameterViewTest.cs @@ -20,7 +20,7 @@ namespace Microsoft.AspNetCore.Components { RenderTreeFrame.ChildComponent(0, typeof(FakeComponent)).WithComponentSubtreeLength(1) }; - var parameters = new ParameterView(null, frames, 0); + var parameters = new ParameterView(ParameterViewLifetime.Unbound, frames, 0); // Assert Assert.Empty(ToEnumerable(parameters)); @@ -34,7 +34,7 @@ namespace Microsoft.AspNetCore.Components { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(1) }; - var parameters = new ParameterView(null, frames, 0); + var parameters = new ParameterView(ParameterViewLifetime.Unbound, frames, 0); // Assert Assert.Empty(ToEnumerable(parameters)); @@ -56,7 +56,7 @@ namespace Microsoft.AspNetCore.Components // end of the owner's descendants RenderTreeFrame.Attribute(3, "orphaned attribute", "value") }; - var parameters = new ParameterView(null, frames, 0); + var parameters = new ParameterView(ParameterViewLifetime.Unbound, frames, 0); // Assert Assert.Collection(ToEnumerable(parameters), @@ -78,7 +78,7 @@ namespace Microsoft.AspNetCore.Components RenderTreeFrame.Element(3, "child element").WithElementSubtreeLength(2), RenderTreeFrame.Attribute(4, "child attribute", "some value") }; - var parameters = new ParameterView(null, frames, 0); + var parameters = new ParameterView(ParameterViewLifetime.Unbound, frames, 0); // Assert Assert.Collection(ToEnumerable(parameters), @@ -93,7 +93,7 @@ namespace Microsoft.AspNetCore.Components var attribute1Value = new object(); var attribute2Value = new object(); var attribute3Value = new object(); - var parameters = new ParameterView(null, new[] + var parameters = new ParameterView(ParameterViewLifetime.Unbound, new[] { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(2), RenderTreeFrame.Attribute(1, "attribute 1", attribute1Value) @@ -114,7 +114,7 @@ namespace Microsoft.AspNetCore.Components public void CanTryGetNonExistingValue() { // Arrange - var parameters = new ParameterView(null, new[] + var parameters = new ParameterView(ParameterViewLifetime.Unbound, new[] { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(2), RenderTreeFrame.Attribute(1, "some other entry", new object()) @@ -132,7 +132,7 @@ namespace Microsoft.AspNetCore.Components public void CanTryGetExistingValueWithCorrectType() { // Arrange - var parameters = new ParameterView(null, new[] + var parameters = new ParameterView(ParameterViewLifetime.Unbound, new[] { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(2), RenderTreeFrame.Attribute(1, "my entry", "hello") @@ -151,7 +151,7 @@ namespace Microsoft.AspNetCore.Components { // Arrange var myEntryValue = new object(); - var parameters = new ParameterView(null, new[] + var parameters = new ParameterView(ParameterViewLifetime.Unbound, new[] { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(2), RenderTreeFrame.Attribute(1, "my entry", myEntryValue), @@ -170,7 +170,7 @@ namespace Microsoft.AspNetCore.Components { // Arrange var myEntryValue = new object(); - var parameters = new ParameterView(null, new[] + var parameters = new ParameterView(ParameterViewLifetime.Unbound, new[] { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(3), RenderTreeFrame.Attribute(1, "my entry", myEntryValue), @@ -188,7 +188,7 @@ namespace Microsoft.AspNetCore.Components public void CanGetValueOrDefault_WithNonExistingValue() { // Arrange - var parameters = new ParameterView(null, new[] + var parameters = new ParameterView(ParameterViewLifetime.Unbound, new[] { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(2), RenderTreeFrame.Attribute(1, "some other entry", new object()) @@ -209,7 +209,7 @@ namespace Microsoft.AspNetCore.Components { // Arrange var explicitDefaultValue = new DateTime(2018, 3, 20); - var parameters = new ParameterView(null, new[] + var parameters = new ParameterView(ParameterViewLifetime.Unbound, new[] { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(2), RenderTreeFrame.Attribute(1, "some other entry", new object()) @@ -226,7 +226,7 @@ namespace Microsoft.AspNetCore.Components public void ThrowsIfTryGetExistingValueWithIncorrectType() { // Arrange - var parameters = new ParameterView(null, new[] + var parameters = new ParameterView(ParameterViewLifetime.Unbound, new[] { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(2), RenderTreeFrame.Attribute(1, "my entry", "hello") @@ -275,7 +275,7 @@ namespace Microsoft.AspNetCore.Components { // Arrange var entry2Value = new object(); - var parameters = new ParameterView(null, new[] + var parameters = new ParameterView(ParameterViewLifetime.Unbound, new[] { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(3), RenderTreeFrame.Attribute(0, "entry 1", "value 1"), @@ -304,7 +304,7 @@ namespace Microsoft.AspNetCore.Components { // Arrange var myEntryValue = new object(); - var parameters = new ParameterView(null, new[] + var parameters = new ParameterView(ParameterViewLifetime.Unbound, new[] { RenderTreeFrame.Element(0, "some element").WithElementSubtreeLength(2), RenderTreeFrame.Attribute(1, "unrelated value", new object()) diff --git a/src/Components/Components/test/RendererTest.cs b/src/Components/Components/test/RendererTest.cs index eac2d1b13a..9d88c3f974 100644 --- a/src/Components/Components/test/RendererTest.cs +++ b/src/Components/Components/test/RendererTest.cs @@ -3706,17 +3706,26 @@ namespace Microsoft.AspNetCore.Components.Test { // Arrange var renderer = new TestRenderer(); - var component = new ParameterViewIllegalCapturingComponent(); - var componentId = renderer.AssignRootComponentId(component); - renderer.RenderRootComponentAsync(componentId); + var rootComponent = new TestComponent(builder => + { + builder.OpenComponent(0); + builder.AddAttribute(1, nameof(ParameterViewIllegalCapturingComponent.SomeParam), 0); + builder.CloseComponent(); + }); + var rootComponentId = renderer.AssignRootComponentId(rootComponent); + + // Note that we're not waiting for the async render to complete, since we want to assert + // about the situation immediately after the component yields the thread + renderer.RenderRootComponentAsync(rootComponentId); // Act/Assert + var capturingComponent = (ParameterViewIllegalCapturingComponent)renderer.GetCurrentRenderTreeFrames(rootComponentId).Array[0].Component; var ex = Assert.Throws(() => { // TODO: check other types of access too - component.CapturedParameterView.TryGetValue("anything", out _); + capturingComponent.CapturedParameterView.TryGetValue("anything", out _); }); - Assert.Equal("blah", ex.Message); + Assert.Equal($"The {nameof(ParameterView)} instance can no longer be read because it has expired. {nameof(ParameterView)} can only be read synchronously and must not be stored for later use.", ex.Message); } private class NoOpRenderer : Renderer @@ -4466,6 +4475,8 @@ namespace Microsoft.AspNetCore.Components.Test { public ParameterView CapturedParameterView { get; private set; } + [Parameter] public int SomeParam { get; set; } + public void Attach(RenderHandle renderHandle) { }