Skip to content
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.

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

[blazor] Set Cross-Origin-Policy headers for multi-threaded runtime #47855

Merged
merged 5 commits into from
Jun 28, 2023

Conversation

maraf
Copy link
Member

@maraf maraf commented Apr 24, 2023

The multi-threaded runtime requires COP headers to enable SharedArrayBuffer in the browser.
In this PR we set them for

  • all .js files under _framework directory
  • fallback index.html

It changes only DevServer.

The runtime changes to enable threads in blazor are in the dotnet/runtime#85109

@maraf maraf added area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly labels Apr 24, 2023
@maraf maraf added this to the 8.0-preview4 milestone Apr 24, 2023
@maraf maraf requested a review from a team as a code owner April 24, 2023 13:15
@maraf maraf self-assigned this Apr 24, 2023
lambdageek
lambdageek previously approved these changes Apr 24, 2023
@maraf
Copy link
Member Author

maraf commented Apr 24, 2023

@javiercn I'm not sure if this is enough for hosted mode

@javiercn
Copy link
Member

@maraf unfortunately no.

We'll have to find a solution for it. The best way to do it so far is to apply the changes in the app code directly, on the call to UseStaticFiles in hosted mode. From Blazor we are not going to intercept the static files middleware (we've done it in the past and causes mayhem)

@maraf
Copy link
Member Author

maraf commented Apr 24, 2023

@javiercn Do we want this change set for standalone mode?

@javiercn
Copy link
Member

@maraf I think so, but we need to understand what consequences (if any) to the dev/deployment experience this has.

/cc: @SteveSandersonMS has thoughts in this space.

@javiercn
Copy link
Member

Longer term I am hoping we can turn these headers dynamically based on configuration or a build time switch.

@lambdageek lambdageek dismissed their stale review April 24, 2023 14:58

I didn't notice this is unconditional

@maraf
Copy link
Member Author

maraf commented Apr 24, 2023

I can move the changes around so they are valid only for devserver

@@ -29,6 +29,21 @@ public static void Configure(IApplicationBuilder app, IConfiguration configurati

app.UseWebAssemblyDebugging();

app.Use(async (ctx, next) =>
Copy link
Member

Choose a reason for hiding this comment

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

Can the DevServer find out about the project build properties somehow? I don't think we want COP for arbitrary blazor projects. it could break someone's existing single threaded project, I think?

Copy link
Member Author

@maraf maraf Apr 25, 2023

Choose a reason for hiding this comment

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

The last change works when the property is put in a csproj. It doesn't accept is passed as commnad-line argument

Copy link
Member Author

Choose a reason for hiding this comment

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

@radical Do you know if there is a way to "evaluate" properties passed as command-line arguments during dotnet run? The passed property has correct value during build, but when RunArguments evaluate, it ignores value from command-line

@ghost
Copy link

ghost commented May 4, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 4, 2023
@pavelsavara
Copy link
Member

Could we detect that the application has <WasmEnableThreads>true</WasmEnableThreads> in it and do it automatically ?

Should that parameter also do something to the published app's server ?

@maraf
Copy link
Member Author

maraf commented Jun 23, 2023

Detecting WasmEnableThreads=true doesn't work for RunArgumets when the property is passed as command-line argument dotnet/sdk#32551. Maybe we should store the information in runtimeConfig.

@pavelsavara
Copy link
Member

I mean in the project file.

@maraf
Copy link
Member Author

maraf commented Jun 26, 2023

@javiercn @pavelsavara Do we want to take this approach? There is a bug in MSBuild property evaluation dotnet/sdk#32551 which breaks it when the property is passed as command-like argument. The issue is assigned and should get fixed. Anyway, I believe that the multi-threading will be almost always set in csproj.

@maraf maraf closed this Jun 26, 2023
@maraf maraf reopened this Jun 26, 2023
@pavelsavara
Copy link
Member

I prefer to make it work with WasmEnableThreads in the csproj as soon as possible.

@maraf maraf merged commit 529e844 into dotnet:main Jun 28, 2023
@ghost ghost modified the milestones: 8.0-preview4, 8.0-preview7 Jun 28, 2023
@maraf maraf deleted the BlazorDevServerCops branch June 28, 2023 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants