Bug 144938

Created attachment 264838 [details] Patch

Attachment 264838 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:99: CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 264838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264838&action=review The approach looks good to me, however EWS detects crashes (assertion failures?) > Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:181 > + if (!getHashAlgorithm(jsDictionary, result->hash, true)) { We try very hard to not have mysterious true/false arguments in new code. Please add either an enum class for the value, or a separate function.

I see some failures running with or without this patch. Here is the output without the patch: ************************************************************************************ Expected to timeout, but passed: (3) fast/frames/lots-of-objects.html imported/w3c/web-platform-tests/html/semantics/document-metadata/the-base-element/base_multiple.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-object-element/object-handler.html Expected to fail, but passed: (4) fast/hidpi/filters-blur.html fast/hidpi/image-srcset-svg-canvas.html imported/w3c/web-platform-tests/html/semantics/text-level-semantics/the-wbr-element/wbr-element.html plugins/navigator-plugins.html Unexpected flakiness: timeouts (7) accessibility/accessibility-node-memory-management.html [ Timeout Pass ] accessibility/mac/abbr-acronym-tags.html [ Timeout Pass ] accessibility/mac/select-text/select-text-1.html [ Timeout Pass ] animations/3d/change-transform-in-end-event.html [ ImageOnlyFailure Timeout Pass ] animations/added-while-suspended.html [ Timeout Pass ] canvas/philip/tests/2d.canvas.readonly.html [ Timeout Pass ] imported/w3c/web-platform-tests/XMLHttpRequest/event-readystatechange-loaded.htm [ Timeout Pass ] Regressions: Unexpected text-only failures (1) http/tests/media/hls/video-controller-getStartDate.html [ Failure ] Regressions: Unexpected timeouts (1) crypto/subtle/rsa-export-generated-keys.html [ Timeout ] ************************************************************************************ And here is the output after the patch: ************************************************************************************ Expected to timeout, but passed: (3) fast/frames/lots-of-objects.html imported/w3c/web-platform-tests/html/semantics/document-metadata/the-base-element/base_multiple.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-object-element/object-handler.html Expected to fail, but passed: (4) fast/hidpi/filters-blur.html fast/hidpi/image-srcset-svg-canvas.html imported/w3c/web-platform-tests/html/semantics/text-level-semantics/the-wbr-element/wbr-element.html plugins/navigator-plugins.html Unexpected flakiness: timeouts (13) accessibility/accessibility-node-memory-management.html [ Timeout Pass ] accessibility/mac/abbr-acronym-tags.html [ Timeout Pass ] accessibility/mac/select-text/select-text-1.html [ Timeout Pass ] animations/3d/change-transform-in-end-event.html [ ImageOnlyFailure Timeout Pass ] animations/added-while-suspended.html [ Timeout Pass ] canvas/philip/tests/2d.canvas.readonly.html [ Timeout Pass ] http/tests/notifications/events.html [ Crash Timeout Pass ] imported/w3c/indexeddb/idbcursor-advance-invalid.htm [ Timeout Pass ] imported/w3c/indexeddb/idbdatabase_createObjectStore8-parameters.htm [ Timeout Pass ] storage/indexeddb/mozilla/cursor-update-updates-indexes.html [ Failure Timeout Pass ] storage/indexeddb/mozilla/cursors.html [ Timeout Pass ] webgl/1.0.2/conformance/context/context-lost-restored.html [ Timeout Pass ] webgl/1.0.2/conformance/rendering/point-size.html [ Timeout Pass ] Regressions: Unexpected text-only failures (1) http/tests/media/hls/video-controller-getStartDate.html [ Failure ] ************************************************************************************ So things are pretty much behaving as expected WRT the patch. The regression failure is due to a buggy test... here's the output diff: -EXPECTED (video.getStartDate() == 'Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)') OK +EXPECTED (video.getStartDate() == 'Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)'), OBSERVED 'Tue Nov 02 2010 22:00:00 GMT-1000 (HST)' FAIL So that is only failing because I'm in the wrong timezone. In any case, I did a git pull today and just re-ran these to verify, I promoted the boolean flag to an enum as requested, and will be submitting a new patch momentarily. Let me know what I can do if there are still failures on EWS.

Are you building debug or release? Assertions are disabled in release builds. It is unfortunate that you are getting multiple failures that bots don't have (these could be hardware dependent), but this is what the problem detected by the bot was: Regressions: Unexpected crashes (2) crypto/subtle/rsaes-pkcs1-v1_5-decrypt.html [ Crash ] crypto/subtle/rsaes-pkcs1-v1_5-wrap-unwrap-aes.html [ Crash ]

Created attachment 264902 [details] Patch

