Skip to content

Throw if a component produces an invalid render tree. Fixes #24579 #24650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 7, 2020
Merged

Throw if a component produces an invalid render tree. Fixes #24579 #24650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Components/Components/src/Rendering/ComponentState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ public void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment rend
CurrentRenderTree.Clear();
renderFragment(CurrentRenderTree);

CurrentRenderTree.AssertTreeIsValid(Component);

var diff = RenderTreeDiffBuilder.ComputeDiff(
_renderer,
batchBuilder,
Expand Down
11 changes: 11 additions & 0 deletions src/Components/Components/src/Rendering/RenderTreeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,17 @@ internal void InsertAttributeExpensive(int insertAtIndex, int sequence, string a
public ArrayRange<RenderTreeFrame> GetFrames() =>
_entries.ToRange();

internal void AssertTreeIsValid(IComponent component)
{
if (_openElementIndices.Count > 0)
{
// It's never valid to leave an element/component/region unclosed. Doing so
// could cause undefined behavior in diffing.
ref var invalidFrame = ref _entries.Buffer[_openElementIndices.Peek()];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it always guaranteed that the index is going to be in range (assuming there are no bugs in the RenderTreeBuilder code) I believe that's the case, but given we are dealing with "failure modes" here I ask just in case to be more cautious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but even if not, the code here is certain to throw (either from the throw statement on L695, or an index-out-of-range on L694), which is all we need.

throw new InvalidOperationException($"Render output is invalid for component of type '{component.GetType().FullName}'. A frame of type '{invalidFrame.FrameType}' was left unclosed. Do not use try/catch inside rendering logic, because partial output cannot be undone.");
}
}

// Internal for testing
internal void ProcessDuplicateAttributes(int first)
{
Expand Down
16 changes: 16 additions & 0 deletions src/Components/Components/test/RendererTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4001,6 +4001,22 @@ public void CanUseCustomComponentActivatorFromServiceProvider()
requestedType => Assert.Equal(typeof(TestComponent), requestedType));
}

[Fact]
public async Task ThrowsIfComponentProducesInvalidRenderTree()
{
// Arrange
var renderer = new TestRenderer();
var component = new TestComponent(builder =>
{
builder.OpenElement(0, "myElem");
});
var rootComponentId = renderer.AssignRootComponentId(component);

// Act/Assert
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => renderer.RenderRootComponentAsync(rootComponentId));
Assert.StartsWith($"Render output is invalid for component of type '{typeof(TestComponent).FullName}'. A frame of type 'Element' was left unclosed.", ex.Message);
}

private class TestComponentActivator<TResult> : IComponentActivator where TResult : IComponent, new()
{
public List<Type> RequestedComponentTypes { get; } = new List<Type>();
Expand Down
67 changes: 67 additions & 0 deletions src/Components/Components/test/Rendering/RenderTreeBuilderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1831,6 +1831,69 @@ public void ProcessDuplicateAttributes_CanRemoveOverwrittenAttributes()
f => AssertFrame.Attribute(f, "3", "see ya"));
}

[Fact]
public void AcceptsClosedFramesAsValid()
{
// Arrange
var builder = new RenderTreeBuilder();
var component = new TestComponent();
builder.OpenElement(0, "myElem");
builder.OpenRegion(1);
builder.OpenComponent<OtherComponent>(2);
builder.CloseComponent();
builder.CloseRegion();
builder.CloseElement();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have validation for when the close statements balance out but are in different order? For example

builder.OpenElement()
builder.OpenRegion().
builder.CloseElement();
builder.CloseRegion()

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good point to raise, but I think we're OK with this one. In practice all of the Close methods will close whatever frame is outstanding on the stack, so even if you do deliberately get the order wrong it won't be harmful. The only weirdness you could cause by this would be failing to pass parameters to a child component because that's the only difference between the close methods.

Considering how perf-critical is this low-level part of rendering, we wouldn't want penalize people using .razor to produce normal components by introducing extra checking logic for cases that a valid .razor component could never trigger. We've always been clear that anyone writing rendertreebuilder logic manually is in the same situation as people writing C# logic manually: if you write a bug, you have a bug.


// Act/Assert
// Lack of exception is success
builder.AssertTreeIsValid(component);
}

[Fact]
public void ReportsUnclosedElementAsInvalid()
{
// Arrange
var builder = new RenderTreeBuilder();
var component = new TestComponent();
builder.OpenElement(0, "outerElem");
builder.OpenElement(1, "innerElem");
builder.CloseElement();

// Act/Assert
var ex = Assert.Throws<InvalidOperationException>(() => builder.AssertTreeIsValid(component));
Assert.StartsWith($"Render output is invalid for component of type '{typeof(TestComponent).FullName}'. A frame of type 'Element' was left unclosed.", ex.Message);
}

[Fact]
public void ReportsUnclosedComponentAsInvalid()
{
// Arrange
var builder = new RenderTreeBuilder();
var component = new TestComponent();
builder.OpenComponent<OtherComponent>(0);
builder.OpenComponent<OtherComponent>(1);
builder.CloseComponent();

// Act/Assert
var ex = Assert.Throws<InvalidOperationException>(() => builder.AssertTreeIsValid(component));
Assert.StartsWith($"Render output is invalid for component of type '{typeof(TestComponent).FullName}'. A frame of type 'Component' was left unclosed.", ex.Message);
}

[Fact]
public void ReportsUnclosedRegionAsInvalid()
{
// Arrange
var builder = new RenderTreeBuilder();
var component = new TestComponent();
builder.OpenRegion(0);
builder.OpenRegion(1);
builder.CloseRegion();

// Act/Assert
var ex = Assert.Throws<InvalidOperationException>(() => builder.AssertTreeIsValid(component));
Assert.StartsWith($"Render output is invalid for component of type '{typeof(TestComponent).FullName}'. A frame of type 'Region' was left unclosed.", ex.Message);
}

private class TestComponent : IComponent
{
public void Attach(RenderHandle renderHandle) { }
Expand All @@ -1839,6 +1902,10 @@ public Task SetParametersAsync(ParameterView parameters)
=> throw new NotImplementedException();
}

private class OtherComponent : TestComponent
{
}

private class TestRenderer : Renderer
{
public TestRenderer() : base(new TestServiceProvider(), NullLoggerFactory.Instance)
Expand Down