Fix JS value to Dart conversion when receiving from a web socket

[osa1]

MessageEvent is a package:web type and data field is a JS value of type JSAny?.

The receiving end of the sink is here: https://github.com/dart-lang/sdk/blob/26107a319a7503deafee404e3462644a873e2920/pkg/vm_service/lib/src/vm_service.dart#L1795

This code currently does not expect to see JS objects, so passing a JSAny? to the sink breaks it.

The fix should be in the generating end rather than the receiving end: JSAny? is supposed to be an unboxed value, not a boxed Dart value. In an ideal world we shouldn't be able to pass it as a Dart object. So we call dartify and convert it to a Dart object.

This fixes DevTools when compiled to Wasm:

Thanks to @eyebrowsoffire and @mkustermann for help with debugging.

Also ping @srujzs for the issue with passing JSAny? as Object?.

[mkustermann]

Do we really need the if / else here? Can this be just

  void _innerListen(MessageEvent event) {
    final JSAny? eventData = event.data;
    _controller.local.sink.add(eventData.dartify());
  }

?

[osa1]

That changes how ArrayBuffer is handled as dartify will give you a ByteBuffer instead of a Uint8List.

Maybe package maintainers can comment on whether that's OK.

[osa1]

I've updated the type test.

I don't know why we want to convert ArrayBuffer (ByteBuffer in Dart) to Uint8List, it's not documented anywhere in this library as far as I can see. The receiver end of this stream in vm_service is here: https://github.com/dart-lang/sdk/blob/26107a319a7503deafee404e3462644a873e2920/pkg/vm_service/lib/src/vm_service.dart#L1795-L1807

It's expecting (1) String (2) List<int> (3) ByteData, but as far as I understand from https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/message_event and this library it will never receive a ByteData (JS DataView). Maybe it needs to handle ByteBuffer instead of ByteData?

[kevmoo]

SO EXCITED to see this wired up. @srujzs should absolutely look

[kevmoo]

I LOVE that the fix was so simple

[srujzs]

It looks like the fix is to dartify non-ArrayBuffers as well, which should only ever be a JS string iiuc. Is it worth adding an assert in an else condition to ensure that the result is a String? This may be useful to avoid passing in unexpected Dart values down.

[kevmoo]

🤷

[mkustermann]

It looks like the fix is to dartify non-ArrayBuffers as well, which should only ever be a JS string iiuc.

With @osa1 we were looking at this today and the browser API says MessageEvent.data can be:

• If the message type is "text", then this field is a string.
• If the message type is "binary" type, then the type of this property can be inferred from the binaryType of this socket:
  • ArrayBuffer if binaryType is "arraybuffer",
  • Blob if binaryType is "blob"

But it seemed the existing code somewhat assumed it's not going to get a Blob but only string or array buffer

[osa1]

But it seemed the existing code somewhat assumed it's not going to get a Blob but only string or array buffer

Expected JS type of binary messages is specified when constructing a web socket channel using this type:

web_socket_channel/lib/html.dart

