Bug 164740

Summary: Update SubtleCrypto::sign to match the latest spec
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, esprehn+autocc, jiewen_tan, kondapallykalyan, rob, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 160883    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
bfulgham: review+, buildbot: commit-queue-
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch for landing
none
Patch for landing none

Description Jiewen Tan 2016-11-14 15:52:16 PST 
Update SubtleCrypto::sign to match the latest spec.
Comment 1 Radar WebKit Bug Importer 2016-11-14 16:40:51 PST 
<rdar://problem/29257864>
Comment 2 Jiewen Tan 2016-11-22 15:36:51 PST 
Created attachment 295347 [details]
Patch
Comment 3 WebKit Commit Bot 2016-11-22 15:39:26 PST 
Attachment 295347 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:71:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:60:  signRSASSA_PKCS1_v1_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:85:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:105:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp:38:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 youenn fablet 2016-11-23 01:43:48 PST 
Comment on attachment 295347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295347&action=review

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:557
> +    auto exceptionCallback = [capturedPromise = promise.copyRef()](ExceptionCode ec) mutable {

You can probably move the promise here.

> Source/WebCore/crypto/SubtleCrypto.idl:36
> +    [Custom] Promise<any> sign(AlgorithmIdentifier algorithm, CryptoKey key, BufferSource data);

Can we try making it non custom?
This would probably require adding converters for AlgorithmIdentifier and CryptoKey but the code is probably already in JSSubtleCryptoCustom.
Comment 5 Brent Fulgham 2016-11-23 09:16:39 PST 
EFL/GTK seems to be failing due to "WebCore::CryptoAlgorithmHMAC::platformSign". Are they not getting built? Or do we need stubs for these platforms?
Comment 6 Jiewen Tan 2016-11-26 16:12:45 PST 
(In reply to comment #5)
> EFL/GTK seems to be failing due to
> "WebCore::CryptoAlgorithmHMAC::platformSign". Are they not getting built? Or
> do we need stubs for these platforms?

I do add WebCore::CryptoAlgorithmHMAC::platformSign into the gnutls implementations. Not sure why GTK/EFL fail to build.
Comment 7 Jiewen Tan 2016-11-26 16:23:39 PST 
Comment on attachment 295347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295347&action=review

Thanks, Youenn.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:557
>> +    auto exceptionCallback = [capturedPromise = promise.copyRef()](ExceptionCode ec) mutable {
> 
> You can probably move the promise here.

Hmmm, you are right.

>> Source/WebCore/crypto/SubtleCrypto.idl:36
>> +    [Custom] Promise<any> sign(AlgorithmIdentifier algorithm, CryptoKey key, BufferSource data);
> 
> Can we try making it non custom?
> This would probably require adding converters for AlgorithmIdentifier and CryptoKey but the code is probably already in JSSubtleCryptoCustom.

CryptoKey is an interface, it should be fine. However, AlgorithmIdentifier equals any here. I don't think it is good to make it an interface. Or if I miss some ways that we could add a customized type?
Comment 8 Jiewen Tan 2016-11-27 13:45:59 PST 
Created attachment 295462 [details]
Patch
Comment 9 Brent Fulgham 2016-11-28 10:00:33 PST 
Comment on attachment 295462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295462&action=review

This patch doesn't seem to apply on EWS. Can you upload a rebaselined version?

> LayoutTests/imported/w3c/WebCryptoAPI/idlharness-expected.txt:51
> +PASS SubtleCrypto interface: calling sign(AlgorithmIdentifier,CryptoKey,BufferSource) on crypto.subtle with too few arguments must throw TypeError 

Yay!
Comment 10 Jiewen Tan 2016-11-28 12:15:08 PST 
Created attachment 295511 [details]
Patch
Comment 11 WebKit Commit Bot 2016-11-28 12:17:58 PST 
Attachment 295511 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:71:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:60:  signRSASSA_PKCS1_v1_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:85:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:105:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp:38:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Jiewen Tan 2016-11-28 12:29:33 PST 
Created attachment 295515 [details]
Patch
Comment 13 WebKit Commit Bot 2016-11-28 12:32:28 PST 
Attachment 295515 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:71:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:60:  signRSASSA_PKCS1_v1_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:85:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:105:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp:38:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Jiewen Tan 2016-11-28 13:02:55 PST 
Created attachment 295519 [details]
Patch
Comment 15 WebKit Commit Bot 2016-11-28 13:06:48 PST 
Attachment 295519 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:71:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:60:  signRSASSA_PKCS1_v1_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:85:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:105:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp:38:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Jiewen Tan 2016-11-28 20:17:06 PST 
Created attachment 295575 [details]
Patch
Comment 17 WebKit Commit Bot 2016-11-28 20:19:08 PST 
Attachment 295575 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:71:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:60:  signRSASSA_PKCS1_v1_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:85:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:107:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp:38:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Build Bot 2016-11-28 22:07:53 PST 
Comment on attachment 295575 [details]
Patch

Attachment 295575 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2586814

New failing tests:
intersection-observer/intersection-observer-entry-interface.html
Comment 19 Build Bot 2016-11-28 22:07:57 PST 
Created attachment 295583 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 20 Brent Fulgham 2016-11-29 09:26:25 PST 
Comment on attachment 295575 [details]
Patch

I think this looks good. Can you investigate the ios-sim failure and see whether that's a real bug? Otherwise, r=me.
Comment 21 Jiewen Tan 2016-11-29 10:50:19 PST 
(In reply to comment #20)
> Comment on attachment 295575 [details]
> Patch
> 
> I think this looks good. Can you investigate the ios-sim failure and see
> whether that's a real bug? Otherwise, r=me.
According to the test history:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=intersection-observer%2Fintersection-observer-entry-interface.html

I think the test just somehow fails all the time regardless of my patch.
Comment 22 Jiewen Tan 2016-11-29 10:50:52 PST 
(In reply to comment #20)
> Comment on attachment 295575 [details]
> Patch
> 
> I think this looks good. Can you investigate the ios-sim failure and see
> whether that's a real bug? Otherwise, r=me.

Thanks Brent for r+ my patch.
Comment 23 Jiewen Tan 2016-11-29 13:21:17 PST 
Created attachment 295629 [details]
Patch for landing
Comment 24 Jiewen Tan 2016-11-29 14:47:57 PST 
Created attachment 295645 [details]
Patch for landing
Comment 25 WebKit Commit Bot 2016-11-29 14:50:48 PST 
Attachment 295645 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:71:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:60:  signRSASSA_PKCS1_v1_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:85:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:107:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp:38:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Jiewen Tan 2016-11-29 14:52:36 PST 
Committed r209092: <http://trac.webkit.org/changeset/209092>