...
Created attachment 373275 [details] Patch
Comment on attachment 373275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373275&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1660 > + session->credentialStorage().removeCredentialsWithOrigin(origin); My understanding is that using an HTTP Auth credential will establish both a WebCore::CredentialStorage entry and a non-persistent NSURLCredentialStorage entry. Is that right? If so, do we need to do additional work to remove the non-persistent NSURLCredentialStorage entry? You might need an HTTP test to test this, to see whether a credential is sent over the HTTP connection. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-536 > -#if PLATFORM(COCOA) > - if (dataTypes.contains(WebsiteDataType::Credentials) && isPersistent()) { > - for (auto& processPool : processPools()) { > - if (!processPool->networkProcess()) > - continue; > - > - callbackAggregator->addPendingCallback(); > - WTF::CompletionHandler<void(Vector<WebCore::SecurityOriginData>&&)> completionHandler = [callbackAggregator](Vector<WebCore::SecurityOriginData>&& origins) mutable { > - WebsiteData websiteData; > - for (auto& origin : origins) > - websiteData.entries.append(WebsiteData::Entry { origin, WebsiteDataType::Credentials, 0 }); > - callbackAggregator->removePendingCallback(WTFMove(websiteData)); > - }; > - processPool->networkProcess()->sendWithAsyncReply(Messages::NetworkProcess::OriginsWithPersistentCredentials(), WTFMove(completionHandler)); > - } > - } > -#endif > - Don't we still need to list non-persistent credentials in this function? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-1104 > - if (dataTypes.contains(WebsiteDataType::Credentials) && isPersistent()) { > - for (auto& processPool : processPools()) { > - if (!processPool->networkProcess()) > - continue; > - > - callbackAggregator->addPendingCallback(); > - WTF::CompletionHandler<void()> completionHandler = [callbackAggregator]() mutable { > - callbackAggregator->removePendingCallback(); > - }; > - processPool->networkProcess()->sendWithAsyncReply(Messages::NetworkProcess::RemoveCredentialsWithOrigins(origins), WTFMove(completionHandler)); > - } > - } > - Don't we still need to remove non-persistent credentials in this function? > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:-207 > - [websiteDataStore removeDataOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] forDataRecords:[NSArray arrayWithObject:expectedRecord.get()] completionHandler:^(void) { Can we include a test that adds a persistent credential to the shared credential store, and then verifies that removing website data does not remove that credential?
Comment on attachment 373275 [details] Patch Attachment 373275 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12632551 New failing tests: fast/block/float/float-with-anonymous-previous-sibling.html
Created attachment 373281 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
(In reply to Geoffrey Garen from comment #2) > Comment on attachment 373275 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=373275&action=review > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1660 > > + session->credentialStorage().removeCredentialsWithOrigin(origin); > > My understanding is that using an HTTP Auth credential will establish both a > WebCore::CredentialStorage entry and a non-persistent NSURLCredentialStorage > entry. Is that right? If so, do we need to do additional work to remove the > non-persistent NSURLCredentialStorage entry? You might need an HTTP test to > test this, to see whether a credential is sent over the HTTP connection. > I plan to fix the API first and treat this as a different bug. Our current problem is fetched credentials cannot be deleted. After this patch the API only has access to credentials stored in maps of CredentialStorage, so if client asks to delete credentials and then fetch, we will return empty set. Next step is to include session credentials in NSURLCredentialStorage in fetch and removal, which needs some testing to see networking behavior. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-536 > > -#if PLATFORM(COCOA) > > - if (dataTypes.contains(WebsiteDataType::Credentials) && isPersistent()) { > > - for (auto& processPool : processPools()) { > > - if (!processPool->networkProcess()) > > - continue; > > - > > - callbackAggregator->addPendingCallback(); > > - WTF::CompletionHandler<void(Vector<WebCore::SecurityOriginData>&&)> completionHandler = [callbackAggregator](Vector<WebCore::SecurityOriginData>&& origins) mutable { > > - WebsiteData websiteData; > > - for (auto& origin : origins) > > - websiteData.entries.append(WebsiteData::Entry { origin, WebsiteDataType::Credentials, 0 }); > > - callbackAggregator->removePendingCallback(WTFMove(websiteData)); > > - }; > > - processPool->networkProcess()->sendWithAsyncReply(Messages::NetworkProcess::OriginsWithPersistentCredentials(), WTFMove(completionHandler)); > > - } > > - } > > -#endif > > - > > Don't we still need to list non-persistent credentials in this function? > It's already handled by processPool->networkProcess()->fetchWebsiteData. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-1104 > > - if (dataTypes.contains(WebsiteDataType::Credentials) && isPersistent()) { > > - for (auto& processPool : processPools()) { > > - if (!processPool->networkProcess()) > > - continue; > > - > > - callbackAggregator->addPendingCallback(); > > - WTF::CompletionHandler<void()> completionHandler = [callbackAggregator]() mutable { > > - callbackAggregator->removePendingCallback(); > > - }; > > - processPool->networkProcess()->sendWithAsyncReply(Messages::NetworkProcess::RemoveCredentialsWithOrigins(origins), WTFMove(completionHandler)); > > - } > > - } > > - > > Don't we still need to remove non-persistent credentials in this function? > It's already handled by processPool->networkProcess()->deleteWebsiteDataForOrigins. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:-207 > > - [websiteDataStore removeDataOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] forDataRecords:[NSArray arrayWithObject:expectedRecord.get()] completionHandler:^(void) { > > Can we include a test that adds a persistent credential to the shared > credential store, and then verifies that removing website data does not > remove that credential? Yes, we should be able to do this with NSURLCredentialStorage API.
Created attachment 373307 [details] Patch
Comment on attachment 373307 [details] Patch Attachment 373307 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12637230 New failing tests: jquery/offset.html
Created attachment 373310 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 373328 [details] Patch
> > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:-207 > > > - [websiteDataStore removeDataOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] forDataRecords:[NSArray arrayWithObject:expectedRecord.get()] completionHandler:^(void) { > > > > Can we include a test that adds a persistent credential to the shared > > credential store, and then verifies that removing website data does not > > remove that credential? > > Yes, we should be able to do this with NSURLCredentialStorage API. Inspecting the API again, t's probably not easy to test this. For API clients, the normal flow is they fetch the records from the API first and then pass some records to API to delete. If they cannot fetch the persistent entries as WKWebsiteDataRecord, they cannot delete it.
Comment on attachment 373328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373328&action=review r=me > Source/WebCore/platform/network/CredentialStorage.cpp:198 > + return; unnecessary > Source/WebCore/platform/network/CredentialStorage.cpp:203 > + return; ditto
Created attachment 373436 [details] Patch for landing
Comment on attachment 373436 [details] Patch for landing Clearing flags on attachment: 373436 Committed r247123: <https://trac.webkit.org/changeset/247123>
All reviewed patches have been landed. Closing bug.
<rdar://problem/52622080>
Reverted r247123 for reason: Caused TestWebKitAPI.Challenge.BasicProposedCredential to fail. Committed r247162: <https://trac.webkit.org/changeset/247162>
(In reply to Ryan Haddad from comment #16) > Reverted r247123 for reason: > > Caused TestWebKitAPI.Challenge.BasicProposedCredential to fail. > > Committed r247162: <https://trac.webkit.org/changeset/247162> TestWebKitAPI.Challenge.BasicProposedCredential _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/Challenge.mm:200 Value of: receivedSecondChallenge Actual: false Expected: true https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20(Tests)/builds/5133/steps/run-api-tests/logs/stdio I was able to reproduce the failure locally with a build of r247123 with the following: run-api-tests Challenge --iterations 10 I could not reproduce the failure with r247121.
*** Bug 199501 has been marked as a duplicate of this bug. ***
(In reply to Ryan Haddad from comment #17) > (In reply to Ryan Haddad from comment #16) > > I was able to reproduce the failure locally with a build of r247123 with the > following: > run-api-tests Challenge --iterations 10 > > I could not reproduce the failure with r247121. It looks like the bots are seeing these tests time out, too: TestWebKitAPI.WKWebsiteDataStore.RemovePersistentCredentials TestWebKitAPI.Challenge.BasicProposedCredential TestWebKitAPI.WKWebsiteDataStore.FetchPersistentCredentials TestWebKitAPI.Challenge.SecIdentity There are keychain prompts on screen asking for access to '127.0.0.1 (testuser) and (username)'.
The patch introduces a behavior change that removeDataOfTypes would not remove persistent credentials. Therefore, the persistent credentials created in one test can be used by the later test, and didReceiveAuthenticationChallenge is not invoked (previous credentials are automatically applied). To solve this issue, we need to provide a method to clear the persistent credentials in the test. The original patch itself should be safe. We just need to make some changes on the tests.
Created attachment 373591 [details] Patch
> There are keychain prompts on screen asking for access to '127.0.0.1 (testuser) and (username)'. Still noticing this on Mac api-tests ews bots. See <rdar://problem/52736905>. iOS API tests bots are happy (https://ews-build.webkit.org/#/builders/9), but mac API tests bots (https://ews-build.webkit.org/#/builders/3) are still consistently failing 4 API tests which Ryan mentioned above (and getting stuck occasionally probably because of keychain prompts).
Comment on attachment 373591 [details] Patch Rejecting attachment 373591 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 373591, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/12698296
Created attachment 373722 [details] Patch for landing
Comment on attachment 373722 [details] Patch for landing Let's have EWS run through this patch before landing. I have pressed the 'Submit to EWS' buttons.
Comment on attachment 373722 [details] Patch for landing Clearing flags on attachment: 373722 Committed r247270: <https://trac.webkit.org/changeset/247270>
Same tests have started to fail again on EWS mac api tests queue. e.g.: https://ews-build.webkit.org/#/builders/3/builds/4439 TestWebKitAPI.Challenge.BasicProposedCredential, TestWebKitAPI.WKWebsiteDataStore.FetchPersistentCredentials, TestWebKitAPI.Challenge.SecIdentity