Skip to content

Eliminate framework use of 'IJSUnmarshalledRuntime' #46693

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

Jump to bottom
Merged
merged 12 commits into from
Mar 7, 2023
Merged

Eliminate framework use of 'IJSUnmarshalledRuntime' #46693

merged 12 commits into from
Mar 7, 2023

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Feb 16, 2023 •
edited
Loading

Eliminate framework use of IJSUnmarshalledRuntime

Removes internal usage of IJSUnmarshalledRuntime to in favor of [JSImport]-style JavaScript interop.

Description

  • Added an IInternalJSImportMethods interface that gives us a layer of abstraction over direct calls to [JSImport] methods:
    • Added TestInternalJSImportMethods and updated tests to utilize it
    • Note: The [JSImport] methods that only make sense in an E2E context do not appear in IInternalJSImportMethods
  • Updated usage of IJSUnmarshalledRuntime for:
    • Navigation (WebAssemblyNavigationManager and WebAssemblyNavigationInterception)
    • Application startup (WebAssemblyHostBuilder)
    • Hot reload (WebAssemblyHotReload)
    • JSON-based interop (InternalCalls and WebAssemblyJSRuntime)
      • Also improved byte array transfer by removing the existing extra interop call.
    • Rendering (WebAssemblyRenderer)

Contributes to #37787 and #46673

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Feb 16, 2023
@MackinnonBuck MackinnonBuck marked this pull request as ready for review February 22, 2023 23:31
@MackinnonBuck MackinnonBuck requested a review from a team as a code owner February 22, 2023 23:31
@MackinnonBuck MackinnonBuck requested review from pavelsavara and SteveSandersonMS and removed request for a team February 22, 2023 23:31
pavelsavara
pavelsavara reviewed Feb 23, 2023
View reviewed changes
Copy link
Member

@pavelsavara pavelsavara left a comment

Choose a reason for hiding this comment

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

Note there are some E2E test failures. I have not looked into details yet.

src/Components/WebAssembly/WebAssembly/src/Services/DefaultWebAssemblyJSRuntime.cs
public static void BeginInvokeDotNet(string callId, string assemblyNameOrDotNetObjectId, string methodIdentifier, string argsJson)
[JSExport]
[SupportedOSPlatform("browser")]
public static void BeginInvokeDotNet(string? callId, string assemblyNameOrDotNetObjectId, string methodIdentifier, string argsJson)
Copy link
Member

Choose a reason for hiding this comment

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

As a next step (could be next PR), this could be replaced by promise/Task instead of Begin/End pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Yes that would be good. We did the begin/end thing because it predated support for Task in the WebAssembly runtime's interop mechanism.

