Last Comment Bug 1135160 - Implement <link rel="preconnect">
: Implement <link rel="preconnect">
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 32 Branch
: x86_64 Linux
-- normal (vote)
: mozilla39
Assigned To: Patrick McManus [:mcmanus]
:
: Andrew Overholt [:overholt]
Mentors:
https://w3c.github.io/resource-hints/...
Depends on:
Blocks: 1150136
  Show dependency treegraph
 
Reported: 2015-02-20 10:38 PST by Patrick McManus [:mcmanus]
Modified: 2015-12-14 16:05 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
39+


Attachments
implement link rel=preconnect (17.21 KB, patch)
2015-02-23 09:26 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
implement link rel=preconnect (17.34 KB, patch)
2015-02-24 07:22 PST, Patrick McManus [:mcmanus]
bugs: review-
Details | Diff | Splinter Review
implement link rel=preconnect (18.14 KB, patch)
2015-02-27 05:47 PST, Patrick McManus [:mcmanus]
bugs: review+
Details | Diff | Splinter Review
implement link rel=preconnect (18.17 KB, patch)
2015-03-10 12:11 PDT, Patrick McManus [:mcmanus]
mcmanus: review+
Details | Diff | Splinter Review
ioservice have speculative connect use proxy-resolve2() (1.19 KB, patch)
2015-03-16 09:00 PDT, Patrick McManus [:mcmanus]
hurley: review+
Details | Diff | Splinter Review

Description User image Patrick McManus [:mcmanus] 2015-02-20 10:38:38 PST
preconnect is a subset of resource hints: http://w3c.github.io/resource-hints/#anonymizing-redirect-preconnect

this bug is not about prefetching or prerendering.

the preconnect would use the existing nsISpeculativeConnect interface made available to priv'd chrome - that controls when it is suitable to make the connection or not.

CSP would not apply to this directive.. see this thread https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/CM5BaP6uVvE/aIasIi1F7pEJ

we would also do the corresponding http response header.

we would not do the pr or loadpolicy attributes at this point.
Comment 1 User image Ilya Grigorik 2015-02-20 10:47:02 PST
> we would not do the pr or loadpolicy attributes at this point.

That's consistent with our implementation in Chrome. +1.
Comment 2 User image Ilya Grigorik 2015-02-23 08:29:02 PST
Patrick, just FYI: https://bugzilla.mozilla.org/show_bug.cgi?id=1134648#c8 -- same applies to preconnect. That is, we should make sure dynamically inserted preconnect hints are executed.
Comment 3 User image Patrick McManus [:mcmanus] 2015-02-23 09:26:30 PST
Created attachment 8568008 [details] [diff] [review]
implement link rel=preconnect
Comment 4 User image Patrick McManus [:mcmanus] 2015-02-23 09:28:20 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f10034f530d7
Comment 5 User image Patrick McManus [:mcmanus] 2015-02-24 07:22:28 PST
Created attachment 8568547 [details] [diff] [review]
implement link rel=preconnect
Comment 6 User image Patrick McManus [:mcmanus] 2015-02-24 07:24:57 PST
new patch has the same gecko bits as the last one, if you started looking at it.. just updated the test to push/pop a pref instead of using clearUserPref (as the test harness had already set it).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb5bb35a630
Comment 7 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2015-02-26 13:03:12 PST
Comment on attachment 8568547 [details] [diff] [review]
implement link rel=preconnect

So of course there isn't any kind of spec for any dynamic changes here.
Is 
var el = document.createElement("link")
el.rel = "preconnect";
el.href = "some url";
supposed to trigger preconnection? That is what the patch does.

I would assume one would need to also add the element to the document before the preconnection is triggered.
So, you need to add something to HTMLLinkElement::BindToTree.
Somewhere there check
if (IsInComposedDoc()) {
  ... update 
}


