Throw if a component produces an invalid render tree. Fixes #24579 #24650
Conversation
SteveSandersonMS
added
area-blazor
| builder.OpenComponent<OtherComponent>(2); | ||
| builder.CloseComponent(); | ||
| builder.CloseRegion(); | ||
| builder.CloseElement(); |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
| { | ||
| // 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()]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Is this something worth patching in 3.1 with an app compat switch? (so that we can turn it on for 3.1 in the templates and users don't get surprised when it breaks later on) |
I don't know we have a mechanism for an appcompat switch given that this is purely a |
I was referring to https://docs.microsoft.com/en-us/dotnet/api/system.appcontext?view=netstandard-2.1, wouldn't this work? |
|
Maybe. It’s worth considering. We need to check if it has any meaningful impact on app size after linking. Might not do if other framework code already references it. |
The cause of #24579 was what I thought. Components aren't allowed to produce invalid trees - this has always been an unsupported situation. However it's unfortunate that developers can produce invalid trees using something as reasonable-seeming as a
try/catchblock, so we need to do something to prevent this class of problems.Previously we didn't detect this scenario, so the diffing logic could just get stuck on it. With this PR we check whether there were any unclosed tree entries left behind, and if so, fails the entire rendering process with an exception, so the behavior is properly defined.
Longer term we might want to prevent the
try/catchblock at compile time in the Razor compiler, but this runtime check is still desirable, and is sufficient to block the situation.