src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs
{
#pragma warning disable CS0618 // Type or member is obsolete
var componentsCount = jsRuntime.InvokeUnmarshalled<int>(RegisteredComponentsInterop.GetRegisteredComponentsCount);
var componentsCount = jsMethods.RegisteredComponents_GetRegisteredComponentsCount();
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this is perf critical but if so, it could be re-implemented as passing int[] Ids, string[] assemblies etc.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. However it's only a one-time thing that happens during startup, so my guess is that we wouldn't be able to observe any difference.

src/Components/Web.JS/src/Boot.WebAssembly.ts
@@ -5,14 +5,14 @@
import { DotNet } from '@microsoft/dotnet-js-interop';
import { Blazor } from './GlobalExports';
import * as Environment from './Environment';
import { byteArrayBeingTransferred, Module, BINDING, monoPlatform } from './Platform/Mono/MonoPlatform';
import { Module, BINDING, monoPlatform } from './Platform/Mono/MonoPlatform';
Copy link
Member

Choose a reason for hiding this comment

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

In next PR we could try re-invent how the goal of invokeJSFromDotNet could be achieved otherwise.
To ged rid of things like BINDING.js_string_to_mono_string

@pavelsavara pavelsavara added the feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly label Feb 23, 2023
@pavelsavara pavelsavara added this to the 8.0 milestone Feb 23, 2023
SteveSandersonMS
SteveSandersonMS reviewed Feb 23, 2023
View reviewed changes
src/Components/test/E2ETest/Tests/InteropTest.cs
["ThrowException"] = @"""System.InvalidOperationException: Threw an exception!",
["ExceptionFromSyncMethod"] = "Function threw an exception!",
["invokeDisposedJSObjectReferenceException"] = @"""Error: JS object instance with ID",
["ThrowException"] = @"""Threw an exception!",
Copy link
Member

Choose a reason for hiding this comment

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

Is the exception type name still exposed on the JS side somehow? Seems like that would be valuable for debugging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, it looks like that information is lost now. The JavaScript errors that get created from exceptions thrown in [JSExport] methods have a message property equivalent to the C# exception's Message property, which excludes the exception type and stack trace. We could fix this by catching exceptions on the .NET side and wrapping them in a new Exception to simulate the old behavior, like:

try
{
    return DotNetDispatcher.Invoke(Instance, callInfo, argsJson);
}
catch (Exception ex)
{
    throw new Exception(ex.ToString(), ex);
}

Or I wonder if there's a way to address this on the runtime side, perhaps by updating ManagedError to include information about the type of exception that got thrown?

Copy link
Member

Choose a reason for hiding this comment

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

They are just a proxy and could also cross the boundary back to managed.
Collecting stack trace is very very expensive and we do it when you call toString() on it (I think).

Copy link
Member

Choose a reason for hiding this comment

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

Collecting stack trace is very very expensive and we do it when you call toString() on it (I think)

@pavelsavara Does that mean we could change the JS-side code to call throw new Error(exceptionProxy.toString()), and thereby get back the information we had before? Or is there some other mechanism you'd recommend?

Copy link
Member

Choose a reason for hiding this comment

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

no, please don't do that. It would be slow as you would pay the cost of stack trace creation also for exceptions which would be caught and handled silently.

We override the implementation of ManagedError.stack property to provide it on demand.

https://github.com/dotnet/runtime/blob/8d938ec1d18de3211040bccca1ea73f2574dc87a/src/mono/wasm/runtime/marshal.ts#L339

I think error.toString() and console.log(error) would both include stack property (at least on chrome). I'm open to suggestions how to improve it.

Proxy in the other direction is also lazy

https://github.com/dotnet/runtime/blob/8d938ec1d18de3211040bccca1ea73f2574dc87a/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSException.cs#L40

@SteveSandersonMS
Copy link
Member

Note there are some E2E test failures. I have not looked into details yet.

If the E2E tests still pass locally but are timing out in CI, that's likely an unrelated issue which we're currently investigating, and you don't need to block this PR on it. TLDR is that the CI machines auto-updated to chromedriver 110, and we don't yet know why, but this suffers connection issues that were not present in chromedriver 109. Please verify locally though!

@SteveSandersonMS SteveSandersonMS self-requested a review February 23, 2023 11:12
SteveSandersonMS
SteveSandersonMS approved these changes Feb 23, 2023
View reviewed changes
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Looks great! I really like the improved simplicity.

@MackinnonBuck
Copy link
Member Author

I've validated the hot reload changes locally.

@MackinnonBuck MackinnonBuck modified the milestones: 8.0, 8.0-preview3 Feb 28, 2023
@MackinnonBuck MackinnonBuck self-assigned this Feb 28, 2023
@pavelsavara pavelsavara mentioned this pull request Mar 3, 2023
Merged
@MackinnonBuck
Copy link
Member Author

I'm going to merge this now and address improvements in a follow-up PR.

@MackinnonBuck MackinnonBuck merged commit 8fbf7f2 into main Mar 7, 2023
@MackinnonBuck MackinnonBuck deleted the mbuck/jsimport-internal-interop branch March 7, 2023 00:29
@ctigrisht
Copy link

I'm going to merge this now and address improvements in a follow-up PR.

Hey, is this available in the latest .NET 8 preview? I want to be able to use CSP security in blazor wasm, thanks.

@ghost
Copy link

ghost commented Apr 7, 2023

Hi @ctigrisht. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@maraf maraf mentioned this pull request Apr 25, 2023
Closed
@LostBeard LostBeard mentioned this pull request May 17, 2023
Closed
1 task
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pavelsavara pavelsavara pavelsavara left review comments

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

Assignees

@MackinnonBuck MackinnonBuck

Labels
area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly
Projects
None yet
Milestone
8.0-preview3
Development

Successfully merging this pull request may close these issues.
4 participants
@MackinnonBuck @SteveSandersonMS @ctigrisht @pavelsavara