Bug 171059 - LayoutTests crypto/subtle/ecdsa-generate-key-sign-verify-p384.html and crypto/subtle/ecdsa-generate-key-sign-verify-p256.html are flaky failures
Summary: LayoutTests crypto/subtle/ecdsa-generate-key-sign-verify-p384.html and crypto...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
: 170921 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-04-20 09:34 PDT by Ryan Haddad
Modified: 2017-04-24 22:10 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.53 KB, patch)
2017-04-24 14:11 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (5.10 KB, patch)
2017-04-24 14:41 PDT, Jiewen Tan
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (5.11 KB, patch)
2017-04-24 18:58 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2017-04-20 09:34:09 PDT
LayoutTests crypto/subtle/ecdsa-generate-key-sign-verify-p384.html and crypto/subtle/ecdsa-generate-key-sign-verify-p256.html are flaky failures

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=crypto%2Fsubtle%2Fecdsa-

--- /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/crypto/subtle/ecdsa-generate-key-sign-verify-p384-expected.txt
+++ /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/crypto/subtle/ecdsa-generate-key-sign-verify-p384-actual.txt
@@ -7,7 +7,7 @@
 Signing
 PASS signature.byteLength is 96
 Verifying
-PASS verified is true
+FAIL verified should be true. Was false.
 PASS successfullyParsed is true
 
 TEST COMPLETE

--- /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/crypto/subtle/ecdsa-generate-key-sign-verify-p256-expected.txt
+++ /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/crypto/subtle/ecdsa-generate-key-sign-verify-p256-actual.txt
@@ -7,7 +7,7 @@
 Signing
 PASS signature.byteLength is 64
 Verifying
-PASS verified is true
+FAIL verified should be true. Was false.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 1 Radar WebKit Bug Importer 2017-04-20 09:34:58 PDT
<rdar://problem/31734958>
Comment 2 Ryan Haddad 2017-04-20 10:50:28 PDT
Marked tests as flaky in https://trac.webkit.org/changeset/215563/webkit
Comment 3 Jiewen Tan 2017-04-24 12:51:51 PDT
*** Bug 170921 has been marked as a duplicate of this bug. ***
Comment 4 Jiewen Tan 2017-04-24 14:11:49 PDT
Created attachment 308009 [details]
Patch
Comment 5 Jiewen Tan 2017-04-24 14:41:14 PDT
Created attachment 308011 [details]
Patch
Comment 6 Brent Fulgham 2017-04-24 17:19:17 PDT
Comment on attachment 308011 [details]
Patch

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

Looks fine, but I think you might want to clean up the code a little. r=me with cleanups tp signECDSA.

> Source/WebCore/crypto/mac/CryptoAlgorithmECDSAMac.cpp:92
> +        size_t pos = newSignature.size();

Isn't pos always zero here, since you only reserved capacity above, and did not actually fill the vector with anything?

> Source/WebCore/crypto/mac/CryptoAlgorithmECDSAMac.cpp:97
> +    // Otherwise skipping the leading 0s of r.

// Otherwise skip the leading 0s of r.

> Source/WebCore/crypto/mac/CryptoAlgorithmECDSAMac.cpp:98
> +    if (signature[offset] > keyLengthInBytes)

Should this be "else if"?

> Source/WebCore/crypto/mac/CryptoAlgorithmECDSAMac.cpp:113
> +    // Otherwise skipping the leading 0s of s.

"// Otherwise skip the leading 0s of s."
Comment 7 Jiewen Tan 2017-04-24 17:41:55 PDT
Comment on attachment 308011 [details]
Patch

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

Thanks Brent for r+ my patch.

>> Source/WebCore/crypto/mac/CryptoAlgorithmECDSAMac.cpp:92
>> +        size_t pos = newSignature.size();
> 
> Isn't pos always zero here, since you only reserved capacity above, and did not actually fill the vector with anything?

True. I just want to make it more general. I can remove pos under this check completely.

>> Source/WebCore/crypto/mac/CryptoAlgorithmECDSAMac.cpp:97
>> +    // Otherwise skipping the leading 0s of r.
> 
> // Otherwise skip the leading 0s of r.

Fixed.

>> Source/WebCore/crypto/mac/CryptoAlgorithmECDSAMac.cpp:98
>> +    if (signature[offset] > keyLengthInBytes)
> 
> Should this be "else if"?

I remember there is style check on if ... else ...
We can only do 'if ... else if ... else ...' not 'if ... else if ...'. Maybe I am wrong. Let me double check.

>> Source/WebCore/crypto/mac/CryptoAlgorithmECDSAMac.cpp:113
>> +    // Otherwise skipping the leading 0s of s.
> 
> "// Otherwise skip the leading 0s of s."

Fixed.
Comment 8 Jiewen Tan 2017-04-24 18:58:24 PDT
Created attachment 308043 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2017-04-24 22:10:03 PDT
Comment on attachment 308043 [details]
Patch for landing

Clearing flags on attachment: 308043

Committed r215721: <http://trac.webkit.org/changeset/215721>