Bug 171059

Summary:

LayoutTests crypto/subtle/ecdsa-generate-key-sign-verify-p384.html and crypto/subtle/ecdsa-generate-key-sign-verify-p256.html are flaky failures

Product:

WebKit

Reporter:

Ryan Haddad <ryanhaddad>

Component:

New Bugs

Assignee:

Jiewen Tan <jiewen_tan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, commit-queue, jiewen_tan, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	bfulgham: review+, bfulgham: commit-queue-
Patch for landing	none

Ryan Haddad

Reported 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

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2017-04-20 09:34:58 PDT

<rdar://problem/31734958>

Ryan Haddad

Comment 2 2017-04-20 10:50:28 PDT

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

Jiewen Tan

Comment 3 2017-04-24 12:51:51 PDT

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

Jiewen Tan

Comment 4 2017-04-24 14:11:49 PDT

Created attachment 308009 [details] Patch

Jiewen Tan

Comment 5 2017-04-24 14:41:14 PDT

Created attachment 308011 [details] Patch

Brent Fulgham

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

Jiewen Tan

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

Jiewen Tan

Comment 8 2017-04-24 18:58:24 PDT

Created attachment 308043 [details] Patch for landing

WebKit Commit Bot

Comment 9 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>

Note You need to log in before you can comment on or make changes to this bug.