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

More support for wasm #178

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
+217 −61
Open

More support for wasm #178

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
68e6af1
fix: Using return values (wasm support)
koji-1009 Nov 30, 2024
e7d9517
test: Update getRandomValue test and support wasm error
koji-1009 Dec 1, 2024
2df83f9
feat: Use js_interop instead of html (wasm support)
koji-1009 Dec 1, 2024
3af9272
feat: Add wasm ci test and set timeout-minutes
koji-1009 Dec 1, 2024
e09f999
Merge branch 'master' into fix/support_wasm
koji-1009 Dec 3, 2024
88f1cc4
test: Stop separating jobs by strategy
koji-1009 Dec 3, 2024
9539b5f
fix: getRandomValues
koji-1009 Dec 3, 2024
bac32e9
feat: Unify the behavior of dart2js and dart2wasm
koji-1009 Dec 3, 2024
1a620f4
fix: Simplify
koji-1009 Dec 3, 2024
45ff7e3
feat: Create UnknownError class
koji-1009 Dec 3, 2024
b44ce75
test: fix
koji-1009 Dec 3, 2024
4e8e9de
refactor: Simplify UnknownError
koji-1009 Dec 4, 2024
7856777
refactor: Added processing branching by kIsWasm
koji-1009 Dec 4, 2024
9d36a73
fix: Show only kIsWasm
koji-1009 Dec 5, 2024
2d3497d
remove: Remove kIsWasm
koji-1009 Dec 5, 2024
e4e0e06
Update lib/src/crypto_subtle.dart
koji-1009 Dec 6, 2024
d27cadf
Apply suggestions from code review
koji-1009 Dec 6, 2024
aeeb3cc
fix: Use jsArray and dartArray
koji-1009 Dec 6, 2024
77991b6
fix: minor fix
koji-1009 Dec 6, 2024
b9f7954
test: Remove skip option
koji-1009 Dec 10, 2024
4efab3b
test: Add tests for various lists
koji-1009 Dec 10, 2024
69aa772
fix: Remove kIsWasm
koji-1009 Dec 10, 2024
bd9425a
fix: throw ArgumentError when type is not supported
koji-1009 Dec 10, 2024
d293552
chore: Check wasm coverage
koji-1009 Dec 10, 2024
4b13a9a
chore: Remove unused step
koji-1009 Dec 10, 2024
33aa079
chore: Remove wasm option from windows
koji-1009 Dec 10, 2024
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
4 changes: 4 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ jobs:
- run: flutter pub run webcrypto:setup
- run: flutter test
- run: flutter test --platform chrome
- run: flutter test --platform chrome --wasm
- run: xvfb-run flutter test integration_test/webcrypto_test.dart -d linux
working-directory: ./example
- uses: nanasess/setup-chromedriver@v2
Expand Down Expand Up @@ -69,6 +70,7 @@ jobs:
- run: flutter pub run webcrypto:setup
- run: flutter test
- run: flutter test --platform chrome
- run: flutter test --platform chrome --wasm
- run: flutter test integration_test/webcrypto_test.dart -d macos
working-directory: ./example
# TODO: Enable chromedriver testing on MacOS when it works reliably
Expand Down Expand Up @@ -105,6 +107,7 @@ jobs:
- run: flutter pub run webcrypto:setup
- run: flutter test
- run: flutter test --platform chrome
- run: flutter test --platform chrome --wasm
- run: flutter test integration_test/webcrypto_test.dart -d macos
working-directory: ./example
# TODO: Enable chromedriver testing on MacOS when it works reliably
Expand Down Expand Up @@ -134,6 +137,7 @@ jobs:
- run: flutter pub run webcrypto:setup
- run: flutter test
#- run: flutter test --platform chrome
#- run: flutter test --platform chrome --wasm
- run: flutter test integration_test/webcrypto_test.dart -d windows
working-directory: ./example
- uses: nanasess/setup-chromedriver@v2
Expand Down
42 changes: 25 additions & 17 deletions lib/src/crypto_subtle.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,29 +334,37 @@ extension type JSRsaOtherPrimesInfo(JSObject _) implements JSObject {

TypedData getRandomValues(TypedData array) {
if (array is Uint8List) {
window.crypto.getRandomValues(array.toJS);
return array;
final values = array.toJS;
window.crypto.getRandomValues(values);
array.setAll(0, values.toDart);
return values.toDart;
} else if (array is Uint16List) {
window.crypto.getRandomValues(array.toJS);
return array;
final values = array.toJS;
window.crypto.getRandomValues(values);
array.setAll(0, values.toDart);
return values.toDart;
} else if (array is Uint32List) {
window.crypto.getRandomValues(array.toJS);
return array;
final values = array.toJS;
window.crypto.getRandomValues(values);
array.setAll(0, values.toDart);
return values.toDart;
} else if (array is Int8List) {
window.crypto.getRandomValues(array.toJS);
return array;
final values = array.toJS;
window.crypto.getRandomValues(values);
array.setAll(0, values.toDart);
return values.toDart;
} else if (array is Int16List) {
window.crypto.getRandomValues(array.toJS);
return array;
final values = array.toJS;
window.crypto.getRandomValues(values);
array.setAll(0, values.toDart);
return values.toDart;
} else if (array is Int32List) {
window.crypto.getRandomValues(array.toJS);
return array;
final values = array.toJS;
window.crypto.getRandomValues(values);
array.setAll(0, values.toDart);
return values.toDart;
} else {
throw ArgumentError.value(
array,
'array',
'Unsupported TypedData type',
);
throw UnsupportedError('Unsupported TypedData type ${array.runtimeType}');
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added test case. 4efab3b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}

Expand Down
7 changes: 7 additions & 0 deletions lib/src/impl_js/impl_js.random.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,12 @@ void fillRandomBytes(TypedData destination) {
subtle.getRandomValues(destination);
} on subtle.JSDomException catch (e) {
throw _translateDomException(e);
} on Error catch (e) {
final errorName = e.toString();
if (errorName != 'JavaScriptError') {
rethrow;
}

throw _translateJavaScriptException(e);
Copy link
Member

Choose a reason for hiding this comment

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

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 🤣

Copy link
Member

Choose a reason for hiding this comment

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

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 🤣)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
}

Copy link
Member

Choose a reason for hiding this comment

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

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

}
}
7 changes: 7 additions & 0 deletions lib/src/impl_js/impl_js.utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,13 @@ Object _translateDomException(
'"${e.name}", message: $message');
}

