Bug 177673

Summary: http/tests/preconnect/link-rel-preconnect-https.html is flaky
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, commit-queue, dbates, japhet, jlewis3, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 177474    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Chris Dumez
Reported 2017-09-29 11:44:25 PDT
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".
Attachments
Patch (33.96 KB, patch)
2017-09-29 14:45 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (34.05 KB, patch)
2017-09-29 14:50 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (33.59 KB, patch)
2017-09-29 15:10 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (33.85 KB, patch)
2017-09-29 16:55 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2017-09-29 11:48:52 PDT
Temporarily skipped in <http://trac.webkit.org/changeset/222657>. Working on a fix right now.
Chris Dumez
Comment 2 2017-09-29 13:06:44 PDT
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
Comment 3 2017-09-29 13:13:32 PDT
(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
Comment 4 2017-09-29 14:45:39 PDT
Created attachment 322235 [details] Patch
Chris Dumez
Comment 5 2017-09-29 14:50:45 PDT
Created attachment 322238 [details] Patch
Chris Dumez
Comment 6 2017-09-29 15:10:19 PDT
Created attachment 322241 [details] Patch
Alex Christensen
Comment 7 2017-09-29 16:45:04 PDT
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 8 2017-09-29 16:51:37 PDT
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
Comment 9 2017-09-29 16:55:00 PDT
Created attachment 322257 [details] Patch
WebKit Commit Bot
Comment 10 2017-09-29 17:35:27 PDT
Comment on attachment 322257 [details] Patch Clearing flags on attachment: 322257 Committed r222673: <http://trac.webkit.org/changeset/222673>
WebKit Commit Bot
Comment 11 2017-09-29 17:35:29 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2017-09-29 17:36:12 PDT
<rdar://problem/34751394>
Note You need to log in before you can comment on or make changes to this bug.