More support for wasm #178

koji-1009 · 2024-12-02T00:28:41Z

There are several related fixes.

In the wasm environment, if an invalid value is passed to getRandomValue, a JavaScriptError will be thrown. However, JavaScriptError is not public. For this reason, the Error class is catched.
~~Fixed getRandomValues argument values are not updated in the wasm environment, so return value is used.~~
Fetch userAgent via js_interop. This change can also be implemented by introducing package:web.
- feat: Replace dart:html with dart:js_interop #180
(I don't know the cause, this has been happening since Dec 1.) In windows-latest, if I run flutter pub run test twice, the first test for firefox fails the second time. To work around this issue and to reduce CI latency, I am separating the coverage jobs.
- chore: Split ci coverage job #179

jonasfj

🚀 Thanks for diving into this, I don't think neither me or @HamdaanAliQuatil has had much time to play with wasm yet.

I'm not sure exactly how to get the error handling, bit right, so let's explore it a bit -- see if there are any related bugs on the issue, etc.

My other suggestion would be to try and split as many of changes into tiny PRs. Those are much easier to land, and it'll make easier for us to focus on the complex issue in this PR.

.github/workflows/test.yml

jonasfj · 2024-12-02T11:00:42Z

lib/src/crypto_subtle.dart

-    window.crypto.getRandomValues(array.toJS);
-    return array;
+    final values = window.crypto.getRandomValues(array.toJS);
+    return (values as JSUint8Array).toDart;


Is there any need for this cast, doesn't getRandomValues always return the first argument that it is given?

jonasfj · 2024-12-02T11:01:41Z

lib/src/impl_js/impl_js.random.dart

-    subtle.getRandomValues(destination);
+    if (destination is Uint8List) {
+      final newValues = subtle.getRandomValues(destination);
+      destination.setAll(0, newValues as Uint8List);


What is this good for? getRandomValues writes to the first argument it is given, does it not?

It seems that dart2js overwrites the value, but dart2wasm does not. Could you please see that the getRandomValues test fails once as I will submit a PR later that runs with the --wasm option in ci?

doesn't that hint at a more fundamental issue.

If you read the documentation for getRandomValues it's very clear that it fills the first argument with random bytes:

https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues

It even says it's updated in-place:

If that's not working when compiling to wasm, then what is happening?

I'd suggest trying to make a minimal example. Because this shouldn't be happening, if we can't pass typedarrays correctly to JS from inside wasm, then something is very wrong.

I tried it and the following code works.
In the case of dart2js, the array.toJS reflected the value in the array, but not in the case of dart2wasm.

final values = array.toJS; window.crypto.getRandomValues(values); return values.toDart;

Can we make a gist that contains the smallest possible test case where:

$ dart test -p chrome -c dart2wasm

and

$ dart test -p chrome -c dart2js

behaves differently.

Then let's report a bug against the SDK and see if we can find a workaround.

If there really is a thing where modifications to a Uint8List passed to Javascript aren't reflected back in Dart that's pretty bad. And we'd need to review the entire code base for all places where we pass something to Javascript to check that we don't rely on side effects.

Ah, I didn't know that .toJS could return a clone.

We should probably review all places in the code where we do a .toJS call.

hmm, I guess we could work around this as follows:

import 'dart:js_interop'; import 'dart:typed_data'; import 'package:web/web.dart' as web; import 'package:test/test.dart'; void main() { test('calculate', () { final array = Uint8List(16); expect( array.any((e) => e == 0), isTrue, ); final jsArray = array.toJS; web.window.crypto.getRandomValues(jsArray); // .toJS may cause a clone, so we can't know if // jsArray.toDart is the same as the original array. final dartArray = jsArray.toDart; if (dartArray == array) { print('We do not need to do anything here'); } else { array.setAll(0, dartArray); } expect( array.any((e) => e != 0), isTrue, ); }); }

probably we can pick some better variable names. But there is no need to always do .setAll.

And we definitely need a comment explaining that .toJS will make a clone of the array when we're compiling with dart2wasm.

I think kIsWasm is a better flag. Because it is a const bool. I added comments and flags. 7856777

Oops, calling import 'package:flutter/foundation.dart' show kIsWasm; on crypto_subtle.dart causes an error.

We can just use bool.fromEnvironment('dart.tool.dart2wasm') it's the same 🤣

test/crypto_subtle_test.dart

jonasfj · 2024-12-02T11:16:36Z

lib/src/impl_js/impl_js.random.dart

+  } on UnsupportedError {
+    rethrow;
+  } on Error catch (e) {
+    throw _translateJavaScriptException(e);


I'm not necessarily convinced this is a good idea.

We don't want to catch errors that we're not supposed to catch.

Hmm, I'm not sure exactly how we should handle this.

I'm guessing it means that the _handleDomException logic we have also won't work as intended.

I wonder if it's:
(a) Possible to get access to the JSObject that was thrown (somehow).
(b) Is there a bug for how error handling differs between js and wasm.

I'm betting that we don't have good enough tests in this package for all the exception cases, hence, also why a lot of tests didn't start failing when _handleDomException doesn't work as expected anymore.

Maybe, we need to put the error handling logic that differs into crypto_suble.dart such that impl_js/ doesn't have to care whether it's running in dart2js or dart2wasm.

But I'm surprised that error handling differs between js and wasm, maybe we're doing something wrong 🤣

Thanks for referencing dart-lang/sdk#55496

I'd suggest we look at toString(), because the only error we want to handle here is one that is _JavaScriptError. We might even want to make this code not run when it's compiled with dart2js (not sure if we can detect wasm vs js, though 🤣)

I would modify the code as follows 👍
I wasn't expecting Error handling, so I was confused when implementing it. Thanks for the review.

} on Error catch (e) { final errorName = e.toString(); if (errorName != 'JavaScriptError') { rethrow; } throw _translateJavaScriptException(e); }

I think this is a good idea, we probably have to do that in _handleDomException and in getRandomValues inside lib/src/crypto_subtle.dart.

lib/src/testing/utils/detected_runtime_js_interop.dart

koji-1009 · 2024-12-02T11:31:09Z

Thanks for the review. I will make some PRs that can be easily separated and make this PR smaller.

dart-lang/sdk#55496
I was also surprised that the error is different between dart2wasm and dart2js. I would be very happy to have access to JavaScriptError, but it seems difficult.

jonasfj · 2024-12-03T14:44:15Z

lib/src/impl_js/impl_js.utils.dart

@@ -99,6 +99,13 @@ Object _translateDomException(
      '"${e.name}", message: $message');
 }

+Object _translateJavaScriptException(Error e) {


Let's give this a documentation comment.

Let's link to the issue in the SDK, that way when we discover that the issue is resolved, we can work to fix this hack.

lib/src/impl_js/impl_js.utils.dart

lib/src/crypto_subtle.dart

jonasfj · 2024-12-05T14:50:05Z

lib/src/crypto_subtle.dart

+    final values = array.toJS;
+    window.crypto.getRandomValues(values);
+    if (array != values.toDart) {
+      array.setAll(0, values.toDart);
+    }


Let's avoid calling .toDart twice, in wasm mode it'll create a wrapper, there is no need to create the wrapper twice.

Suggested change

final values = array.toJS;

window.crypto.getRandomValues(values);

if (array != values.toDart) {

array.setAll(0, values.toDart);

}

final jsarray = array.toJS;

window.crypto.getRandomValues(jsarray);

final dartArray = jsarray.toDart;

if (array != dartArray) {

array.setAll(0, dartArray);

}

See: https://api.dart.dev/dart-js_interop/JSUint8ArrayToUint8List/toDart.html

jonasfj · 2024-12-05T14:51:19Z

lib/src/crypto_subtle.dart

-      'array',
-      'Unsupported TypedData type',
-    );
+    throw UnsupportedError('Unsupported TypedData type ${array.runtimeType}');


Why are we changing behavior here?

Is this deliberate? If so should it not be orthogonal to this PR, and happen in a separate PR?

I noticed that when generating unsupported lists (e.g. Uint64List) in dart2js, an UnsupportedError is thrown. For example, the following test succeeds.

test('Uint64List: not supported type', () { expect( () => Uint64List(32), throwsA( isA<UnsupportedError>(), ), ); });

The test fails in dart2wasm.

There is no JS class corresponding to Uint64List. Therefore, when fillRandomBytes(Uint64List(32)) is called in dart2wasm, the process reaches this code.

I changed it to UnsupportedError to keep the process consistent between dart2js and dart2wasm. It is possible to split a PR.

I added test case. 4efab3b

And restore previous code. The addition of the test makes it easier to understand the scope of the impact. bd9425a

lib/src/impl_js/impl_js.utils.dart

jonasfj · 2024-12-05T14:54:39Z

test/crypto_subtle_test.dart

@@ -68,32 +73,60 @@ void main() {
        data.any((e) => e != 0),
        isTrue,
      );
-    });
+    }, skip: 'dart2wasm');


