Bug 177474

Summary: Add support for <link rel=preconnect>
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, cdumez, commit-queue, dbates, esprehn+autocc, ggaren, gyuyoung.kim, japhet, jlewis3, kangil.han, kondapallykalyan, lforschler, mkwst, orzage, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://w3c.github.io/resource-hints/#preconnect
See Also: https://bugs.webkit.org/show_bug.cgi?id=177626
Bug Depends on: 177455, 177673    
Bug Blocks:    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-09-25 16:45:00 PDT
Add support for <link rel=preconnect>:
- https://w3c.github.io/resource-hints/#preconnect
Comment 1 Chris Dumez 2017-09-25 16:50:17 PDT
Created attachment 321766 [details]
WIP Patch
Comment 2 Build Bot 2017-09-25 16:55:17 PDT
Attachment 321766 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/LinkLoader.cpp:261:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/NetworkProcess/PreconnectTask.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Chris Dumez 2017-09-25 16:57:10 PDT
Created attachment 321768 [details]
WIP Patch
Comment 4 Build Bot 2017-09-25 16:58:34 PDT
Attachment 321768 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/LinkLoader.cpp:262:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Chris Dumez 2017-09-25 17:02:44 PDT
Created attachment 321770 [details]
WIP Patch
Comment 6 Build Bot 2017-09-25 17:15:01 PDT
Attachment 321770 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/LinkLoader.cpp:262:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Chris Dumez 2017-09-26 10:14:30 PDT
Created attachment 321833 [details]
WIP Patch
Comment 8 Chris Dumez 2017-09-26 10:20:31 PDT
Created attachment 321835 [details]
WIP Patch
Comment 9 Chris Dumez 2017-09-26 10:54:43 PDT
<rdar://problem/33141380>
Comment 10 Chris Dumez 2017-09-26 12:02:24 PDT
Created attachment 321853 [details]
WIP Patch
Comment 11 Chris Dumez 2017-09-26 13:10:59 PDT
Created attachment 321859 [details]
WIP Patch
Comment 12 Chris Dumez 2017-09-26 13:39:48 PDT
Created attachment 321861 [details]
Patch
Comment 13 Chris Dumez 2017-09-26 14:14:07 PDT
Created attachment 321867 [details]
Patch
Comment 14 Alex Christensen 2017-09-26 19:11:13 PDT
Comment on attachment 321867 [details]
Patch

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

> Source/WebKit/NetworkProcess/PreconnectTask.cpp:64
> +    ASSERT(!m_completionHandler);

Isn't this built into the definition of a completion handler?

> Source/WebKit/NetworkProcess/PreconnectTask.cpp:101
> +    m_completionHandler(error);

Will this call didFinish and delete this?

> Source/WebKit/NetworkProcess/PreconnectTask.cpp:118
> +    didFinish(cannotShowURLError(ResourceRequest { }));

Can't we make an error that knows the URL we were given?

> Source/WebKit/NetworkProcess/PreconnectTask.cpp:125
> +    delete this;

PingLoad has a timeout to prevent memory leaks if the server never responds.  Do we want to do something similar here?  Do we want to just expand the PingLoad to handle this responsibility, too?  It seems like we could do without this class by just adding a flag to PingLoad.

> LayoutTests/http/tests/preconnect/link-rel-preconnect-http.html:17
> +setTimeout(finishJSTest, 1000);

Since we're waiting for a completion handler and writing to the console and checking that, couldn't we make an internals function that checks to see if the message has been written to the console?  I don't particularly like the tests that have to be slow enough to not time out on the bots
Comment 15 Chris Dumez 2017-09-27 09:23:30 PDT
Comment on attachment 321867 [details]
Patch

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

>> Source/WebKit/NetworkProcess/PreconnectTask.cpp:64
>> +    ASSERT(!m_completionHandler);
> 
> Isn't this built into the definition of a completion handler?

You're right, I forgot. I'll drop this assertion.

>> Source/WebKit/NetworkProcess/PreconnectTask.cpp:101
>> +    m_completionHandler(error);
> 
> Will this call didFinish and delete this?

Oh, it should indeed call didFinish.

>> Source/WebKit/NetworkProcess/PreconnectTask.cpp:118
>> +    didFinish(cannotShowURLError(ResourceRequest { }));
> 
> Can't we make an error that knows the URL we were given?

We can but it means we'll need an extra member. I did not think it was worth it (WebCore already knows the URL since there is no redirect involved) but OK.

>> Source/WebKit/NetworkProcess/PreconnectTask.cpp:125
>> +    delete this;
> 
> PingLoad has a timeout to prevent memory leaks if the server never responds.  Do we want to do something similar here?  Do we want to just expand the PingLoad to handle this responsibility, too?  It seems like we could do without this class by just adding a flag to PingLoad.

I'll look into this.

