dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.5k stars 10.04k forks source link

Prevent use of local functions inside markup #53269

Open verdie-g opened 10 months ago

verdie-g commented 10 months ago

[Edit by @SteveSandersonMS] This issue was originally reported by @verdie-g as follows below the line. On investigation the problem is that C# has added a new syntax that doesn't work in Razor.

The Razor compiler allows arbitrary C# code within @{ ... } blocks. Unfortunately this means it allows the use of local functions in a way that confuses the parsing logic, causing it to use the wrong __builder instance. Example:

<FluentTreeView>
@{
    RenderTree(0, 3);

    void RenderTree(int depth, int maxDepth)
    {
        if (depth >= maxDepth)
        {
            return;
        }

        <FluentTreeItem Text="item">
            @{ RenderTree(depth + 1, maxDepth); }
        </FluentTreeItem>
    }
}
</FluentTreeView>

Here, the child content of FluentTreeItem should be compiled as a RenderFragment that acts on whatever RenderTreeBuilder is passed in. But because of C# scoping rules, the RenderFragment actually acts on the __builder captured from its parent context, so it is simply corrupting the output instead of doing something useful.

Possible solutions:

  1. We could ask for the Razor compiler block the use of local functions inside @{ ... } specifically. However that's probably impractical because Razor doesn't parse the contents of @{ ... }.
    • Perhaps it is achievable as an analyzer that acts on the code after the Razor compiler has generated it.
  2. We could do something in the runtime to detect more generally any cases where the wrong RenderTreeBuilder is invoked. For example if the runtime set an "rendering in progress" flag on it before it starts rendering and synchronously unsets that flag at the end of rendering, then it would have caught this case because child components are rendered afterwards (not recursively), so when the child is rendered it would see it's trying to write to a builder that does not have the "rendering in progress" flag set.
    • Drawback: how do we even check if this flag is set? We would not check it as part of each rendering instruction. There's plenty of evidence that rendering perf is sensitive to that kind of thing (and we can't just check it in development either).
    • Possible solution: instead of just setting a flag, actually null out the referencing to the underlying buffer (storing it in some other field to be swapped back later). Then if anyone tries to write to the builder while it's not marked as rendering-in-progress, they will get a NullReferenceException instead of corrupt output. Obviously that's not super easy to understand but avoids any perf cost.

Is there an existing issue for this?

Describe the bug

I'm rendering a blazor wasm component using a recursive C# method and while it's working fine using C# only (OpenComponent, AddAttribute, etc.), it fails when returning HTML from that recursive method.

Expected Behavior

I'm expecting a tree structure to be built and clicking on a line should expand its children but it seems like the children are not rendered and an error is thrown on click.

Steps To Reproduce

I was not able to reproduce the issue without the library fluentui-blazor.

dotnet new install Microsoft.FluentUI.AspNetCore.Templates
dotnet new fluentblazorwasm --name aspnetcore-issue-53269
cd aspnetcore-issue-53269

Then replace Home.razor with

@page "/"

<PageTitle>Home</PageTitle>

<FluentTreeView>
@{
    RenderTree(0, 3);

    void RenderTree(int depth, int maxDepth)
    {
        if (depth >= maxDepth)
        {
            return;
        }

        <FluentTreeItem Text="item">
            @{ RenderTree(depth + 1, maxDepth); }
        </FluentTreeItem>
    }
}
</FluentTreeView>

Click on the item generated and check the error in the console.

