JSRuntime.Invoke APIs aren't correctly annotated for trimming

[pranavkm]

Follow up to https://github.com/dotnet/aspnetcore/pull/39838. JSRuntime's API (excluding the unmarshalled ones) use JSON serialization to serialize the args array and as such are subject to being trimmed away. S.T.Js API for its serialization are annotated with RequiresUnreferencedCode with a recommendation to use the source-generator based overload to avoid this.

Blazor's APIs suppress STJ's warnings (since it happens deep in its bowels) and don't have a trimmer safe alternative. Perhaps one option is to annotate all of the JSRuntime APIs with RequiresUnferencedCode and add an JSRuntime.InvokeAsync overload that accepts exactly one argument a JsonTypeInfo to go with it. Something like so:

TValue IJSRuntime.InvokeAsync<TArg, TValue>(string method, TArg arg, JsonTypeInfo<TArg> t1,  JsonTypeInfo<TValue> t2, CancellationToken cancellationToken)

At the very least, we could require that all code in the framework uses this API for it's interop.

[pranavkm]

FYI @TanayParikh

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[yugabe]

I wanted to file a similar issue, as I essentially wanted to advise something very close to the one outlined here.

1) The IJSRuntime is incorrectly annotated for linking. The IJSRuntime.InvokeAsync methods need the [RequiresUnreferencedCode] attribute applied, as the IJSInProcessRuntime already does on the Invoke method.

---

2) I think there could easily be overloads which don't break trimming if the arguments passed are typed instead of objects.

public interface IJSRuntime
{
    // Existing signature, but this should also have [RequiresUnreferencedCode] as the "args" are dynamically referenced.
    ValueTask<TValue> InvokeAsync<[DynamicallyAccessedMembers(JsonSerialized)] TValue>(string identifier, object?[]? args);

    // Added overloads, with default implementation, doesn't break trimming (probably can use [UnconditionalSuppressMessage] or similar).
    ValueTask<TValue> InvokeAsync<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] TValue>(string identifier) => InvokeAsync<TValue>(identifier, null);
    ValueTask<TValue> InvokeAsync<[DynamicallyAccessedMembers(JsonSerialized)] TValue, [DynamicallyAccessedMembers(JsonSerialized)] TArg>(string identifier, TArg argument) => InvokeAsync<TValue>(identifier, new object?[] { argument });
    // And so on, for multiple arguments with <TArg1, TArg2>, up until maybe 3 or 4?

---

3) Additionally, I am not sure there exists sufficient documentation regarding how the trimming warnings should be fixed in these cases. As IJSRuntime is not correctly annotated, I guess people expect it to just work, but it will fail if the argument types aren't preserved. The question then (after fixing the annotations) becomes how to actually fix these issues in code, as [RequiresUnreferencedCode] will mitigate it to the caller (still the user) and [UnconditionalSuppressMessage] will just suppress it, but neither will actually solve the problem of actually keeping the correct types in the assemblies (although my point no. 2 above could solve that problem).

I hope @guardrex won't mind me pinging.

---

4) For 100% accuracy, I think a return type should correctly have DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.PublicProperties, as it is defined in https://github.com/dotnet/aspnetcore/blob/main/src/Shared/LinkerFlags.cs as JsonSerialized. But in fact, serialization requires only DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.PublicProperties (reading them, so only the get accessors), and only deserialization (so, return types) requires DynamicallyAccessedMemberTypes.PublicConstructors as well (and writing the properties, so set accessors).

This probably doesn't make too much difference in most cases, but it might trim out some unused public constructors, leaving us with a smaller assembly size in the end.

[TanayParikh]

@pavelsavara would this still be applicable once we update to using the new JS interop with [JSImportAttribute] to make it CSP friendly?

https://github.com/dotnet/aspnetcore/issues/37787#issuecomment-1238183412

[pavelsavara]

@pavelsavara would this still be applicable once we update to using the new JS interop with [JSImportAttribute] to make it CSP friendly?

#37787 (comment)

Yes, those are separate problems. Solving internal client side interop will fix CSP of Blazor as a framework. This is problem with IJSRuntime which will not go away by solving CSP.

If users adopted new runtime interop with JSImportAttribute instead of IJSRuntime it would help. But JSImportAttribute is not available on server side (hybrid Blazor), so it's not complete solution.