> HTMLLinkElement::SetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>                          nsIAtom* aPrefix, const nsAString& aValue,
>                          bool aNotify)
...
>         UpdateImport();
>+      } else if (linkTypes & ePRECONNECT) {
>+        UpdatePreconnect();
So we probably don't want to UpdatePreconnect here if the element isn't in (composed) document.
Comment 8 User image Patrick McManus [:mcmanus] 2015-02-27 05:47:19 PST
Created attachment 8570463 [details] [diff] [review]
implement link rel=preconnect
Comment 9 User image Patrick McManus [:mcmanus] 2015-02-27 05:48:58 PST
s.
> 
> I would assume one would need to also add the element to the document before
> the preconnection is triggered.


thank you!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aa92f1fa2d8
Comment 10 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2015-02-27 08:42:41 PST
Comment on attachment 8570463 [details] [diff] [review]
implement link rel=preconnect


>+  // construct URI using document charset
>+  const nsACString &charset = mDocument->GetDocumentCharacterSet();
Nit, const nsACString& charset

>+  if (IsInUncomposedDoc()) {
>+    UpdatePreconnect();
>+  }
I think we want IsInComposedDoc here


>+HTMLLinkElement::UpdatePreconnect()
>+{
>+  // rel type should be preconnect
>+  nsAutoString rel;
>+  GetAttr(kNameSpaceID_None, nsGkAtoms::rel, rel);
And then return early here if we don't have rel attribute.
if (!GetAttr(kNameSpaceID_None, nsGkAtoms::rel, rel)) {
  return;
}


>+      } else if (linkTypes & ePRECONNECT && IsInComposedDoc()) {
} else if ((linkTypes & ePRECONNECT) && IsInComposedDoc()) {


Interesting test, looks handy.
Comment 11 User image Ilya Grigorik 2015-02-27 13:48:02 PST
@Olli, Patrick: PTAL at https://github.com/w3c/resource-hints/issues/25#issuecomment-76475919, curious to hear your thoughts.
Comment 12 User image Ilya Grigorik 2015-03-09 22:03:03 PDT
Patrick: is this now in nightly, or is there a build to test this stuff? I'm working on some demos and would love to play with it :)
Comment 13 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2015-03-10 04:26:05 PDT
Based on the comment 9, http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mcmanus@ducksong.com-0aa92f1fa2d8/ should have the patch attached to the bug.
Comment 14 User image Patrick McManus [:mcmanus] 2015-03-10 12:11:10 PDT
Created attachment 8575475 [details] [diff] [review]
implement link rel=preconnect
Comment 15 User image Patrick McManus [:mcmanus] 2015-03-10 12:12:17 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/020545d52adf
Comment 16 User image Patrick McManus [:mcmanus] 2015-03-10 12:15:04 PDT
(In reply to Ilya Grigorik from comment #12)
> Patrick: is this now in nightly, or is there a build to test this stuff? I'm
> working on some demos and would love to play with it :)

the try build is a possibility - it will probably also be in tomorrow's nightly. (assuming it sticks.)
Comment 18 User image Ilya Grigorik 2015-03-12 15:16:52 PDT
Running some tests...

1) HTTP preconnect stops at DNS prefetch? Why?
* test page: http://jsbin.com/bikeru/2/quiet
* results: http://www.webpagetest.org/result/150312_68_1HHF/
-- https://www.cloudshark.org/captures/7c8a0c8dc6ff?filter=tcp.stream%3D%3D8
-- https://www.cloudshark.org/captures/7c8a0c8dc6ff?filter=tcp.stream%3D%3D12

2) HTTPS preconnect (via declared element)
* test page: http://jsbin.com/duwomo/6/quiet
* WPT is not playing well with visualizing HTTPS + FF? 
-- http://www.webpagetest.org/result/150312_E4_1H7Y/1/details/
* Looking at tcpdump: https://www.cloudshark.org/captures/fd4c7f6f08f8?filter=tcp.stream%20%3D%3D%209
-- HTML request completes at ~5s
-- Full TCP+TLS connection is established ~200ms later: https://www.cloudshark.org/captures/fd4c7f6f08f8?filter=tcp.stream%20%3D%3D%2010
-- Omitting preconnect hint doesn't show same TCP+TLS preconnect (as expected): https://www.cloudshark.org/captures/7a4bef128152?filter=tcp.stream%3D%3D7

3) "reactive preconnect" for HTTP destination
- test page: http://jsbin.com/duwomo/9/quiet 
- http://www.webpagetest.org/result/150312_PY_1HPH/1/details/ 
-- seems to work as advertised .. \o/

