http/tests/preconnect/link-rel-preconnect-https.html is flaky

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

http/tests/preconnect/link-rel-preconnect-https.html is flaky:

diff:
--- /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-results/http/tests/preconnect/link-rel-preconnect-https-expected.txt
+++ /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-results/http/tests/preconnect/link-rel-preconnect-https-actual.txt
@@ -1,4 +1,4 @@
-CONSOLE MESSAGE: Failed to preconnect to https://localhost:8443/. Error: The certificate for this server is invalid. You might be connecting to a server that is pretending to be “localhost” which could put your confidential information at risk.
+CONSOLE MESSAGE: Successfuly preconnected to https://localhost:8443/
 Tests that Link's rel=preconnect works as expected over HTTPS.

 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

[Chris Dumez]

Temporarily skipped in <http://trac.webkit.org/changeset/222657>. Working on a fix right now.

[Chris Dumez]

Alex, the test is currently inherently flaky because it relies on the state of the SSL certificate on our bots.

I see 2 ways of addressing this:
1. Drop the HTTPS test.
2. Ask the client if the certificate is valid, similarly to resource loads. When the client is WKTR, it will accept invalid certificates and the test will always pass. This would require a bit more code to hand-shake with the WebProcess but it is doable. However, I do not know if it is suitable to ask the client to validate the certificate of the preconnect.

Which one do you prefer? Do you have another proposal?

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> Alex, the test is currently inherently flaky because it relies on the state
> of the SSL certificate on our bots.
> 
> I see 2 ways of addressing this:
> 1. Drop the HTTPS test.
> 2. Ask the client if the certificate is valid, similarly to resource loads.
> When the client is WKTR, it will accept invalid certificates and the test
> will always pass. This would require a bit more code to hand-shake with the
> WebProcess but it is doable. However, I do not know if it is suitable to ask
> the client to validate the certificate of the preconnect.
> 
> Which one do you prefer? Do you have another proposal?

Third proposal:
1. When a console message listener is set via internals, report the console message to the listener but do not log it. We leave it up to the test to log the console message or not since we pass the string to the listener.

[Chris Dumez]

Created attachment 322235 [details]
Patch

[Chris Dumez]

Created attachment 322238 [details]
Patch

[Chris Dumez]

Created attachment 322241 [details]
Patch

[Alex Christensen]

Comment on attachment 322241 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:466
> +void WebLoaderStrategy::preconnectTo(NetworkingContext* context, const WebCore::URL& url, StoredCredentialsPolicy storedCredentialsPolicy, PreconnectCompletionHandler&& completionHandler)

Could we make this take a reference?  If there's no NetworkingContext, we probably just shouldn't try to preconnect.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:475
> +    WebFrameNetworkingContext* webContext = static_cast<WebFrameNetworkingContext*>(context);
> +    WebFrameLoaderClient* webFrameLoaderClient = webContext->webFrameLoaderClient();
> +    WebFrame* webFrame = webFrameLoaderClient ? webFrameLoaderClient->webFrame() : nullptr;
> +    WebPage* webPage = webFrame ? webFrame->page() : nullptr;

I think we should early return if any of these are null instead of trying to connect without a page.  The caller has already verified that a frame exists.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:477
> +    NetworkResourceLoadParameters parameters;

Could we use an initializer list to make sure we haven't forgotten any parameters?

[Chris Dumez]

Comment on attachment 322241 [details]
Patch

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

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:477
>> +    NetworkResourceLoadParameters parameters;
> 
> Could we use an initializer list to make sure we haven't forgotten any parameters?

NetworkResourceLoadParameters has a lot of members and a parent class. I am definitely not initializing all the members.

[Chris Dumez]

Created attachment 322257 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 322257 [details]
Patch

Clearing flags on attachment: 322257

Committed r222673: <http://trac.webkit.org/changeset/222673>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34751394>