Bug 177474 – Add support for <link rel=preconnect>

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Add support for <link rel=preconnect>:
- https://w3c.github.io/resource-hints/#preconnect

[Chris Dumez]

Created attachment 321766 [details]
WIP Patch

[Build Bot]

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.

[Chris Dumez]

Created attachment 321768 [details]
WIP Patch

[Build Bot]

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.

[Chris Dumez]

Created attachment 321770 [details]
WIP Patch

[Build Bot]

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.

[Chris Dumez]

Created attachment 321833 [details]
WIP Patch

[Chris Dumez]

Created attachment 321835 [details]
WIP Patch

[Chris Dumez]

<rdar://problem/33141380>

[Chris Dumez]

Created attachment 321853 [details]
WIP Patch

[Chris Dumez]

Created attachment 321859 [details]
WIP Patch

[Chris Dumez]

Created attachment 321861 [details]
Patch

[Chris Dumez]

Created attachment 321867 [details]
Patch

[Alex Christensen]

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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Geoffrey Garen]

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?

[Chris Dumez]

(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).

[Chris Dumez]

Created attachment 322002 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

Created attachment 322040 [details]
Patch

[Alex Christensen]

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?

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 322086 [details]
Patch

[Chris Dumez]

Created attachment 322087 [details]
Patch

[Chris Dumez]

Comment on attachment 322087 [details]
Patch

Clearing flags on attachment: 322087

Committed r222613: <http://trac.webkit.org/changeset/222613>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

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: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[Matt Lewis]

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".

[Alexey Proskuryakov]

*** Bug 155835 has been marked as a duplicate of this bug. ***