Bug 216509 - Remove testRunner.setAllowsAnySSLCertificate
Summary: Remove testRunner.setAllowsAnySSLCertificate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-14 17:13 PDT by Alex Christensen
Modified: 2020-09-16 00:45 PDT (History)
3 users (show)

See Also:


Attachments
Patch (32.68 KB, patch)
2020-09-14 17:19 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.65 KB, patch)
2020-09-14 23:26 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2020-09-14 17:13:33 PDT
Remove testRunner.setAllowsAnySSLCertificate
Comment 1 Alex Christensen 2020-09-14 17:19:26 PDT
Created attachment 408769 [details]
Patch
Comment 2 Alexey Proskuryakov 2020-09-14 18:36:19 PDT
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.
Comment 3 Alex Christensen 2020-09-14 22:04:32 PDT
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.
Comment 4 Alex Christensen 2020-09-14 23:26:07 PDT
Created attachment 408796 [details]
Patch
Comment 5 Alex Christensen 2020-09-14 23:27:37 PDT
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.
Comment 6 youenn fablet 2020-09-14 23:48:10 PDT
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().
Comment 7 youenn fablet 2020-09-14 23:50:38 PDT
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?
Comment 8 Alex Christensen 2020-09-15 09:23:47 PDT
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.
Comment 9 EWS 2020-09-15 09:28:38 PDT
Committed r267088: <https://trac.webkit.org/changeset/267088>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408796 [details].
Comment 10 Radar WebKit Bug Importer 2020-09-15 09:29:17 PDT
<rdar://problem/68926904>
Comment 11 Alexey Proskuryakov 2020-09-15 11:32:03 PDT
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.
Comment 12 Alex Christensen 2020-09-15 11:33:43 PDT
(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.
Comment 13 Alexey Proskuryakov 2020-09-15 16:10:26 PDT
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.
Comment 14 youenn fablet 2020-09-16 00:45:02 PDT
Also, API tests have a larger tendency to be platform-specific which is annoying when testing platform-agnostic features.
Comment 15 youenn fablet 2020-09-16 00:45:43 PDT
And they cannot be shared with other browsers, so are in general not ideal for ensuring interoperability.