I'm curious, why do we need to skip this?

This is because with dart2wasm, the value is not reflected in the UInt8List. All data values are 0.

Co-authored-by: Jonas Finnemann Jensen <[email protected]>

koji-1009 added 4 commits December 2, 2024 08:07

fix: Using return values (wasm support)

68e6af1

test: Update getRandomValue test and support wasm error

e7d9517

feat: Use js_interop instead of html (wasm support)

2df83f9

feat: Add wasm ci test and set timeout-minutes

3af9272

koji-1009 changed the title ~~More support for wasm environment~~ More support for wasm Dec 2, 2024

jonasfj reviewed Dec 2, 2024

View reviewed changes

This was referenced Dec 2, 2024

chore: Split ci coverage job #179

Merged

feat: Replace dart:html with dart:js_interop #180

Merged

test: Modified to a more strict type #181

Merged

koji-1009 added 2 commits December 3, 2024 21:59

Merge branch 'master' into fix/support_wasm

e09f999

test: Stop separating jobs by strategy

88f1cc4

koji-1009 force-pushed the fix/support_wasm branch from 8ba379b to 12b0567 Compare December 3, 2024 13:21

fix: getRandomValues

9539b5f

koji-1009 force-pushed the fix/support_wasm branch from 12b0567 to 9539b5f Compare December 3, 2024 13:29

