Patrick McManus [:mcmanus] Assignee
	Description • 9 years ago

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.

	Ilya Grigorik
	Comment 1 • 9 years ago

> we would not do the pr or loadpolicy attributes at this point.

That's consistent with our implementation in Chrome. +1.

	Ilya Grigorik
	Comment 2 • 9 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 4 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f10034f530d7

	Patrick McManus [:mcmanus] Assignee
	Comment 6 • 9 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 9 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 9 • 9 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 9 years ago

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.

	Ilya Grigorik
	Comment 11 • 9 years ago

@Olli, Patrick: PTAL at https://github.com/w3c/resource-hints/issues/25#issuecomment-76475919, curious to hear your thoughts.

	Ilya Grigorik
	Comment 12 • 9 years ago

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 :)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 9 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/020545d52adf

	Patrick McManus [:mcmanus] Assignee
	Comment 16 • 9 years ago

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

	Ekanan Ketunuti
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/020545d52adf

	Ilya Grigorik
	Comment 18 • 9 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 19 • 9 years ago

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.

	Ilya Grigorik
	Comment 20 • 9 years ago

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/

	Patrick McManus [:mcmanus] Assignee
	Comment 21 • 9 years ago

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)

	Patrick McManus [:mcmanus] Assignee
	Comment 22 • 9 years ago

that try build from comment 21
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/mcmanus@ducksong.com-0e9fcf164bd5

	Ilya Grigorik
	Comment 24 • 9 years ago

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?

	Patrick McManus [:mcmanus] Assignee
	Comment 25 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/25b22d25c82c

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 26 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/25b22d25c82c

	Patrick McManus [:mcmanus] Assignee
	Comment 27 • 9 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 28 • 9 years ago

I should add that it seems like just adding it to the speculative loader would probably be sufficient.. but that seems a bit inconsistent

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 29 • 9 years ago

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

	Patrick McManus [:mcmanus] Assignee
	Comment 30 • 9 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 31 • 9 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 32 • 9 years ago

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.

	Ilya Grigorik
	Comment 33 • 9 years ago

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

	Ilya Grigorik
	Comment 34 • 9 years ago

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.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 35 • 9 years ago

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)]:

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 36 • 9 years ago

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.

	Ilya Grigorik
	Comment 37 • 9 years ago

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 :)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 38 • 9 years ago

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?

	Ilya Grigorik
	Comment 39 • 9 years ago

- 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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 40 • 9 years ago

https://wiki.mozilla.org/RapidRelease/Calendar

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 41 • 9 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 42 • 9 years ago

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

	Ilya Grigorik
	Comment 43 • 9 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 44 • 9 years ago

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types
and
https://developer.mozilla.org/en-US/Firefox/Releases/39#HTML

	Ilya Grigorik
	Comment 45 • 9 years ago

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?

	Patrick McManus [:mcmanus] Assignee
	Comment 46 • 9 years ago

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

	Patrick McManus [:mcmanus] Assignee
	Comment 47 • 9 years ago

> 
> I'll get it started and we'll see what happens.

https://bugzilla.mozilla.org/show_bug.cgi?id=1174152

	Ilya Grigorik
	Comment 48 • 9 years ago

sgtm, thanks Patrick!

	John Daggett (:jtd)
	Comment 49 • 9 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 50 • 9 years ago

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.

	John Daggett (:jtd)
	Comment 51 • 9 years ago

(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!