Lines 177 to 184 in df096a9

	/// An enum for choosing what type [HtmlWebSocketChannel] emits for binary
	/// messages.
	class BinaryType {
	/// Tells the channel to emit binary messages as [Blob]s.
	static const blob = BinaryType._('blob', 'blob');
	/// Tells the channel to emit binary messages as [Uint8List]s.
	static const list = BinaryType._('list', 'arraybuffer');

as either Blob (I don't know what would be the Dart type for this) or ArrayBuffer (which this library converts to Uint8List).

When I searched for BinaryType (ag -w --dart BinaryType) I can't find any uses in SDK or devtools, so it seems to me like we use the default BinaryType which is ArrayBuffer. That's why the use site doesn't handle the Blob case.

(Relevant MDN: https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/message_event#event_properties)

So we can't assert here that the data is either JSArrayBuffer or a String, it can also be Blob.

We could assert that the data is one of JSArrayBuffer, JSString, or the JS type for Blob.

I think it makes sense to avoid dartify here (which will do more type tests than necessary), and explicitly cover only these three cases. If the data is not a string or ArrayBuffer we can assert that it's a Blob.

[osa1]

@srujzs how should we convert the Blob data to Dart? This can happen when you pass BinaryType.blob to HtmlWebSocketChannel.connect.

[srujzs]

If I'm interpreting the code in vm_service correctly here, it doesn't look like we handle Blobs at all. On dart2js and ddc, we have dart:html, which includes Blob as a natively intercepted type. None of the type checks in the vm_service should apply to this type. Blob is now in package:web as a @staticInterop type. It's a JS object which doesn't have an equivalent Dart type since it's not a typed array or any of the other JS types.

Since we don't handle Blobs here to begin with, the correct migration might be to assert that it's one of those three types, and in the case of Blobs, pass it directly to the vm_service and have it log that it's an unknown message type.

If we want, we can extract the contents of the Blob into a typed array and use that, but it doesn't look like the code was doing that before: https://developer.mozilla.org/en-US/docs/Web/API/Blob#extracting_data_from_a_blob.

[osa1]

"and in the case of Blobs, pass it directly to the vm_service"

Do you mean pass the dartify of Blob, or the JSValue for it?

I think it should be the former, but want to make sure.

[srujzs]

dartify shouldn't do anything for a Blob as it's not any of the primitive types or typed arrays, so either way should produce the same JSValue.

[osa1]

OK, in that case I think the current changes are fine?

[srujzs]

Yes, it's up to you if you want to assert the types of data, but I think the functional changes are fine.

[srujzs]

issue with passing JSAny? as Object?.

The general issue with disallowing this is Object? is a top-type. We discussed another possible top-type so that the two hierarchies truly are orthogonal, but there are several complications. For example, collections can no longer store JS values (which may be a plus for some). There are also language and front-end complications iirc.

With extension types, we can disallow assignment of JSAny to Object if we wanted due to the way the implements clause works, but we can't disallow assignment to Object?. I don't know if we want the first part if the second part can't be addressed, because it just seems weirdly inconsistent. I haven't landed that change yet though, so if we do want it, this would be a good time to disallow it.

All this being said, we can certainly make it a lint when we upcast to Object?. Unlike JSString as String, it's a valid cast on all backends, however. It just might not be want you want in most cases.

[mkustermann]

All this being said, we can certainly make it a lint when we upcast to Object?. Unlike JSString as String, it's a valid cast on all backends, however. It just might not be want you want in most cases.

If I understand this correctly, then the JS* classes will (on wasm) eventually be represented as inline classes with an externref member. Whenever such a JS* type flows into a top type, then it needs to be boxed (afaik JSValue is the actual box, right?) - otherwise just bad things happen, e.g. compiler crashes, ...

In the VM's C FFI we have an extra type checker (of sort) that will - for otherwise perfectly language compliant code - issue compile-time errors if it's not valid C FFI DSL code. Why not do something similar here? Whenever a JS* flows into top types (including into places of type parameters that have top type bounds), we issue a compile-time error.

[srujzs]

Kind of. externrefs are boxed to JSValue and then wrapped by the JS* extension types (whose representation types are naturally JSValue). So assigning it to an Object wouldn't break soundness since JSValue is still a Dart Object. We've discussed various alternatives to this like boxing only when flowing to Object, eliding the JSValue boxes, etc.

Can you point me to FFI's type checker where you check for flowing into top-types? I presumed catching all upcasts to top types is complicated, but I'd be curious to look at the implementation.

[mkustermann]

Kind of. externrefs are boxed to JSValue and then wrapped by the JS* extension types (whose representation types are naturally JSValue). So assigning it to an Object wouldn't break soundness since JSValue is still a Dart Object.

Ah, so it means JS values are always boxed in a JSValue (and other JS* inline classes + extensions on them are for convenience)? Though then I'm confused, as the problematic code in this PR looks like this:

  void _innerListen(MessageEvent event) {
    final eventData = event.data;
    Object? data;
    if (eventData.typeofEquals('object') &&
        (eventData as JSObject).instanceOfString('ArrayBuffer')) {
      data = (eventData as JSArrayBuffer).toDart.asUint8List();
    } else {
      data = event.data;  // <---XXX
    }
    _controller.local.sink.add(data);
  }

If what you say is true, then shouldn't we be adding a JSValue box to sink.add(data) here at XXX? Where it seems what actually happens is that data (when using dart2js) isn't a JSValue box around a javascript string but the javascript string itself.

(If all JS objects are immediately boxed into JSValue which are valid Dart objects and we have relaxed guarantees about identities of JSValue then our compilers could "unbox" those JSValues across method calls / in fields / etc and transparently box when flowing into top types - so doing this as a compiler optimization instead of something at compile-time)

Can you point me to FFI's type checker where you check for flowing into top-types? I presumed catching all upcasts to top types is complicated, but I'd be curious to look at the implementation.

The VM gets the valid type-checked Dart code from CFE in the form of kernel. In the VM target's modular transformations we then perform extra checks and issue compile-time errors if the checks don't succeed.

The checks e.g. check correspondence between FFI types and Dart types and various other properties of native definitions, native uses, ffi struct classes, .... See ffi/definitions.dart, ffi/use_sites.dart, ...

The FFI checks aren't related to things flowing into top types, but I'm suggesting we could do that for the the JS interop.

[srujzs]

Where it seems what actually happens is that data (when using dart2js) isn't a JSValue box around a javascript string but the javascript string itself.

Right, the JSValue discussion is purely for dart2wasm. JSValue as a concept doesn't quite exist in dart2js or DDC. In those compilers, we use a combination of leveraging the fact that primitives are JS values and the notion of interceptors.

For example:

• JSString is really just String under the hood because String is a JS string.
• JSObject are an "interceptor" under the hood. The key part here is that interceptors are not boxes. The value is still just the JS value, but dispatch on these values are handled specially. There's a useful doc here explaining them.

This (necessary) distinction between the compilers is where the source of the error comes from. This is partly why is, as, and various other runtime mechanisms are prone to error and I want to lock them down. It might help to look at my extension types CL that maps JS types to their reified types here to get an idea of what each JS type is under the hood on the different backends. In the current SDK, we do this with @staticInterop and custom erasure here.

we have relaxed guarantees about identities of JSValue

Ignoring identical, which I don't think is reasonable to make guarantees on, == forwards to the underlying JS value, so your observation that this could be a compiler optimization is spot on! That's the ideal plan.

The FFI checks aren't related to things flowing into top types, but I'm suggesting we could do that for the the JS interop.

Ah, got it. I think one of the things we need to investigate is whether these flow points are always observable in the kernel. Naturally, an explicit cast will be, but should implicit upcasts or assignments be harder to catch? There are more questions here, but that's the kind of stuff I'd want to look into. Long-term, we discussed doing similar things for the JS backends, but there's existing interop that might make this more complicated.

[eyebrowsoffire]

@srujzs Re: your comment here:

Ah, got it. I think one of the things we need to investigate is whether these flow points are always observable in the kernel. Naturally, an explicit cast will be, but should implicit upcasts or assignments be harder to catch?

I think if we are to have unboxed flow of externrefs in dart2wasm (which we don't have right now but Joshua was working on before he left) we will have to detect all the upcast cases and downcast cases and emit code that boxes and unboxes them respectively anyway if we want to support JSAny flowing into top types. Since we have to detect these cases anyway, it's probably actually less work to just never emit this boxing/unboxing code anyway and write a frontend pass that detects these cases and emits a compile error. The pass could be used by js and wasm backends, and would mean we'd never run into these sorts of dynamic craziness, and it would mean we would never have to emit bytecode that deals with upcasts of downcasts between JS types and Object/dynamic

[kevmoo]

Have we converged here? Can we land?

[kevmoo]

Friendly ping...any more discussion that needs to happen here? @osa1 ?

[kevmoo]

Land it!!!

…

On Thu, Dec 7, 2023, 09:02 Srujan Gaddam ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/html.dart <#298 (comment)> : > @@ -127,13 +127,9 @@ class HtmlWebSocketChannel extends StreamChannelMixin } void _innerListen(MessageEvent event) { - final eventData = event.data; - Object? data; - if (eventData.typeofEquals('object') && - (eventData as JSObject).instanceOfString('ArrayBuffer')) { - data = (eventData as JSArrayBuffer).toDart.asUint8List(); - } else { - data = event.data; + var data = event.data.dartify(); Yes, it's up to you if you want to assert the types of data, but I think the functional changes are fine. — Reply to this email directly, view it on GitHub <#298 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEFCVN557S76ZJ4P3NP2TYIHZCDAVCNFSM6AAAAABAA2DRHOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZQGY2DANZYHE> . You are receiving this because your review was requested.Message ID: ***@***.***>

[osa1]

As discussed yesterday at the dart2wasm meeting, it would be good to avoid dartify as dartify handles more cases than necessary and it'll prevent tree shaking of some JS interop classes. I will replace the dartify call with type tests to explicitly handle the possibilities.

[osa1]

I've updated the conversion to avoid using dartify. @srujzs ptal.

We should document what kind of values this library can write to the channel, but in a separate PR.

[srujzs]

Changes to use type-checks instead LGTM!