koji-1009 marked this pull request as ready for review December 3, 2024 13:45

koji-1009 added 2 commits December 3, 2024 23:04

feat: Unify the behavior of dart2js and dart2wasm

bac32e9

fix: Simplify

1a620f4

koji-1009 requested a review from jonasfj December 3, 2024 14:33

jonasfj reviewed Dec 3, 2024

View reviewed changes

koji-1009 added 2 commits December 4, 2024 00:52

feat: Create UnknownError class

45ff7e3

test: fix

b44ce75

jonasfj reviewed Dec 4, 2024

View reviewed changes

lib/src/impl_js/impl_js.utils.dart Outdated Show resolved Hide resolved

koji-1009 force-pushed the fix/support_wasm branch from 27c1bb3 to ee3ca75 Compare December 4, 2024 14:18

koji-1009 added 2 commits December 4, 2024 23:20

refactor: Simplify UnknownError

4e8e9de

refactor: Added processing branching by kIsWasm

7856777

koji-1009 force-pushed the fix/support_wasm branch from ee3ca75 to 7856777 Compare December 4, 2024 14:20

koji-1009 added 2 commits December 5, 2024 18:37

fix: Show only kIsWasm

9d36a73

remove: Remove kIsWasm

2d3497d

jonasfj reviewed Dec 5, 2024

View reviewed changes

koji-1009 and others added 6 commits December 6, 2024 21:30

Update lib/src/crypto_subtle.dart

e4e0e06

Co-authored-by: Jonas Finnemann Jensen <[email protected]>

Apply suggestions from code review

d27cadf

Co-authored-by: Jonas Finnemann Jensen <[email protected]>

fix: Use jsArray and dartArray

aeeb3cc

fix: minor fix

77991b6

test: Remove skip option

b9f7954

test: Add tests for various lists

4efab3b

koji-1009 requested a review from jonasfj December 10, 2024 10:44

koji-1009 added 5 commits December 10, 2024 19:50

fix: Remove kIsWasm

69aa772

fix: throw ArgumentError when type is not supported

bd9425a

chore: Check wasm coverage

d293552

chore: Remove unused step

4b13a9a

chore: Remove wasm option from windows

33aa079

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More support for wasm #178

More support for wasm #178

koji-1009 commented Dec 2, 2024 •

edited

Loading

jonasfj left a comment

jonasfj Dec 2, 2024

jonasfj Dec 2, 2024

koji-1009 Dec 2, 2024

jonasfj Dec 2, 2024

koji-1009 Dec 3, 2024

jonasfj Dec 3, 2024

jonasfj Dec 4, 2024

jonasfj Dec 4, 2024 •

edited

Loading

koji-1009 Dec 4, 2024

koji-1009 Dec 5, 2024

jonasfj Dec 5, 2024

jonasfj Dec 2, 2024

jonasfj Dec 2, 2024

koji-1009 Dec 2, 2024

jonasfj Dec 3, 2024

koji-1009 commented Dec 2, 2024

jonasfj Dec 3, 2024

jonasfj Dec 5, 2024

jonasfj Dec 5, 2024

koji-1009 Dec 6, 2024

koji-1009 Dec 10, 2024

koji-1009 Dec 10, 2024

jonasfj Dec 5, 2024

koji-1009 Dec 6, 2024

-    final values = array.toJS;
-    window.crypto.getRandomValues(values);
-    if (array != values.toDart) {
-      array.setAll(0, values.toDart);
-    }
+    final jsarray = array.toJS;
+    window.crypto.getRandomValues(jsarray);
+    final dartArray = jsarray.toDart;
+    if (array != dartArray) {
+      array.setAll(0, dartArray);
+    }

More support for wasm #178

Are you sure you want to change the base?

More support for wasm #178

Conversation

koji-1009 commented Dec 2, 2024 • edited Loading

jonasfj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasfj Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koji-1009 commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koji-1009 commented Dec 2, 2024 •

edited

Loading

jonasfj Dec 4, 2024 •

edited

Loading