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

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryan Haddad]

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

[Radar WebKit Bug Importer]

<rdar://problem/31734958>

[Ryan Haddad]

Marked tests as flaky in https://trac.webkit.org/changeset/215563/webkit

[Jiewen Tan]

*** Bug 170921 has been marked as a duplicate of this bug. ***

[Jiewen Tan]

Created attachment 308009 [details]
Patch

[Jiewen Tan]

Created attachment 308011 [details]
Patch

[Brent Fulgham]

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."

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 308043 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 308043 [details]
Patch for landing

Clearing flags on attachment: 308043

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