Bug 188639

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>