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.

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

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

Merged
merged 10 commits into from
Dec 9, 2023
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1f34c30
Fix JS value to Dart conversion when receiving from a web socket
osa1 Nov 30, 2023
78387a6
Address comment
osa1 Nov 30, 2023
1916b79
Remove type annotation
osa1 Nov 30, 2023
10ec2ca
Update byte buffer to uint8list conversion
osa1 Nov 30, 2023
6bbe316
Merge remote-tracking branch 'origin/master' into osa1/js_value_fix
osa1 Nov 30, 2023
3bdf975
Add fix to allow tests to pass with Wasm!
kevmoo Dec 1, 2023
28b48f2
oops
kevmoo Dec 1, 2023
08fdebf
Fix Wasm spelling
osa1 Dec 1, 2023
a5011a2
Update conversion
osa1 Dec 8, 2023
31db374
Convert ArrayBuffer to Uint8List
osa1 Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions lib/html.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

🤷

Copy link
Member

Choose a reason for hiding this comment

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

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:

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

"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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

if (data is ByteBuffer) {
data = data.asUint8List();
}
_controller.local.sink.add(data);
}
Expand Down
11 changes: 9 additions & 2 deletions test/html_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,19 @@ import 'dart:js_interop';
import 'dart:typed_data';

import 'package:async/async.dart';
import 'package:stream_channel/stream_channel.dart';
osa1 marked this conversation as resolved.
Show resolved Hide resolved
import 'package:test/test.dart';
import 'package:web/helpers.dart' hide BinaryType;
import 'package:web_socket_channel/html.dart';
import 'package:web_socket_channel/src/web_helpers.dart';
import 'package:web_socket_channel/web_socket_channel.dart';

extension on StreamChannel {
/// Handles the WASM case where the runtime type is actually [double] instead
/// of the JS case where it's [int].
Future<int> get firstAsInt async => ((await stream.first) as num).toInt();
}

void main() {
late int port;
setUpAll(() async {
Expand All @@ -35,7 +42,7 @@ void main() {
}
''', stayAlive: true);

port = await channel.stream.first as int;
port = await channel.firstAsInt;
});

test('communicates using an existing WebSocket', () async {
Expand Down Expand Up @@ -169,7 +176,7 @@ void main() {
// TODO(nweiz): Make this channel use a port number that's guaranteed to be
// invalid.
final channel = HtmlWebSocketChannel.connect(
'ws://localhost:${await serverChannel.stream.first}');
'ws://localhost:${await serverChannel.firstAsInt}');
expect(channel.ready, throwsA(isA<WebSocketChannelException>()));
expect(channel.stream.toList(), throwsA(isA<WebSocketChannelException>()));
});
Expand Down