Stop using canAuthenticateAgainstProtectionSpace in modern WebKit
Created attachment 347248 [details] Patch
Comment on attachment 347248 [details] Patch Attachment 347248 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8877013 New failing tests: accessibility/smart-invert-reference.html
Created attachment 347251 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 347248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347248&action=review > Source/WebKit/ChangeLog:12 > + with the 1 client that still uses it (not for long, see rdar://problem/43358403) and simplify and optimize No public API is impacted? > Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:-106 > - NetworkProcess::singleton().canAuthenticateAgainstProtectionSpace(challenge.protectionSpace(), m_parameters.pageID, m_parameters.frameID, [this, weakThis = makeWeakPtr(this), completionHandler = WTFMove(completionHandler), challenge = WTFMove(challenge)] (bool canAuthenticate) mutable { NetworkCORSPreflightChecker no longer needs to be weakptr. > Source/WebKit/NetworkProcess/NetworkLoad.cpp:254 > void NetworkLoad::didReceiveChallenge(AuthenticationChallenge&& challenge, ChallengeCompletionHandler&& completionHandler) We probably no longer need to pass an AuthenticationChallenge&&. > Source/WebKit/NetworkProcess/NetworkLoad.cpp:269 > +#endif We do not have this handling in NetworkCORSPreflightChecker, is that fine? Should we move that code in NetworkDataTaskCocoa::didReceiveChallenge?
Comment on attachment 347248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347248&action=review >> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:-106 >> - NetworkProcess::singleton().canAuthenticateAgainstProtectionSpace(challenge.protectionSpace(), m_parameters.pageID, m_parameters.frameID, [this, weakThis = makeWeakPtr(this), completionHandler = WTFMove(completionHandler), challenge = WTFMove(challenge)] (bool canAuthenticate) mutable { > > NetworkCORSPreflightChecker no longer needs to be weakptr. Right! >> Source/WebKit/NetworkProcess/NetworkLoad.cpp:254 >> void NetworkLoad::didReceiveChallenge(AuthenticationChallenge&& challenge, ChallengeCompletionHandler&& completionHandler) > > We probably no longer need to pass an AuthenticationChallenge&&. I think we still should for conceptual correctness. >> Source/WebKit/NetworkProcess/NetworkLoad.cpp:269 >> +#endif > > We do not have this handling in NetworkCORSPreflightChecker, is that fine? > Should we move that code in NetworkDataTaskCocoa::didReceiveChallenge? That's fine. This is actually from SPI just for testing that I plan to remove soon.
Created attachment 347271 [details] Patch
(In reply to youenn fablet from comment #4) > Comment on attachment 347248 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=347248&action=review > > > Source/WebKit/ChangeLog:12 > > + with the 1 client that still uses it (not for long, see rdar://problem/43358403) and simplify and optimize > > No public API is impacted? No public API is impacted. The only public API we have is WKNavigationDelegate's didReceiveAuthenticationChallenge which works "the right way". This is removing old stuff.
Comment on attachment 347271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347271&action=review > Source/WebKit/NetworkProcess/PreconnectTask.h:-53 > - void canAuthenticateAgainstProtectionSpaceAsync(const WebCore::ProtectionSpace&) final; Maybe PreconnectTask no longer needs either to be CanMakeWeakPtr either.
For extra safety, I would tend to verify we do not regress CORS preflight+ ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested challenges with this patch.
Given how much we regress this, we should write a test for such case. Do we receive a ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested challenge when loading https://localhost:8443? If so, we could crash the network process and then do a CORS preflight.
http://trac.webkit.org/r234941
<rdar://problem/43388361>