CryptoAlgorithm::generateKey() and its overrides always expect to receive a non-null pointer to a CryptoAlgorithmParameter object and never take ownership of it. Currently the CryptoAlgorithmParameter object is passed as a lvalue reference to a std::unique_ptr<CryptoAlgorithmParameter> const. It is sufficient to pass it as a lvalue reference to a CryptoAlgorithmParameter const.
Created attachment 295528 [details] Patch
Attachment 295528 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:91: CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:80: CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:66: CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:91: CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:71: CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 5 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #0) > CryptoAlgorithm::generateKey() and its overrides always expect to receive a > non-null pointer to a CryptoAlgorithmParameter object and never take > ownership of it. Currently the CryptoAlgorithmParameter object is passed as > a lvalue reference to a std::unique_ptr<CryptoAlgorithmParameter> const. It > is sufficient to pass it as a lvalue reference to a CryptoAlgorithmParameter > const. Actually, it is a backward compatibility issue. Both SubtleCrypto::generateKey() and WebKitSubtleCrypto::generateKey() go through the same underlying CryptoKeyRSA::generatePair() to generate a RSA key pair. However, parameters used by those two are different. That's why I extract parameters outside of the CryptoAlgorithmParameter object. Ideally, we should pass the CryptoAlgorithmParameter object to CryptoKeyRSA::generatePair() once WebKitSubtleCrypto is deprecated.
(In reply to comment #3) > (In reply to comment #0) > > CryptoAlgorithm::generateKey() and its overrides always expect to receive a > > non-null pointer to a CryptoAlgorithmParameter object and never take > > ownership of it. Currently the CryptoAlgorithmParameter object is passed as > > a lvalue reference to a std::unique_ptr<CryptoAlgorithmParameter> const. It > > is sufficient to pass it as a lvalue reference to a CryptoAlgorithmParameter > > const. > > Actually, it is a backward compatibility issue. Both > SubtleCrypto::generateKey() and WebKitSubtleCrypto::generateKey() go through > the same underlying CryptoKeyRSA::generatePair() to generate a RSA key pair. > However, parameters used by those two are different. That's why I extract > parameters outside of the CryptoAlgorithmParameter object. Ideally, we > should pass the CryptoAlgorithmParameter object to > CryptoKeyRSA::generatePair() once WebKitSubtleCrypto is deprecated. And because CryptoKeyRSA::generatePair() will generate keys async. It will take over the ownership of the CryptoAlgorithmParameter object.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #0) > > > CryptoAlgorithm::generateKey() and its overrides always expect to receive a > > > non-null pointer to a CryptoAlgorithmParameter object and never take > > > ownership of it. Currently the CryptoAlgorithmParameter object is passed as > > > a lvalue reference to a std::unique_ptr<CryptoAlgorithmParameter> const. It > > > is sufficient to pass it as a lvalue reference to a CryptoAlgorithmParameter > > > const. > > > > Actually, it is a backward compatibility issue. Both > > SubtleCrypto::generateKey() and WebKitSubtleCrypto::generateKey() go through > > the same underlying CryptoKeyRSA::generatePair() to generate a RSA key pair. > > However, parameters used by those two are different. That's why I extract > > parameters outside of the CryptoAlgorithmParameter object. Ideally, we > > should pass the CryptoAlgorithmParameter object to > > CryptoKeyRSA::generatePair() once WebKitSubtleCrypto is deprecated. > > And because CryptoKeyRSA::generatePair() will generate keys async. It will > take over the ownership of the CryptoAlgorithmParameter object. Jiewen and I discussed this in person today (11/28). For now the proposed change is sufficient as key generation (CryptoAlgorithm::generateKey()) does not need to take ownership of the CryptoAlgorithmParameter object in both the old and new Web Crypto implementation. In the future we plan to change the new implementation such that key generation is asynchronous; => it will make sense for CryptoAlgorithm::generateKey() to take ownership of the CryptoAlgorithmParameter object. We will update the interface of CryptoAlgorithm::generateKey() as necessary to support such transfer of ownership.
Created attachment 295553 [details] Patch
Attachment 295553 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:91: CryptoAlgorithmRSA_OAEP::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:80: CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:66: CryptoAlgorithmAES_KW::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:91: CryptoAlgorithmAES_CBC::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:71: CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 5 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 295553 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295553&action=review > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:555 > + algorithm->generateKey(*params.get(), extractable, keyUsages, WTFMove(callback), WTFMove(exceptionCallback), *scriptExecutionContextFromExecState(&state)); Should not need the .get() here.
(In reply to comment #8) > > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:555 > > + algorithm->generateKey(*params.get(), extractable, keyUsages, WTFMove(callback), WTFMove(exceptionCallback), *scriptExecutionContextFromExecState(&state)); > > Should not need the .get() here. Will remove .get() before landing.
Committed r209077: <http://trac.webkit.org/changeset/209077>