Attachment 264902 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:99: CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #5) > Are you building debug or release? Assertions are disabled in release builds. > > It is unfortunate that you are getting multiple failures that bots don't > have (these could be hardware dependent), but this is what the problem > detected by the bot was: > > Regressions: Unexpected crashes (2) > crypto/subtle/rsaes-pkcs1-v1_5-decrypt.html [ Crash ] > crypto/subtle/rsaes-pkcs1-v1_5-wrap-unwrap-aes.html [ Crash ] I'll look into those further. I was just doing a build-webkit without any options, I will do a --debug build. I'm running on an older macbook pro with El Capitan.

Comment on attachment 264902 [details] Patch Attachment 264902 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/388122 New failing tests: crypto/subtle/rsaes-pkcs1-v1_5-wrap-unwrap-aes.html crypto/subtle/rsaes-pkcs1-v1_5-decrypt.html

Created attachment 264910 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 264914 [details] Patch

Attachment 264914 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:99: CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #6) > Created attachment 264902 [details] > Patch I was able to reproduce the crashes in the debug build, and this version of the patch fixes the issue. I missed one case in JSCryptoKeySerializationJWK.cpp

Comment on attachment 264914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264914&action=review Looks good to me. Marking r- because there are minor nits that needs to be addressed. > Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:49 > +enum HashRequirement { Please use a strongly typed enum (enum class). > Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:50 > + HashIsOptional = 0, No need for a value here. > Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:121 > + if (HashIsRequired == isRequired) While it is sometimes recommended to put constants at the left to avoid accidental assignment, this is not a style we use in WebKit. We prefer readability over catching such mistakes, which are caught by compiler warnings and by tests anyway.

Created attachment 264956 [details] Patch

Cleaned up the remaining nits, and remembered to "git add" the unit test this time.

Attachment 264956 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:99: CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 264956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264956&action=review > Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:52 > +enum class HashRequirement { > + HashIsOptional, > + HashIsRequired, > +}; I'd use shorter values, given that the enum name is now required, e.g. HashRequirement::Optional.

Good point... sending it up now.

Created attachment 264958 [details] Patch

Attachment 264958 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:99: CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Please mark the patch as "commit-queue?" if you'd like it to be automatically landed. This is available through Details link to the right of the patch.

(In reply to comment #22) > Please mark the patch as "commit-queue?" if you'd like it to be > automatically landed. This is available through Details link to the right of > the patch. Is that going to work with the style-queue issue?

Comment on attachment 264958 [details] Patch Yes, it will work despite the style queue being unhappy.

Comment on attachment 264958 [details] Patch Clearing flags on attachment: 264958 Committed r192126: <http://trac.webkit.org/changeset/192126>

All reviewed patches have been landed. Closing bug.

(In reply to comment #25) > Comment on attachment 264958 [details] > Patch > > Clearing flags on attachment: 264958 > > Committed r192126: <http://trac.webkit.org/changeset/192126> The format of the ChangeLog entries in this commit are not consistent with the format of a ChangeLog entry used by the WebKit OpenSource Project. In particular, the ChangeLog entries did not mention the name of the person that reviewed the patch (Alexey Proskuryakov). You can read more about the format of a ChangeLog entry in section ChangeLog files on <http://www.webkit.org/coding/contributing.html>. Substituted "Unreviewed initial submission" for "Reviewed by Alexey Proskuryakov" in the ChangeLog entries associated with this patch and committed this change in <http://trac.webkit.org/changeset/192128>.

(In reply to comment #27) [...] > Substituted "Unreviewed initial submission" for "Reviewed by Alexey > Proskuryakov" in the ChangeLog entries associated with this patch and > committed this change in <http://trac.webkit.org/changeset/192128>. I meant to write: Substituted "Reviewed by Alexey Proskuryakov" for "Unreviewed initial submission" in the ChangeLog entries associated with this patch and committed this change in <http://trac.webkit.org/changeset/192128>.

(In reply to comment #28) > (In reply to comment #27) > [...] > > Substituted "Unreviewed initial submission" for "Reviewed by Alexey > > Proskuryakov" in the ChangeLog entries associated with this patch and > > committed this change in <http://trac.webkit.org/changeset/192128>. > > I meant to write: > > Substituted "Reviewed by Alexey Proskuryakov" for "Unreviewed initial > submission" in the ChangeLog entries associated with this patch and > committed this change in <http://trac.webkit.org/changeset/192128>. Thanks, I wasn't sure if that was automated or if I was supposed to do that.

The test added with this patch is frequently timing out on Mac Most recent timeout: <https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/builds/9786> Flakiness Dashboard: <http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=crypto%2Fsubtle%2Frsa-export-generated-keys.html>

Perhaps marking it a Slow would help?

Alright, marked as slow in <https://trac.webkit.org/r192182>

It might help to change the generated key size to 2048.

Changed it to 2048 in r192279, and removed the Slow expectation, as this should make it fast enough indeed.

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