Object _translateJavaScriptException(Error e) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

// dart2wasm throws _JavaScriptError, but _JavaScriptError is not exposed.
final errorMessage = e.toString();
final message = 'browser threw "$errorMessage"';
jonasfj marked this conversation as resolved.
Show resolved Hide resolved
return ArgumentError(message);
jonasfj marked this conversation as resolved.
Show resolved Hide resolved
}

/// Handle instances of [subtle.JSDomException] specified in the
/// [Web Cryptograpy specification][1].
///
Expand Down
55 changes: 33 additions & 22 deletions test/crypto_subtle_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,37 +63,48 @@ void main() {
data.every((e) => e == 0),
isTrue,
);
subtle.window.crypto.getRandomValues(data.toJS);
final values = data.toJS;
subtle.window.crypto.getRandomValues(values);
expect(
data.any((e) => e != 0),
values.toDart.any((e) => e != 0),
isTrue,
);
});

test('getRandomValues: too long', () {
expect(
() => subtle.window.crypto.getRandomValues(Uint8List(1000000).toJS),
throwsA(
isA<subtle.JSDomException>().having(
(e) => e.name,
'name',
'QuotaExceededError',
),
),
);
try {
subtle.window.crypto.getRandomValues(Uint8List(1000000).toJS);
} on subtle.JSDomException catch (e) {
// dart2js throws QuotaExceededError
expect(
e.name,
'QuotaExceededError',
);
} on Error catch (e) {
// dart2wasm throws JavaScriptError
expect(
e.toString(),
'JavaScriptError',
);
}
});

test('getRandomValues: not supported type', () {
expect(
() => subtle.window.crypto.getRandomValues(Float32List(32).toJS),
throwsA(
isA<subtle.JSDomException>().having(
(e) => e.name,
'name',
'TypeMismatchError',
),
),
);
try {
subtle.window.crypto.getRandomValues(Float32List(32).toJS);
} on subtle.JSDomException catch (e) {
// dart2js throws TypeMismatchError
expect(
e.name,
'TypeMismatchError',
);
} on Error catch (e) {
// dart2wasm throws JavaScriptError
expect(
e.toString(),
'JavaScriptError',
);
}
});
});

Expand Down
Loading