[webkit-reviews] review requested: [Bug 216509] Remove testRunner.setAllowsAnySSLCertificate : [Attachment 408769] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has asked  for review:
Bug 216509: Remove testRunner.setAllowsAnySSLCertificate
https://bugs.webkit.org/show_bug.cgi?id=216509

Attachment 408769: Patch

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




--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 408769
  --> https://bugs.webkit.org/attachment.cgi?id=408769
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.