4) "reactive preconnect" for HTTPS destination 
- test page: http://jsbin.com/duwomo/10/quiet
- http://www.webpagetest.org/result/150312_43_1HSR/1/details/
-- similarly, seems to work as advertised.

-------------

Patrick, any idea why (1) stops at DNS prefetch? It'd be nice to see the TCP handshake finished to unblock the font download in that example.
Comment 19 User image Patrick McManus [:mcmanus] 2015-03-13 06:37:51 PDT
Ilya, thanks for the tests. I haven't looked at the details yet - I just want to clarify whether test 2 works as expected or not (other than the WPT interaction).. It sounds like a pass, but I wanted to clarify.

I'll take a look at #1.
Comment 20 User image Ilya Grigorik 2015-03-13 13:32:39 PDT
Yes, I think #2 is working; I believe the HTTPS requests are not showing up in WPT due to missing HTTP/2 decode logic. When I look at the tcpdump, I do see the handshake for preconnect host firing at the same time as the CSS request goes out. Here's another test run for this:

- http://www.webpagetest.org/result/150313_J0_17N3/1/details/
- https://www.cloudshark.org/captures/16a71c618e23?filter=tcp.stream%3D%3D8 (fonts.googleapis.com)
- https://www.cloudshark.org/captures/16a71c618e23?filter=tcp.stream%3D%3D9 (fonts.gstatic.com)

Both fonts connections fire at ~same time; looks like preconnect is working.

However, same page but replacing https:// with http://.. (test #1) shows that preconnect stops at DNS:
- http://www.webpagetest.org/result/150313_J0_17N3/1/details/
Comment 21 User image Patrick McManus [:mcmanus] 2015-03-13 14:37:49 PDT
I have a try build (just started) with a change that might help case number 1.. do you want to try it, Ilya?

Oh, since you have to use nightly here it might make sense to test things with e10s both on (which it is by default only on the nightly channel) and off. It's pretty much the first thing on the preference pane. Its still the trigger of a lot of new bugs (which is why its on by default on nightly)
Comment 23 User image Patrick McManus [:mcmanus] 2015-03-16 09:00:24 PDT
Created attachment 8578072 [details] [diff] [review]
ioservice have speculative connect use proxy-resolve2()
Comment 24 User image Ilya Grigorik 2015-03-17 09:18:50 PDT
Hmm, I can't seem to run the OSX version in above build - reports that it's "damaged". 

FWIW, when I try running the http + https preconnects in latest Nightly, the network waterfall within FF (not WPT) seems to indicate that neither is being picked up:

- https://www.evernote.com/shard/s1/sh/38c80c7b-c16c-4329-b43b-8863374249ed/a054fac56f453f7029b7a15c1e0e2594
- https://www.evernote.com/shard/s1/sh/8807f7c1-e3fe-449c-9505-48fd898b1a94/b73ea69fa034c83096427a07598f856f

Am I missing anything?
Comment 25 User image Patrick McManus [:mcmanus] 2015-03-18 05:14:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b22d25c82c
Comment 26 User image Wes Kocher (:KWierso) 2015-03-18 15:04:22 PDT
https://hg.mozilla.org/mozilla-central/rev/25b22d25c82c
Comment 27 User image Patrick McManus [:mcmanus] 2015-03-20 13:40:38 PDT
I can see some of what Ilya is seeing with his test case #1. The directive does work (i.e. if you remove any references to the host other than the preconnect you do see a connect) - it just doesn't happen in this case as fast as we would like or as fast as it needs to be truly useful.

There are a few things in play
 1] The IsInComposedDoc() semantics :smaug was interested in, means this happens a bit later than HTMLLinkElement.cpp is really aware of parsing it.
 2] The implementation of pre-connect is in general out of the tree builder, while the stylesheet fetcher (and I suspect the font fetcher) is initiated from the speculative loader.. so the stylesheet and fonts get in front of the preconnect in the queue
 3] e10s might also be playing a role here on nightly.. not entirely sure.

I suggest we abandon the isincomposeddoc() semantic. The presence of the attribute means a hint to connect, which gecko does not have to honor. (indeed it never allows a new one if there is an idle connect or an in progress preconnect anyhow). Additionally I can add preconnect to the speculative loader - It seems less scary than preloadimage(), which we already allow.

Olli - what do you think? That would obviously impact the feedback to Ilya on the spec as well.
Comment 28 User image Patrick McManus [:mcmanus] 2015-03-20 13:46:03 PDT
I should add that it seems like just adding it to the speculative loader would probably be sufficient.. but that seems a bit inconsistent
Comment 29 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-03-20 13:51:52 PDT
Could we, and would it make sense to, use IsInComposedDoc semantics for <link> elements created using the DOM? But use the HTML parser to kick off preconnects for <link> elements that come from the markup? I believe that the HTML parser generally sees elements much before we even create the HTMLLinkElement since the parser lives on a separate thread from where we create the HTMLLinkElement
Comment 30 User image Patrick McManus [:mcmanus] 2015-03-20 14:01:34 PDT
(In reply to Jonas Sicking (:sicking) from comment #29)
> Could we, and would it make sense to, use IsInComposedDoc semantics for
> <link> elements created using the DOM? But use the HTML parser to kick off
> preconnects for <link> elements that come from the markup? I believe that
> the HTML parser generally sees elements much before we even create the
> HTMLLinkElement since the parser lives on a separate thread from where we
> create the HTMLLinkElement

That's what I was trying to suggest in comment 28.. I think it would give the desired effect it just seems a little inconsistent. I'm ok with it though if you and :smaug are ok.
Comment 31 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2015-03-20 14:12:06 PDT
In this case I'm ok with almost any solution which is just spec'ed. But relying on unspecified behavior, where blink does X and Gecko does Y isn't an option.

Currently the spec is just too vague.


But yeah, comment 29 sounds rather good.
Comment 32 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-03-20 14:23:33 PDT
Ilya: Could you make sure to make the spec define exactly what DOM actions indicate that the page wants to generate a dynamic preconnect. Specifically, does

var link = document.createElement("link");
link.rel = "preconnect";
link.href = ...;

indicate that the page wants to do a preconnect? Or does the page also have to do

document.head.appendChild(link);

?

Also, does it matter where in the DOM the element lives? I.e. if it's inserted into the head or into the body, or somewhere else. Making this explicit in the spec would be good.

Of course, in all of these situations the browser always have the option *not* to preconnect even if the page asks for it. The question here is to define when "the page asks for it", not to define exactly when preconnect will actually happen since that's subject to UA policies.
Comment 33 User image Ilya Grigorik 2015-03-20 15:38:20 PDT
Jonas: Yep, I think we already reached agreement on this one, I just need to land the update - see: https://github.com/w3c/resource-hints/issues/25#issuecomment-76849410 - tl;dr: you have to appendChild, location doesn't matter. 

Re, consistency: Pat is adding preconnect support into Blink preloader [1], so I think we're in good shape.


[1] https://code.google.com/p/chromium/issues/detail?id=450682
Comment 34 User image Ilya Grigorik 2015-03-23 10:42:33 PDT
Patrick: a test case for preload support @ http://resourcehints.info/preconnect/preload-support.html

Without preload scanner support the preconnect is a no-op: preloader skips over it and fetches the CSS stylesheet, but even then the connection to fonts.gstatic.com is delayed until the blocking script has been downloaded and CSS is processed... which is really painful! This is an important case to cover.
Comment 35 User image Liz Henry (:lizzard) (needinfo? me) 2015-03-30 15:14:12 PDT
Release Note Request (optional, but appreciated)
[Why is this notable]: Useful tip for developers to improve page load time
[Suggested wording]:  Implement <link rel="preconnect">
[Links (documentation, blog post, etc)]:
Comment 36 User image Liz Henry (:lizzard) (needinfo? me) 2015-03-31 10:50:40 PDT
Please need-info me when there is a good developer documentation page or a post to link to, so I can update the release note.
Comment 37 User image Ilya Grigorik 2015-05-26 09:53:32 PDT
This bug marks preconnect for M39, but preload scanner support is set for M41. Any chance we can ship both as a bundle? Sooner milestone would be a nice plus :)
Comment 38 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-05-26 11:33:35 PDT
If we want to ship both at the same time then that likely means holding back base support until Firefox 41. Not shipping both at Firefox 39.

Is the argument here the ability to feature-detect? I.e. is it beneficial to hold the base support until 41?
Comment 39 User image Ilya Grigorik 2015-05-26 12:54:18 PDT
- Speaking of feature detection, that's an open issue [1].
- We also have the "crossorigin" discussion outstanding [2].

[1] https://github.com/w3c/preload/issues/7#issuecomment-99728174
[2] https://github.com/w3c/resource-hints/pull/33#issuecomment-105604897

That said, neither of the above are "blocking": preconnect is an optimization _hint_, so lack of preloader or crossorigin support does not break the functional contract. That said, my worry is on the efficacy side of things.. developers using it and concluding that the resulting gains are "meh" without the right context about upcoming improvements, etc. As such, I'm on the fence.. I'd like to see it shipped ASAP, but I also want to make sure we do it right :)

Q: What is the M41 branch cutoff and expected ship date? I'm assuming we're already past due for M39/40 branch cutoffs?
Comment 40 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2015-05-26 12:58:15 PDT
https://wiki.mozilla.org/RapidRelease/Calendar
Comment 41 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2015-05-26 13:44:12 PDT
Also, I think it's Patrick's call when this should or should not ship. I think we'd need rather strong arguments at this point to back out the feature for the Firefox 39 release.
Comment 42 User image Patrick McManus [:mcmanus] 2015-06-01 08:49:14 PDT
sorry for the delay - been on vacation.

I've got at least one very interested party in this feature that doesn't involve the preload scanner (they are looking for a js/dom mechanism to drive the preconnect).. so given that this is just an optimization I don't really mind it getting better over a few releases..
Comment 43 User image Ilya Grigorik 2015-06-01 11:23:45 PDT
SGTM, we can ship and iterate. Looks like the current plan is:

- M39: preconnect
- M41: preloader support for preconnect (any chance we can move this to M40? :-))
- TBD: crossorigin support for preconnect
Comment 45 User image Ilya Grigorik 2015-06-10 13:43:08 PDT
Re, crossorigin: attribute added to the spec, for details see..
- https://github.com/w3c/resource-hints/pull/33#issuecomment-110905978
- http://w3c.github.io/resource-hints/#preconnect

Patrick, I believe you had a patch for it already? Would M41 be a plausible target, alongside preloader support?
Comment 46 User image Patrick McManus [:mcmanus] 2015-06-11 08:35:34 PDT
(In reply to Ilya Grigorik from comment #45)
> Re, crossorigin: attribute added to the spec, for details see..
> - https://github.com/w3c/resource-hints/pull/33#issuecomment-110905978
> - http://w3c.github.io/resource-hints/#preconnect
> 
> Patrick, I believe you had a patch for it already? Would M41 be a plausible
> target, alongside preloader support?

I'll get it started and we'll see what happens.
Comment 47 User image Patrick McManus [:mcmanus] 2015-06-12 06:36:17 PDT
> 
> I'll get it started and we'll see what happens.

https://bugzilla.mozilla.org/show_bug.cgi?id=1174152
Comment 48 User image Ilya Grigorik 2015-06-12 07:52:20 PDT
sgtm, thanks Patrick!
Comment 49 User image John Daggett (:jtd) 2015-11-15 21:20:26 PST
(In reply to Jean-Yves Perrier [:teoli] from comment #44)
> Doc updated:
> https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types
> and
> https://developer.mozilla.org/en-US/Firefox/Releases/39#HTML

Jean-Yves, don't we need to update the table in the docs for the <link> element?

https://developer.mozilla.org/en/docs/Web/HTML/Element/link

The table lists the preconnect attribute as available in Chrome 46 but not in Firefox.
Comment 50 User image Jean-Yves Perrier [:teoli] 2015-12-14 02:27:06 PST
Good catch.

What happened is that preconnect and dns-prefetch (and their related compat info) have been added to <link> as attributes. This was incorrect (they are values of the rel attribute) and I have fixed this by removing the entries and added the compat data to: https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types

Thanks a lot.
Comment 51 User image John Daggett (:jtd) 2015-12-14 16:05:49 PST
(In reply to Jean-Yves Perrier [:teoli] from comment #50)

> What happened is that preconnect and dns-prefetch (and their related compat
> info) have been added to <link> as attributes. This was incorrect (they are
> values of the rel attribute) and I have fixed this by removing the entries
> and added the compat data to:
> https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types

Great!

Note You need to log in before you can comment on or make changes to this bug.