Return value of SecItemAdd is often ignored. Even if it isn't, we don't have the ability to serialize SecKeychainItemRef.Otherwise, it would go through the weird route of serializing SecKeychainItemRef by asking Keychain for its persistent reference. This contradicts the purpose of SecItemShim, which is to proxy all Keychain operations to UIProcess.
<rdar://problem/40892596>
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>