RESOLVED FIXED 177474
Add support for <link rel=preconnect>
https://bugs.webkit.org/show_bug.cgi?id=177474
Summary Add support for <link rel=preconnect>
Chris Dumez
Reported 2017-09-25 16:45:00 PDT
Add support for <link rel=preconnect>: - https://w3c.github.io/resource-hints/#preconnect
Attachments
WIP Patch (37.65 KB, patch)
2017-09-25 16:50 PDT, Chris Dumez
no flags
WIP Patch (41.02 KB, patch)
2017-09-25 16:57 PDT, Chris Dumez
no flags
WIP Patch (41.07 KB, patch)
2017-09-25 17:02 PDT, Chris Dumez
no flags
WIP Patch (42.72 KB, patch)
2017-09-26 10:14 PDT, Chris Dumez
no flags
WIP Patch (44.12 KB, patch)
2017-09-26 10:20 PDT, Chris Dumez
no flags
WIP Patch (49.19 KB, patch)
2017-09-26 12:02 PDT, Chris Dumez
no flags
WIP Patch (57.10 KB, patch)
2017-09-26 13:10 PDT, Chris Dumez
no flags
Patch (67.10 KB, patch)
2017-09-26 13:39 PDT, Chris Dumez
no flags
Patch (71.42 KB, patch)
2017-09-26 14:14 PDT, Chris Dumez
no flags
Patch (82.21 KB, patch)
2017-09-27 12:43 PDT, Chris Dumez
no flags
Patch (82.83 KB, patch)
2017-09-27 16:30 PDT, Chris Dumez
no flags
Patch (82.85 KB, patch)
2017-09-28 09:16 PDT, Chris Dumez
no flags
Patch (83.65 KB, patch)
2017-09-28 09:21 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2017-09-25 16:50:17 PDT
Created attachment 321766 [details] WIP Patch
Build Bot
Comment 2 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.
Chris Dumez
Comment 3 2017-09-25 16:57:10 PDT
Created attachment 321768 [details] WIP Patch
Build Bot
Comment 4 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.
Chris Dumez
Comment 5 2017-09-25 17:02:44 PDT
Created attachment 321770 [details] WIP Patch
Build Bot
Comment 6 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.
Chris Dumez
Comment 7 2017-09-26 10:14:30 PDT
Created attachment 321833 [details] WIP Patch
Chris Dumez
Comment 8 2017-09-26 10:20:31 PDT
Created attachment 321835 [details] WIP Patch
Chris Dumez
Comment 9 2017-09-26 10:54:43 PDT
Chris Dumez
Comment 10 2017-09-26 12:02:24 PDT
Created attachment 321853 [details] WIP Patch
Chris Dumez
Comment 11 2017-09-26 13:10:59 PDT
Created attachment 321859 [details] WIP Patch
Chris Dumez
Comment 12 2017-09-26 13:39:48 PDT
Chris Dumez
Comment 13 2017-09-26 14:14:07 PDT
Alex Christensen
Comment 14 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
Chris Dumez
Comment 15 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.
Chris Dumez
Comment 16 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.
Geoffrey Garen
Comment 17 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?
Chris Dumez
Comment 18 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).
Chris Dumez
Comment 19 2017-09-27 12:43:05 PDT
Alex Christensen
Comment 20 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.
Chris Dumez
Comment 21 2017-09-27 16:30:09 PDT
Alex Christensen
Comment 22 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?
Chris Dumez
Comment 23 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.
Chris Dumez
Comment 24 2017-09-28 09:16:23 PDT
Chris Dumez
Comment 25 2017-09-28 09:21:39 PDT
Chris Dumez
Comment 26 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>
Chris Dumez
Comment 27 2017-09-28 10:05:35 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 28 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.
Matt Lewis
Comment 29 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".
Alexey Proskuryakov
Comment 30 2019-04-18 09:43:57 PDT
*** Bug 155835 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.