>> LayoutTests/http/tests/preconnect/link-rel-preconnect-http.html:17
>> +setTimeout(finishJSTest, 1000);
> 
> Since we're waiting for a completion handler and writing to the console and checking that, couldn't we make an internals function that checks to see if the message has been written to the console?  I don't particularly like the tests that have to be slow enough to not time out on the bots

Good ideal. I know of other tests that have fairly large timeouts because they need to wait for a console message. Adding such functionality would help a lot.
Comment 16 Chris Dumez 2017-09-27 09:31:30 PDT
Comment on attachment 321867 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/PreconnectTask.cpp:118
>>> +    didFinish(cannotShowURLError(ResourceRequest { }));
>> 
>> Can't we make an error that knows the URL we were given?
> 
> We can but it means we'll need an extra member. I did not think it was worth it (WebCore already knows the URL since there is no redirect involved) but OK.

Actually, I think I can get the URL from m_task.
Comment 17 Geoffrey Garen 2017-09-27 10:48:38 PDT
Comment on attachment 321867 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:90
> +- (void)_preconnectToServer:(NSURL *)serverURL WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Are there any restrictions on what kind of URL you can provide? Does this have to be domain only? Or a valid dummy resource?
Comment 18 Chris Dumez 2017-09-27 10:50:14 PDT
(In reply to Geoffrey Garen from comment #17)
> Comment on attachment 321867 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321867&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:90
> > +- (void)_preconnectToServer:(NSURL *)serverURL WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Are there any restrictions on what kind of URL you can provide? Does this
> have to be domain only? Or a valid dummy resource?

Can be any HTTP/HTTPS URL but technically with only need the origin (scheme + host + port).
Comment 19 Chris Dumez 2017-09-27 12:43:05 PDT
Created attachment 322002 [details]
Patch
Comment 20 Alex Christensen 2017-09-27 16:05:29 PDT
Comment on attachment 322002 [details]
Patch

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

> Source/WebKit/NetworkProcess/PreconnectTask.cpp:56
> +    m_task = NetworkDataTask::create(NetworkSession::defaultSession(), *this, parameters);

Pass the SessionID as a parameter.
Comment 21 Chris Dumez 2017-09-27 16:30:09 PDT
Created attachment 322040 [details]
Patch
Comment 22 Alex Christensen 2017-09-27 21:26:04 PDT
Comment on attachment 322040 [details]
Patch

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

> Source/WebCore/bindings/scripts/IDLAttributes.json:189
> -            "contextsAllowed": ["interface", "dictionary", "enum"],
> +            "contextsAllowed": ["interface", "dictionary", "enum", "callback-function"],

I don't know why you're adding this.

> LayoutTests/platform/mac-wk2/TestExpectations:796
> +# Link preconnect is disabled on ElCapitan because it does not use NETWORK_SESSION.

What about Sierra and iOS10 where the SPI doesn't exist?
Comment 23 Chris Dumez 2017-09-27 21:29:16 PDT
Comment on attachment 322040 [details]
Patch

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

>> Source/WebCore/bindings/scripts/IDLAttributes.json:189
>> +            "contextsAllowed": ["interface", "dictionary", "enum", "callback-function"],
> 
> I don't know why you're adding this.

This is because I added support for using ExportMacro IDL extended attribute on callback functions. The json needed to be updated to reflect that of the IDL parser would treat the IDL as invalid. I needed to use ExportMacro on StringCallback interface so that I could use it in Internals.
Comment 24 Chris Dumez 2017-09-28 09:16:23 PDT
Created attachment 322086 [details]
Patch
Comment 25 Chris Dumez 2017-09-28 09:21:39 PDT
Created attachment 322087 [details]
Patch
Comment 26 Chris Dumez 2017-09-28 10:05:32 PDT
Comment on attachment 322087 [details]
Patch

Clearing flags on attachment: 322087

Committed r222613: <http://trac.webkit.org/changeset/222613>
Comment 27 Chris Dumez 2017-09-28 10:05:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 WebKit Commit Bot 2017-09-28 10:14:32 PDT
The commit-queue encountered the following flaky tests while processing attachment 322087 [details]:

media/modern-media-controls/seek-backward-support/seek-backward-support.html bug 174916 (authors: graouts@apple.com, mcatanzaro@igalia.com, and ryanhaddad@apple.com)
The commit-queue is continuing to process your patch.
Comment 29 Matt Lewis 2017-09-29 11:31:42 PDT
The test http/tests/preconnect/link-rel-preconnect-https.html that was added with this change has been flaky from the time it has been added.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fpreconnect%2Flink-rel-preconnect-https.html

https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r222650%20(142)/results.html
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/142

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".
Comment 30 Alexey Proskuryakov 2019-04-18 09:43:57 PDT
*** Bug 155835 has been marked as a duplicate of this bug. ***