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

[release/7.0] WasmAppHost: Fix crash when starting wasmbrowser project #76236

Merged
merged 3 commits into from
Sep 29, 2022

Conversation

radical
Copy link
Member

@radical radical commented Sep 27, 2022

/cc @lewing @thaystg @pavelsavara

Customer impact

On 7.0, we created a new template to create a wasm browser app without using blazor, but running it would crash because we try to read the web server addresses right after await host.StartAsync, but they might not be available at that time. Instead, we now read the addresses in response to the ApplicationStarted event.

This could fail like:

WasmAppHost --runtime-config /tmp/cc/bin/Debug/net7.0/browser-wasm/AppBundle/cc.runtimeconfig.json --forward-console
Unhandled exception. System.InvalidOperationException: Failed to determine web server's IP address or port
   at Microsoft.WebAssembly.AppHost.WebServer.StartAsync(WebServerOptions options, ILogger logger, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.BrowserHost.StartWebServerAsync(String appPath, Boolean forwardConsole, String[] urls, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.BrowserHost.RunAsync(ILoggerFactory loggerFactory, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.BrowserHost.InvokeAsync(CommonConfiguration commonArgs, ILoggerFactory loggerFactory, ILogger logger, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.WasmAppHost.Main(String[] args)
   at Microsoft.WebAssembly.AppHost.WasmAppHost.<Main>(String[] args)

Testing

Unit tests with workloads, and manual testing.

Risk

Low risk, prevents a crash.

This could fail like:
```
WasmAppHost --runtime-config /tmp/cc/bin/Debug/net7.0/browser-wasm/AppBundle/cc.runtimeconfig.json --forward-console
Unhandled exception. System.InvalidOperationException: Failed to determine web server's IP address or port
   at Microsoft.WebAssembly.AppHost.WebServer.StartAsync(WebServerOptions options, ILogger logger, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.BrowserHost.StartWebServerAsync(String appPath, Boolean forwardConsole, String[] urls, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.BrowserHost.RunAsync(ILoggerFactory loggerFactory, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.BrowserHost.InvokeAsync(CommonConfiguration commonArgs, ILoggerFactory loggerFactory, ILogger logger, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.WasmAppHost.Main(String[] args)
   at Microsoft.WebAssembly.AppHost.WasmAppHost.<Main>(String[] args)
```

Instead read the addresses on application lifetime's ApplicationStarted
event.
@radical radical added arch-wasm WebAssembly architecture area-VM-meta-mono labels Sep 27, 2022
@ghost ghost assigned radical Sep 27, 2022
@ghost
Copy link

ghost commented Sep 27, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

This could fail like:

WasmAppHost --runtime-config /tmp/cc/bin/Debug/net7.0/browser-wasm/AppBundle/cc.runtimeconfig.json --forward-console
Unhandled exception. System.InvalidOperationException: Failed to determine web server's IP address or port
   at Microsoft.WebAssembly.AppHost.WebServer.StartAsync(WebServerOptions options, ILogger logger, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.BrowserHost.StartWebServerAsync(String appPath, Boolean forwardConsole, String[] urls, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.BrowserHost.RunAsync(ILoggerFactory loggerFactory, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.BrowserHost.InvokeAsync(CommonConfiguration commonArgs, ILoggerFactory loggerFactory, ILogger logger, CancellationToken token)
   at Microsoft.WebAssembly.AppHost.WasmAppHost.Main(String[] args)
   at Microsoft.WebAssembly.AppHost.WasmAppHost.<Main>(String[] args)

Instead read the addresses on application lifetime's ApplicationStarted event.

Author: radical
Assignees: -
Labels:

arch-wasm, area-VM-meta-mono

Milestone: -

@radical radical changed the title [release/7.0] WasmAppHost: improve how we get the urls for the webserver [release/7.0] WasmAppHost: Fix crash when starting wasmbrowser project Sep 27, 2022
@radical radical marked this pull request as ready for review September 27, 2022 12:07
@pavelsavara
Copy link
Member

Also could we remove --forward-console option from the host ?
It's not doing anything right now. There is internal withConsoleForwarding() we use for WBT.

@pavelsavara
Copy link
Member

pavelsavara commented Sep 27, 2022

Also we had the issue with "unsafe ports" (as considered by browsers) on helix . Should we try to avoid those port numbers here too ?

@carlossanlop
Copy link
Member

@radical is this a product code change? If yes, can you please fill out the template in the description, add the servicing-consider label and then send an email to Tactics to request approval?

@radical
Copy link
Member Author

radical commented Sep 28, 2022

@carlossanlop approved over email

@radical radical added the Servicing-approved Approved for servicing release label Sep 28, 2022
@radical
Copy link
Member Author

radical commented Sep 28, 2022

The wasm/aot failure is #76110 .
And the other one is an unrelated timeout.

@carlossanlop
Copy link
Member

@radical some pending things:

  • Do you plan on addressing @pavelsavara 's suggestions in this PR?
  • Can you please resolve the merge conflicts?
  • Can you please get a code review sign-off when ready?

@radical
Copy link
Member Author

radical commented Sep 29, 2022

@radical some pending things:

  • Do you plan on addressing @pavelsavara 's suggestions in this PR?

Not in this PR.

  • Can you please resolve the merge conflicts?

Done

  • Can you please get a code review sign-off when ready?

cc @lewing @thaystg

@radical
Copy link
Member Author

radical commented Sep 29, 2022

@pavelsavara about the unsafe ports, is there a list of the ports that chrome would reject? And I guess, other browsers.

@pavelsavara
Copy link
Member

@pavelsavara about the unsafe ports, is there a list of the ports that chrome would reject? And I guess, other browsers.

Chrome
https://chromium.googlesource.com/chromium/src.git/+/refs/heads/master/net/base/port_util.cc#27

FF
https://www-archive.mozilla.org/projects/netlib/portbanning#portlist

Safari
https://github.com/WebKit/WebKit/blob/42f5a93823a7f087a800cd65c6bc0551dbeb55d3/Source/WTF/wtf/URL.cpp#L969

@radical
Copy link
Member Author

radical commented Sep 29, 2022

@carlossanlop this is ready to go!

@carlossanlop
Copy link
Member

Let's do this! 💪🏼 :shipit: 💪🏼

@carlossanlop carlossanlop merged commit 3c40805 into dotnet:release/7.0 Sep 29, 2022
@radical radical deleted the app-host-fixes-7 branch September 29, 2022 15:58
kasperk81 added a commit to kasperk81/templating that referenced this pull request Oct 6, 2022
extracted from three browsers' sources: dotnet/runtime#76236 (comment).
vlada-shubina pushed a commit to dotnet/templating that referenced this pull request Oct 10, 2022
* block all unsafe ports

extracted from three browsers' sources: dotnet/runtime#76236 (comment).

* fb

* sort
github-actions bot pushed a commit to dotnet/templating that referenced this pull request Oct 12, 2022
extracted from three browsers' sources: dotnet/runtime#76236 (comment).
vlada-shubina pushed a commit to dotnet/templating that referenced this pull request Oct 12, 2022
extracted from three browsers' sources: dotnet/runtime#76236 (comment).
@ghost ghost locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-VM-meta-mono Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants