Bug 196870

Created attachment 367337 [details] Patch

@wilander, this should take care of your issue with HTTPS ping loads.

Comment on attachment 367337 [details] Patch Nice. Could you add a Ping layout test with testRunner.setAllowsAnySSLCertificate(true) and the ping endpoint as https? Should be easy.

Created attachment 367347 [details] Patch

(In reply to John Wilander from comment #3) > Comment on attachment 367337 [details] > Patch > > Nice. Could you add a Ping layout test with > testRunner.setAllowsAnySSLCertificate(true) and the ping endpoint as https? > Should be easy. Yep, probably need to terminate network process and then do a ping.

Created attachment 367351 [details] Patch

Comment on attachment 367351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367351&action=review I think this needs an API test that verifies this delegate is being called under the correct circumstances and the credentials are used as expected. > Source/WebKit/NetworkProcess/PingLoad.cpp:135 > - completionHandler(AuthenticationChallengeDisposition::PerformDefaultHandling, { }); > + m_networkLoadChecker->networkProcess().authenticationManager().didReceiveAuthenticationChallenge(m_parameters.sessionID, 0, 0, challenge, WTFMove(completionHandler)); We almost certainly don't want ping loads to handle authentication challenges. Then there would be random popups asking for credentials. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:82 > + case NSURLSessionAuthChallengeUseCredential: This is duplicate code with NavigationState.mm > LayoutTests/ChangeLog:8 > + * http/wpt/beacon/cors/crossorigin-arraybufferview-no-preflight.html: Wouldn't this changed test have passed before because we performed default behavior? Now we're just performing default behavior in a different place, so this test does not verify the change works.

Challenge.mm has a growing number of similar authentication unit tests.

(In reply to Alex Christensen from comment #7) > Comment on attachment 367351 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367351&action=review > > I think this needs an API test that verifies this delegate is being called > under the correct circumstances and the credentials are used as expected. I'll add one. > > Source/WebKit/NetworkProcess/PingLoad.cpp:135 > > - completionHandler(AuthenticationChallengeDisposition::PerformDefaultHandling, { }); > > + m_networkLoadChecker->networkProcess().authenticationManager().didReceiveAuthenticationChallenge(m_parameters.sessionID, 0, 0, challenge, WTFMove(completionHandler)); > > We almost certainly don't want ping loads to handle authentication > challenges. Then there would be random popups asking for credentials. This is limited to server trust evaluation, there is no need to ask user for credentials. There might be a need for prompt to ask whether user trusts the site or not, but this is really an application decision. Overall, the design I see is that this additional delegate would only be about asking whether the given certificate is already granted, not whether user is granting it. Also, ping loads are no different from service worker fetches in that respect. In the future, ping loads might just be regular fetch until their context is destroyed. See https://bugs.webkit.org/show_bug.cgi?id=196807 > > LayoutTests/ChangeLog:8 > > + * http/wpt/beacon/cors/crossorigin-arraybufferview-no-preflight.html: > > Wouldn't this changed test have passed before because we performed default > behavior? Now we're just performing default behavior in a different place, > so this test does not verify the change works. Before the patch, the test would be broken. By killing the network process, we are sure there is no connection. The ping triggers a new connection, thus a server trust evaluation. The default behavior will be done in network process and will cancel the creation of the connection. After the patch, we go to UIProcess and ask WTR whether to proceed with the load.

Created attachment 368012 [details] Patch

Created attachment 368033 [details] Patch

Comment on attachment 368033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368033&action=review > Source/WebKit/NetworkProcess/PingLoad.cpp:151 > + m_networkLoadChecker->networkProcess().authenticationManager().didReceiveAuthenticationChallenge(m_parameters.sessionID, 0, 0, challenge, WTFMove(completionHandler)); This needs rebasing. The 0's are now empty identifiers. We might need to make them nullopt's. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:54 > + , m_hasAuthenticationChallengeSelector([m_delegate.get() respondsToSelector:@selector(didReceiveAuthenticationChallenge: completionHandler:)]) extra space

Created attachment 376999 [details] Patch for landing

Comment on attachment 376999 [details] Patch for landing Clearing flags on attachment: 376999 Committed r249001: <https://trac.webkit.org/changeset/249001>

All reviewed patches have been landed. Closing bug.

<rdar://problem/54593556>

It looks like the changes in https://trac.webkit.org/changeset/249001/webkit caused this test to fail constantly: imported/w3c/web-platform-tests/service-workers/service-worker/websocket-in-service-worker.https.html it was a flakey failure before. The diff makes it look like this is just a rebasline, can you confirm? History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fwebsocket-in-service-worker.https.html Diff: --- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/websocket-in-service-worker.https-expected.txt +++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/websocket-in-service-worker.https-actual.txt @@ -1,3 +1,3 @@ -FAIL Verify WebSockets can be created in a Service Worker assert_equals: expected "PASS" but got "FAIL: Got an error event" +PASS Verify WebSockets can be created in a Service Worker

Reverted r249001 for reason: Caused one layout test to fail on all configurations and another to time out on Catalina / iOS 13. Committed r249063: <https://trac.webkit.org/changeset/249063>

Created attachment 377238 [details] Relanding

(In reply to Ryan Haddad from comment #18) > Reverted r249001 for reason: > > Caused one layout test to fail on all configurations and another to time out > on Catalina / iOS 13. Layout test was going from FAIL to PASS. The other test was updated to cope with the more accurate handling of window.testRunner.setAllowsAnySSLCertificate

Comment on attachment 377238 [details] Relanding Rejecting attachment 377238 [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-03', 'validate-changelog', '--check-oops', '--non-interactive', 377238, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/12968665

Created attachment 377241 [details] Patch for landing

Comment on attachment 377241 [details] Patch for landing Clearing flags on attachment: 377241 Committed r249096: <https://trac.webkit.org/changeset/249096>

All reviewed patches have been landed. Closing bug.