Adding new trimming friendly signatures to the IJSRuntime is way to go. And maybe marking the old signatures obsolete ?

I think that implementing Roslyn code analyzer to hint or force users to pass JsonTypeInfo or JsonSerializerContext would be good.

[yugabe]

Also wanted to mention there are some incorrect suppressions too, which can and do break functionality. I found one here, but I slightly remember more void-returning ones having this mistake: https://github.com/dotnet/aspnetcore/blob/1c385f463e488dc9a280fc24352740e919c6bc3f/src/JSInterop/Microsoft.JSInterop/src/JSInProcessObjectReferenceExtensions.cs#L20

The justification states "The method returns void, so nothing is deserialized.". Although the method indeed returns void, but the params object?[]? args arguments are passed to the method unchecked, which will be serialized dynamically. This can break if any of the arguments are trimmed away. It's also quite important, as other libraries do in fact depend on these APIs, like Msal, which has had its fair share of problems regarding trimming too.

A generic typed alternative would go a long way.

@pavelsavara I started to migrate to the new JSImport API, but it's not on feature parity with the current Blazor one which makes it very hard to migrate to. Disregarding JSON serialization (I understand it can be easily misused and can be a bottleneck), there is the in-box ElementReference and IJSObjectReference scenarios, the lack of the latter in the new API especially makes it hard to model JavaScript object graphs correctly, as there is no way to invoke functions on JSObject references (at least I haven't found any), and ElementReference can't be efficiently migrated to JSImport APIs either. It's a shame, as I saw some 30-50 times increase in some benchmarks. I think it would also be worth considering opening up some internal APIs, like System.Runtime.InteropServices.JavaScript.JSFunctionBinding to let end users call methods dynamically (instead of making custom workarounds which take "methodName" and other parameters).

Adding new trimming friendly signatures to the IJSRuntime is way to go. And maybe marking the old signatures obsolete ?

👍👍👍

[pavelsavara]

there is the in-box ElementReference and IJSObjectReference scenarios, the lack of the latter in the new API especially makes it hard to model JavaScript object graphs correctly, as there is no way to invoke functions on JSObject references

That's there on purpose, making OOP calls over interop boundary is usually bad design because those tend to be chatty interactions. Especially talking to DOM. Instead you are better off if you aggregate the logic into single JS method and call that.

Also for first version of the interop we limitted the scope. We started with low level and fast scenarios. We will collect feedback about use cases and see if they could be solved by code generators developed by the community or if there need to be runtime improvements.

That said, what is the specific use case in which you think it's good idea to call methods on JS instances ?

[yugabe]

That said, what is the specific use case in which you think it's good idea to call methods on JS instances ?

There isn't really a definite "gotcha", but obviously sometimes you need the context of the object to execute a function... That's how object-oriented programming was born. Yes, I can "simulate" object-oriented design by making global/static methods in JavaScript and passing the context object as its first parameter. But that defeats the whole purpose of object-oriented features in both JavaScript and C#. I'd rather not bind to JavaScript's this either, there's been quite a few years since that went out of fashion.

I'm not saying it's effectively a "good idea" to call methods on instances, but for example I create typed proxy instances for JavaScript managed instances for third party libraries. I store the Bootstrap plugin "ScrollSpy" instance instantiated along with my Blazor component, so their lifecycles are synchronized, and event handling can be delegated this way from the component through to the third party library. I could mess with lookup dictionaries on both sides and generated IDs, but that's just me writing the framework for something that could be in it anyway.

That's there on purpose, making OOP calls over interop boundary is usually bad design because those tend to be chatty interactions. Especially talking to DOM.

This is exactly right. But I will call the interaction anyways (because I need to), whether the framework provides the API for it or not. It's just harder to do and more inconvenient if there is no way to call methods directly on objects. If it's an artificial limitation, then it's just making harder for users to generalize code, or write dependent libraries.

If the user calls DOM queries with each method call in a loop, that's on them, their mistake. But preventing calling methods in object instances won't prevent this in any way, in my opinion. System.Runtime.InteropServices.JavaScript.JSFunctionBinding being internal won't stop me from forking it either, it's just a lot more troublesome and it would be nice if I didn't have to.

[ghost]

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.

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.