Summary: | Make SecItemShim to not send return value for SecItemAdd | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||||||
Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, ews-watchlist, jiewen_tan, rniwa, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Jiewen Tan
2018-06-18 14:42:53 PDT
Created attachment 342980 [details]
Patch
Comment on attachment 342980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342980&action=review r=me with these changes, and with clean EWS. > Source/WebKit/Shared/mac/SecItemShim.cpp:102 > static OSStatus webSecItemAdd(CFDictionaryRef query, CFTypeRef* result) Suggest renaming to "unusedResult" > Source/WebKit/Shared/mac/SecItemShim.cpp:104 > + // Return value of SecItemAdd is often ignored. Even if it isn't, we don't have the ability to // Return value of SecItemAdd should be ignored for WebKit use cases. WebKit can't serialize SecKeychainItemRef, so we do not use it. // If someone passes a result value to be populated, the API contract is being violated so we should assert. > Source/WebKit/Shared/mac/SecItemShim.cpp:106 > + if (result != nullptr) { if (unusedResult) { ... > Source/WebKit/Shared/mac/SecItemShim.cpp:113 > return errSecInteractionNotAllowed; I also suggest you remove lines 115 and 116, since we don't ever expect to have something here to fill in. Comment on attachment 342980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342980&action=review Thanks Brent for r+ this patch. >> Source/WebKit/Shared/mac/SecItemShim.cpp:102 >> static OSStatus webSecItemAdd(CFDictionaryRef query, CFTypeRef* result) > > Suggest renaming to "unusedResult" Fixed. >> Source/WebKit/Shared/mac/SecItemShim.cpp:104 >> + // Return value of SecItemAdd is often ignored. Even if it isn't, we don't have the ability to > > // Return value of SecItemAdd should be ignored for WebKit use cases. WebKit can't serialize SecKeychainItemRef, so we do not use it. > // If someone passes a result value to be populated, the API contract is being violated so we should assert. Fixed. >> Source/WebKit/Shared/mac/SecItemShim.cpp:106 >> + if (result != nullptr) { > > if (unusedResult) { ... Fixed. >> Source/WebKit/Shared/mac/SecItemShim.cpp:113 >> return errSecInteractionNotAllowed; > > I also suggest you remove lines 115 and 116, since we don't ever expect to have something here to fill in. Fixed. Comment on attachment 342980 [details] Patch Attachment 342980 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/8244046 New failing tests: accessibility/mac/selection-notification-focus-change.html Created attachment 343027 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
The tree of mac-wk2-ews is red. Disregard that result, I am landing the patch. Created attachment 343067 [details]
Patch for landing
Comment on attachment 343067 [details] Patch for landing Rejecting attachment 343067 [details] from commit-queue. Number of test failures exceeded the failure limit. Full output: http://webkit-queues.webkit.org/results/8252701 Created attachment 343097 [details]
Archive of layout-test-results from webkit-cq-02 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02 Port: mac-sierra Platform: Mac OS X 10.12.6
Committed r232990: <https://trac.webkit.org/changeset/232990> |