Change SubtleCrypto.generateKey return type [chromium/src : main]
Hubert Chao (Gerrit)
Hubert Chao added 1 comment![]( "Open in Gerrit")
Questions:
1. Is this the right way to do this? this seems like a lot of boilerplate but maybe that's due to WebCrypto's structure?
2. I couldn't get rid of the casts to Any in crypto_result_impl.cc because a bunch of tests in V8ScriptValueSerializerForModulesTest failed; I tried fixing them but I didn't follow the templated stuff well enough (starting with SubtleCryptoSync) to figure out what a possible fix might be. Any ideas?
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Questions:
1. Is this the right way to do this? this seems like a lot of boilerplate but maybe that's due to WebCrypto's structure?
2. I couldn't get rid of the casts to Any in crypto_result_impl.cc because a bunch of tests in V8ScriptValueSerializerForModulesTest failed; I tried fixing them but I didn't follow the templated stuff well enough (starting with SubtleCryptoSync) to figure out what a possible fix might be. Any ideas?
@jap...@chromium.org can you help answer these questions? I'm not familiar enough with the bindings anymore.
Related details
- Hubert Chao
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
Daniel ChengQuestions:
1. Is this the right way to do this? this seems like a lot of boilerplate but maybe that's due to WebCrypto's structure?
2. I couldn't get rid of the casts to Any in crypto_result_impl.cc because a bunch of tests in V8ScriptValueSerializerForModulesTest failed; I tried fixing them but I didn't follow the templated stuff well enough (starting with SubtleCryptoSync) to figure out what a possible fix might be. Any ideas?
@jap...@chromium.org can you help answer these questions? I'm not familiar enough with the bindings anymore.
1. Yeah, sorry. When I went through and typed all the promises, to ensure that spec-defined promises always resolve with an IDL-compatible type, there were only a couple of places where the boilerplate got bad. WebCrypo was just about the worst in the entire codebase.
2. Can you give me an example error? I might be able to spot something :/
Related details
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao added 1 comment![]( "Open in Gerrit")
Daniel ChengQuestions:
1. Is this the right way to do this? this seems like a lot of boilerplate but maybe that's due to WebCrypto's structure?
2. I couldn't get rid of the casts to Any in crypto_result_impl.cc because a bunch of tests in V8ScriptValueSerializerForModulesTest failed; I tried fixing them but I didn't follow the templated stuff well enough (starting with SubtleCryptoSync) to figure out what a possible fix might be. Any ideas?
Nate Chapin@jap...@chromium.org can you help answer these questions? I'm not familiar enough with the bindings anymore.
1. Yeah, sorry. When I went through and typed all the promises, to ensure that spec-defined promises always resolve with an IDL-compatible type, there were only a couple of places where the boilerplate got bad. WebCrypo was just about the worst in the entire codebase.
2. Can you give me an example error? I might be able to spot something :/
1. Yeah, sorry. When I went through and typed all the promises, to ensure that spec-defined promises always resolve with an IDL-compatible type, there were only a couple of places where the boilerplate got bad. WebCrypo was just about the worst in the entire codebase.
Yeah there are way too many layers of abstraction; @davi...@chromium.org and I keep complaining about them. Maybe someday one of us will get annoyed enough and have the cycles to do something about it.
2. Can you give me an example error? I might be able to spot something :/
You can see the errors from the checks in patchset 3. Example:
```
31892:431892:FATAL:third_party/blink/renderer/modules/crypto/crypto_result_impl.cc:216] Check failed: type_ == ResolverType::kTyped (0 vs. 1).
#0 0x5db842d866f2 base::debug::CollectStackTrace()
#1 0x5db842d6ccb1 base::debug::StackTrace::StackTrace()
#2 0x5db842c481cc logging::LogMessage::Flush()
#3 0x5db842c4809c logging::LogMessage::~LogMessage()
#4 0x5db842c30a42 logging::(anonymous namespace)::CheckLogMessage::~CheckLogMessage()
#5 0x5db842c305bb logging::CheckNoreturnError::~CheckNoreturnError()
#6 0x5db84940ae00 blink::CryptoResultImpl::CompleteWithKeyPair()
#7 0x5db846f4f2f1 blink::WebCryptoResult::CompleteWithKeyPair()
#8 0x5db84a5680b3 webcrypto::GenerateKeyResult::Complete()
#9 0x5db84a55c19d webcrypto::WebCryptoImpl::GenerateKey()
#10 0x5db8380b72a6 blink::(anonymous namespace)::SyncGenerateKeyPair()
#11 0x5db8380bcdbf blink::(anonymous namespace)::V8ScriptValueSerializerForModulesTest_RoundTripCryptoKeyX25519_Test::TestBody()
```
Specifically there's something off in the template code that starts with `SyncGenerateKeyPair`/`SyncGenerateKey`[1] in
third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules_test.cc that i wasn't able to figure out
Related details
- Daniel Cheng
- David Benjamin
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
Yeah, it's ugly. I think these diffs will get it working on PS6.
```
diff --git a/third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules_test.cc b/third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules_test.cc
index 947e3b296e138..8c575643cf5ef 100644
--- a/third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules_test.cc
+++ b/third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules_test.cc
@@ -31,12 +31,14 @@
#include "third_party/blink/renderer/bindings/modules/v8/v8_browser_capture_media_stream_track.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_crop_target.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_crypto_key.h"
+#include "third_party/blink/renderer/bindings/modules/v8/v8_crypto_key_pair.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_dom_file_system.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_media_stream_track.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_restriction_target.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_rtc_certificate.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_rtc_data_channel.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_rtc_data_channel_state.h"
+#include "third_party/blink/renderer/bindings/modules/v8/v8_union_cryptokey_cryptokeypair.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_video_frame.h"
#include "third_party/blink/renderer/core/dom/dom_exception.h"
#include "third_party/blink/renderer/core/frame/local_frame.h"
@@ -276,25 +278,9 @@ TEST(V8ScriptValueSerializerForModulesTest, DecodeInvalidRTCCertificate) {
// A bunch of voodoo which allows the asynchronous WebCrypto operations to be
// called synchronously, with the resulting JavaScript values extracted.
-using CryptoKeyPair = std::pair<CryptoKey*, CryptoKey*>;
-
template <typename T>
T ConvertCryptoResult(v8::Isolate*, const ScriptValue&);
-template <>
-CryptoKey* ConvertCryptoResult<CryptoKey*>(v8::Isolate* isolate,
- const ScriptValue& value) {
- return V8CryptoKey::ToWrappable(isolate, value.V8Value());
-}
-template <>
-CryptoKeyPair ConvertCryptoResult<CryptoKeyPair>(v8::Isolate* isolate,
- const ScriptValue& value) {
- Dictionary dictionary(isolate, value.V8Value(), ASSERT_NO_EXCEPTION);
- v8::Local<v8::Value> private_key, public_key;
- EXPECT_TRUE(dictionary.Get("publicKey", public_key));
- EXPECT_TRUE(dictionary.Get("privateKey", private_key));
- return std::make_pair(V8CryptoKey::ToWrappable(isolate, public_key),
- V8CryptoKey::ToWrappable(isolate, private_key));
-}
+
template <>
DOMException* ConvertCryptoResult<DOMException*>(v8::Isolate* isolate,
const ScriptValue& value) {
@@ -333,6 +319,11 @@ class WebCryptoResultAdapter
void React(ScriptState* script_state, CryptoKey* crypto_key) {
function_.Run(crypto_key);
}
+ template <typename I = IDLType>
+ requires(std::is_same_v<I, V8UnionCryptoKeyOrCryptoKeyPair>)
+ void React(ScriptState* script_state, V8UnionCryptoKeyOrCryptoKeyPair* crypto_union) {
+ function_.Run(crypto_union);
+ }
template <typename I = IDLType>
requires(std::is_same_v<I, DOMArrayBuffer>)
void React(ScriptState* script_state, DOMArrayBuffer* buffer) {
@@ -384,22 +375,14 @@ T SubtleCryptoSync(V8TestingScope& scope, PMF func, Args&&... args) {
return result;
}
-CryptoKey* SyncGenerateKey(V8TestingScope& scope,
+V8UnionCryptoKeyOrCryptoKeyPair* SyncGenerateKey(V8TestingScope& scope,
const WebCryptoAlgorithm& algorithm,
bool extractable,
WebCryptoKeyUsageMask usages) {
- return SubtleCryptoSync<CryptoKey*, IDLAny>(scope, &WebCrypto::GenerateKey,
+ return SubtleCryptoSync<V8UnionCryptoKeyOrCryptoKeyPair*, V8UnionCryptoKeyOrCryptoKeyPair>(scope, &WebCrypto::GenerateKey,
algorithm, extractable, usages);
}
-CryptoKeyPair SyncGenerateKeyPair(V8TestingScope& scope,
- const WebCryptoAlgorithm& algorithm,
- bool extractable,
- WebCryptoKeyUsageMask usages) {
- return SubtleCryptoSync<CryptoKeyPair, IDLAny>(
- scope, &WebCrypto::GenerateKey, algorithm, extractable, usages);
-}
-
CryptoKey* SyncImportKey(V8TestingScope& scope,
WebCryptoKeyFormat format,
std::vector<unsigned char> data,
@@ -470,7 +453,7 @@ TEST(V8ScriptValueSerializerForModulesTest, RoundTripCryptoKeyAES) {
WebCryptoAlgorithm algorithm(kWebCryptoAlgorithmIdAesCbc, std::move(params));
CryptoKey* key =
SyncGenerateKey(scope, algorithm, true,
- kWebCryptoKeyUsageEncrypt | kWebCryptoKeyUsageDecrypt);
+ kWebCryptoKeyUsageEncrypt | kWebCryptoKeyUsageDecrypt)->GetAsCryptoKey();
// Round trip it and check the visible attributes.
v8::Local<v8::Value> wrapper =
@@ -545,7 +528,7 @@ TEST(V8ScriptValueSerializerForModulesTest, RoundTripCryptoKeyHMAC) {
std::move(generate_key_params));
CryptoKey* key =
SyncGenerateKey(scope, generate_key_algorithm, true,
- kWebCryptoKeyUsageSign | kWebCryptoKeyUsageVerify);
+ kWebCryptoKeyUsageSign | kWebCryptoKeyUsageVerify)->GetAsCryptoKey();
// Round trip it and check the visible attributes.
v8::Local<v8::Value> wrapper =
@@ -618,11 +601,11 @@ TEST(V8ScriptValueSerializerForModulesTest, RoundTripCryptoKeyRSAHashed) {
new WebCryptoRsaHashedKeyGenParams(hash, 1024, {1, 0, 1}));
WebCryptoAlgorithm generate_key_algorithm(kWebCryptoAlgorithmIdRsaPss,
std::move(generate_key_params));
- CryptoKey* public_key;
- CryptoKey* private_key;
- std::tie(public_key, private_key) =
- SyncGenerateKeyPair(scope, generate_key_algorithm, true,
- kWebCryptoKeyUsageSign | kWebCryptoKeyUsageVerify);
+ CryptoKeyPair* key_pair =
+ SyncGenerateKey(scope, generate_key_algorithm, true,
+ kWebCryptoKeyUsageSign | kWebCryptoKeyUsageVerify)->GetAsCryptoKeyPair();
+ CryptoKey* public_key = key_pair->publicKey();
+ CryptoKey* private_key = key_pair->privateKey();
// Round trip the private key and check the visible attributes.
v8::Local<v8::Value> wrapper =
@@ -713,11 +696,11 @@ TEST(V8ScriptValueSerializerForModulesTest, RoundTripCryptoKeyEC) {
new WebCryptoEcKeyGenParams(kWebCryptoNamedCurveP256));
WebCryptoAlgorithm generate_key_algorithm(kWebCryptoAlgorithmIdEcdsa,
std::move(generate_key_params));
- CryptoKey* public_key;
- CryptoKey* private_key;
- std::tie(public_key, private_key) =
- SyncGenerateKeyPair(scope, generate_key_algorithm, true,
- kWebCryptoKeyUsageSign | kWebCryptoKeyUsageVerify);
+ CryptoKeyPair* key_pair =
+ SyncGenerateKey(scope, generate_key_algorithm, true,
+ kWebCryptoKeyUsageSign | kWebCryptoKeyUsageVerify)->GetAsCryptoKeyPair();
+ CryptoKey* public_key = key_pair->publicKey();
+ CryptoKey* private_key = key_pair->privateKey();
// Round trip the private key and check the visible attributes.
v8::Local<v8::Value> wrapper =
@@ -798,11 +781,11 @@ TEST(V8ScriptValueSerializerForModulesTest, RoundTripCryptoKeyEd25519) {
// Generate an Ed25519 key pair.
WebCryptoAlgorithm generate_key_algorithm(kWebCryptoAlgorithmIdEd25519,
nullptr);
- CryptoKey* public_key;
- CryptoKey* private_key;
- std::tie(public_key, private_key) =
- SyncGenerateKeyPair(scope, generate_key_algorithm, true,
- kWebCryptoKeyUsageSign | kWebCryptoKeyUsageVerify);
+ CryptoKeyPair* key_pair =
+ SyncGenerateKey(scope, generate_key_algorithm, true,
+ kWebCryptoKeyUsageSign | kWebCryptoKeyUsageVerify)->GetAsCryptoKeyPair();
+ CryptoKey* public_key = key_pair->publicKey();
+ CryptoKey* private_key = key_pair->privateKey();
// Round trip the private key and check the visible attributes.
v8::Local<v8::Value> wrapper =
@@ -878,9 +861,11 @@ TEST(V8ScriptValueSerializerForModulesTest, RoundTripCryptoKeyX25519) {
// Generate an X25519 key pair.
WebCryptoAlgorithm generate_key_algorithm(kWebCryptoAlgorithmIdX25519,
nullptr);
- auto [public_key, private_key] = SyncGenerateKeyPair(
- scope, generate_key_algorithm, true,
- kWebCryptoKeyUsageDeriveKey | kWebCryptoKeyUsageDeriveBits);
+ CryptoKeyPair* key_pair =
+ SyncGenerateKey(scope, generate_key_algorithm, true,
+ kWebCryptoKeyUsageDeriveKey | kWebCryptoKeyUsageDeriveBits)->GetAsCryptoKeyPair();
+ CryptoKey* public_key = key_pair->publicKey();
+ CryptoKey* private_key = key_pair->privateKey();
// Round trip the private key and check the visible attributes.
v8::Local<v8::Value> wrapper =
diff --git a/third_party/blink/renderer/modules/crypto/crypto_result_impl.cc b/third_party/blink/renderer/modules/crypto/crypto_result_impl.cc
index 4fa99722728f1..97e9a26553fe0 100644
--- a/third_party/blink/renderer/modules/crypto/crypto_result_impl.cc
+++ b/third_party/blink/renderer/modules/crypto/crypto_result_impl.cc
@@ -198,13 +198,8 @@ void CryptoResultImpl::CompleteWithKeyForGenerateKey(const WebCryptoKey& key) {
}
auto* result = MakeGarbageCollected<CryptoKey>(key);
- if (type_ == ResolverType::kTyped) {
- resolver_->DowncastTo<V8UnionCryptoKeyOrCryptoKeyPair>()->Resolve(result);
- } else {
- ScriptState* script_state = resolver_->GetScriptState();
- ScriptState::Scope scope(script_state);
- resolver_->DowncastTo<IDLAny>()->Resolve(result->ToV8(script_state));
- }
+ CHECK_EQ(type_, ResolverType::kTyped);
+ resolver_->DowncastTo<V8UnionCryptoKeyOrCryptoKeyPair>()->Resolve(result);
ClearResolver();
}
@@ -219,13 +214,8 @@ void CryptoResultImpl::CompleteWithKeyPairForGenerateKey(
result->setPublicKey(MakeGarbageCollected<CryptoKey>(public_key));
result->setPrivateKey(MakeGarbageCollected<CryptoKey>(private_key));
- if (type_ == ResolverType::kTyped) {
- resolver_->DowncastTo<V8UnionCryptoKeyOrCryptoKeyPair>()->Resolve(result);
- } else {
- ScriptState* script_state = resolver_->GetScriptState();
- ScriptState::Scope scope(script_state);
- resolver_->DowncastTo<IDLAny>()->Resolve(result->ToV8(script_state));
- }
+ CHECK_EQ(type_, ResolverType::kTyped);
+ resolver_->DowncastTo<V8UnionCryptoKeyOrCryptoKeyPair>()->Resolve(result);
ClearResolver();
}
```
Related details
- Daniel Cheng
- David Benjamin
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |