WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
WIP Patch
(41.02 KB, patch)
2017-09-25 16:57 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
WIP Patch
(41.07 KB, patch)
2017-09-25 17:02 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
WIP Patch
(42.72 KB, patch)
2017-09-26 10:14 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
WIP Patch
(44.12 KB, patch)
2017-09-26 10:20 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
WIP Patch
(49.19 KB, patch)
2017-09-26 12:02 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
WIP Patch
(57.10 KB, patch)
2017-09-26 13:10 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(67.10 KB, patch)
2017-09-26 13:39 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(71.42 KB, patch)
2017-09-26 14:14 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(82.21 KB, patch)
2017-09-27 12:43 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(82.83 KB, patch)
2017-09-27 16:30 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(82.85 KB, patch)
2017-09-28 09:16 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(83.65 KB, patch)
2017-09-28 09:21 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(12)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/33141380
>
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
Created
attachment 321861
[details]
Patch
Chris Dumez
Comment 13
2017-09-26 14:14:07 PDT
Created
attachment 321867
[details]
Patch
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
Created
attachment 322002
[details]
Patch
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
Created
attachment 322040
[details]
Patch
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
Created
attachment 322086
[details]
Patch
Chris Dumez
Comment 25
2017-09-28 09:21:39 PDT
Created
attachment 322087
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug