Bug 188639 – Stop using canAuthenticateAgainstProtectionSpace in modern WebKit

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Stop using canAuthenticateAgainstProtectionSpace in modern WebKit

[Alex Christensen]

Created attachment 347248 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

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?

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 347271 [details]
Patch

[Alex Christensen]

(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.

[youenn fablet]

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.

[youenn fablet]

For extra safety, I would tend to verify we do not regress CORS preflight+ ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested challenges with this patch.

[youenn fablet]

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.

[Alex Christensen]

http://trac.webkit.org/r234941

[Radar WebKit Bug Importer]

<rdar://problem/43388361>