It could be an error with the library (initially reported here https://github.com/microsoft/fluentui-blazor/issues/1289) but this code works fine:

@page "/"

<PageTitle>Home</PageTitle>

<FluentTreeView>
    @RenderTree(0, 3)
</FluentTreeView>

@code {
    public static RenderFragment RenderTree(int depth, int maxDepth)
    {
        return builder =>
        {
            if (depth >= maxDepth)
            {
                return;
            }

            builder.OpenComponent<FluentTreeItem>(0);
            builder.AddAttribute(1, "Text", "item");
            builder.AddAttribute(2, "ChildContent", RenderTree(depth + 1, maxDepth));
            builder.CloseComponent();
        };
    }
}

Exceptions (if any)

blazor.webassembly.js:1 crit: Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
      Unhandled exception rendering component: TypeError: Cannot read properties of undefined (reading 'parentNode')
TypeError: Cannot read properties of undefined (reading 'parentNode')
Ft @ blazor.webassembly.js:1
(anonymous) @ invoke-js.ts:176
Ul @ invoke-js.ts:276
$func350 @ 00b1ee2e:0x1faef
$func246 @ 00b1ee2e:0x1bf8c
$func239 @ 00b1ee2e:0xf173
$func307 @ 00b1ee2e:0x1e7e5
$func328 @ 00b1ee2e:0x1efdb
$func217 @ 00b1ee2e:0xcfd9
$mono_wasm_execute_timer @ 00b1ee2e:0x100f91
e.<computed> @ cwraps.ts:338
mono_wasm_schedule_timer_tick @ scheduling.ts:75
callUserCallback @ dotnet.native.8.0.1.3jibzmihho.js:8
(anonymous) @ dotnet.native.8.0.1.3jibzmihho.js:8
setTimeout (async)
safeSetTimeout @ dotnet.native.8.0.1.3jibzmihho.js:8
Ul @ scheduling.ts:66
$func3522 @ 00b1ee2e:0x100fb4
$func350 @ 00b1ee2e:0x1faef
$func246 @ 00b1ee2e:0x1bf8c
$func239 @ 00b1ee2e:0xf173
$func273 @ 00b1ee2e:0x1d1b1
$func3185 @ 00b1ee2e:0xe81aa
$func2506 @ 00b1ee2e:0xbdf5b
$func2505 @ 00b1ee2e:0xbdeeb
$func1875 @ 00b1ee2e:0x99f9d
$func350 @ 00b1ee2e:0x1fb73
$func246 @ 00b1ee2e:0x1bf8c
$func239 @ 00b1ee2e:0xf173
$func273 @ 00b1ee2e:0x1d1b1
$func3185 @ 00b1ee2e:0xe81aa
$func2506 @ 00b1ee2e:0xbdf5b
$func2512 @ 00b1ee2e:0xbe75c
$func2536 @ 00b1ee2e:0xc0db3
$mono_wasm_invoke_method_bound @ 00b1ee2e:0xa526
Module._mono_wasm_invoke_method_bound @ dotnet.native.8.0.1.3jibzmihho.js:8
kr @ invoke-cs.ts:273
(anonymous) @ invoke-cs.ts:247
beginInvokeDotNetFromJS @ blazor.webassembly.js:1
invokeDotNetMethodAsync @ blazor.webassembly.js:1
invokeMethodAsync @ blazor.webassembly.js:1
(anonymous) @ blazor.webassembly.js:1
N @ blazor.webassembly.js:1
(anonymous) @ blazor.webassembly.js:1
invokeWhenHeapUnlocked @ blazor.webassembly.js:1
(anonymous) @ blazor.webassembly.js:1
N @ blazor.webassembly.js:1
C @ blazor.webassembly.js:1
dispatchGlobalEventToAllElements @ blazor.webassembly.js:1
(anonymous) @ blazor.webassembly.js:1
onGlobalEvent @ blazor.webassembly.js:1
emit @ web-components.js:2323
$emit @ web-components.js:2423
selectedChanged @ web-components.js:16989
setValue @ web-components.js:1871
set @ web-components.js:669
handleClick @ web-components.js:17246
(anonymous) @ web-components.js:17044
handleEvent @ web-components.js:1117

.NET Version

8.0.101

Anything else?

No response

SteveSandersonMS commented 10 months ago

I'm afraid you can't do this:

<FluentTreeView>
@{
    RenderTree(0, 3);

    void RenderTree(int depth, int maxDepth)
    {
        if (depth >= maxDepth)
        {
            return;
        }

        <FluentTreeItem Text="item">
            @{ RenderTree(depth + 1, maxDepth); }
        </FluentTreeItem>
    }
}
</FluentTreeView>

This is adding procedural logic in the middle of rendering that shares the same RenderTreeBuilder just as a fluke of C# scoping rules. It will write output to the builder in an unsupported order, without any correctness guarantees provided by normal Razor syntax. It's unfortunate that C# local method syntax creates the possibility of this pattern which will not work.

It could be an error with the library (initially reported here https://github.com/microsoft/fluentui-blazor/issues/1289) but this code works fine:

That does work fine because it's returning a real RenderFragment which can be rendered properly.

verdie-g commented 10 months ago

So why does it seem to work fine with div elements? 🤔

<div>
    @{
        RenderTree(0, 3);

        void RenderTree(int depth, int maxDepth)
        {
            if (depth >= maxDepth)
            {
                return;
            }

            <div>
                item
                @{ RenderTree(depth + 1, maxDepth); }
            </div>
        }
    }
</div>

Also is it possible to detect that and throw a proper error?

SteveSandersonMS commented 10 months ago

Rendering an HTML element and rendering a child component are very different. It's still not supported to do this for <div> even if it happens to work by chance in this case.

Also is it possible to detect that and throw a proper error?

That would be good. I'll update the issue title to match.

ghost commented 10 months ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.