Remove testRunner.setAllowsAnySSLCertificate

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Remove testRunner.setAllowsAnySSLCertificate

[Alex Christensen]

Created attachment 408769 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 408769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408769&action=review

> Source/WebKit/ChangeLog:8
> +        It used to be our only way to test TLS failures, but now that we have TCPServer and HTTPServer used in many tests with TLS, it is unnecessary.

Aren't TCPServer and HTTPServer only for API tests? Moving test from layout tests to API is super undesirable, because the latter are unscalable and difficult to work with.

Also, in this patch you are just invalidating a lot of tests without replacement.

[Alex Christensen]

Comment on attachment 408769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408769&action=review

>> Source/WebKit/ChangeLog:8
>> +        It used to be our only way to test TLS failures, but now that we have TCPServer and HTTPServer used in many tests with TLS, it is unnecessary.
> 
> Aren't TCPServer and HTTPServer only for API tests? Moving test from layout tests to API is super undesirable, because the latter are unscalable and difficult to work with.
> 
> Also, in this patch you are just invalidating a lot of tests without replacement.

I know you've never been a big fan of TCPServer and HTTPServer, but they do test exactly this with greater resolution than we can in layout tests.  API tests are indeed scalable and most of them are parallelizable.  We simply need to do the work to figure out which small set of tests are not parallelizable and make infrastructure that recognizes them.

testRunner.setAllowsAnySSLCertificate is used in the web socket tests because without it we would be unable to connect using the old CFStream WebSocket implementation, and we needed some tests there.  We still have all that test coverage, because the default in WebKitTestRunner and DumpRenderTree is now to allow all certificates, allowing our test apache certificates to be accepted.

This invalidates exactly two tests.  One was in crossorigin-arraybufferview-no-preflight.html which tests that beacons are not sent to servers to which we cannot establish a secure connection.  The other is in server-trust-evaluation.https.html which verifies that service workers also cannot connect to servers to which we cannot establish a secure connection.  I assert that we continue to have many tests that verify behavior when attempting such insecure connections, and I re-request review.  If it is deemed that these two specific cases are important enough to add tests specific to these two situations, I will add them, but I think they would be redundant.  In any case, WebProcessPool should not be the place where we make this decision, and I will keep pushing until we find an agreeable solution that does not have it there.

[Alex Christensen]

Created attachment 408796 [details]
Patch

[Alex Christensen]

I attached a second approach that would work for me for https://bugs.webkit.org/show_bug.cgi?id=216041
I find it less elegant, but it may be less controversial to reviewers.

[youenn fablet]

I am not a big fan of API tests either.

I agree with the idea to move to WebsiteDataStore. We partially did that work with WebsiteDataStore delegate didReceiveAuthenticationChallenge:.
Maybe we can use it for the service worker test as well and remove the use of the setAllowsAnySSLCertificate test runner API without modifying what the test is doing.

For WebSocket, we could also do the transitioning to a WebsiteDataStore dedicated boolean value and send it to NetworkProcess, to remove entirely DeprecatedGlobalSettings::allowsAnySSLCertificate().

[youenn fablet]

Comment on attachment 408796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408796&action=review

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2268
> +            networkProcess->sendSync(Messages::NetworkProcess::SetAllowsAnySSLCertificateForWebSocket(allows), Messages::NetworkProcess::SetAllowsAnySSLCertificateForWebSocket::Reply(), 0);

Do we really need to have the IPC synchronous?

[Alex Christensen]

Comment on attachment 408796 [details]
Patch

Unfortunately it does need to be synchronous right now or it will introduce flakiness because TestController::resetStateToConsistentValues does not have a CompletionHandler.

[EWS]

Committed r267088: <https://trac.webkit.org/changeset/267088>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408796 [details].

[Radar WebKit Bug Importer]

<rdar://problem/68926904>

[Alexey Proskuryakov]

Thank you for not reducing test coverage.

> API tests are indeed scalable and most of them are parallelizable.  We simply need
> to do the work to figure out which small set of tests are not parallelizable and
> make infrastructure that recognizes them.

This argument is not new, and API tests are just not getting better - there is still no parallelism, no test expectations short of ifdefs, they can't be run in production builds, there is not even a nice presentation of results like results.html.

[Alex Christensen]

(In reply to Alexey Proskuryakov from comment #11)
> This argument is not new, and API tests are just not getting better - there
> is still no parallelism, no test expectations short of ifdefs, they can't be
> run in production builds, there is not even a nice presentation of results
> like results.html.

This doesn't happen by itself.  It needs to have an engineer work on it.  It can be done, though.

[Alexey Proskuryakov]

I think that's exactly what I'm saying. Let's not rely on this argument until we have some hope of finding such an engineer.

[youenn fablet]

Also, API tests have a larger tendency to be platform-specific which is annoying when testing platform-agnostic features.

[youenn fablet]

And they cannot be shared with other browsers, so are in general not ideal for ensuring interoperability.