The default bug view has changed. See this FAQ.

Implement <link rel="preconnect">

RESOLVED FIXED in Firefox 39

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

({dev-doc-complete})

32 Branch
mozilla39
x86_64
Linux
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox39 fixed, relnote-firefox 39+)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

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

Comment 2

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

Comment 3

2 years ago
Created attachment 8568008 [details] [diff] [review]
implement link rel=preconnect
Attachment #8568008 - Flags: review?(bugs)
(Assignee)

Comment 4

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f10034f530d7
(Assignee)

Comment 5

2 years ago
Created attachment 8568547 [details] [diff] [review]
implement link rel=preconnect
Attachment #8568547 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8568008 - Attachment is obsolete: true
Attachment #8568008 - Flags: review?(bugs)
(Assignee)

Comment 6

2 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
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.
Attachment #8568547 - Flags: review?(bugs) → review-
(Assignee)

Comment 8

2 years ago
Created attachment 8570463 [details] [diff] [review]
implement link rel=preconnect
Attachment #8570463 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8568547 - Attachment is obsolete: true
(Assignee)

Comment 9

2 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
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.
Attachment #8570463 - Flags: review?(bugs) → review+

Comment 11

2 years ago
@Olli, Patrick: PTAL at https://github.com/w3c/resource-hints/issues/25#issuecomment-76475919, curious to hear your thoughts.
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)

Comment 12

2 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 :)
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.
(Assignee)

Comment 14

2 years ago
Created attachment 8575475 [details] [diff] [review]
implement link rel=preconnect
(Assignee)

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/020545d52adf
(Assignee)

Updated

2 years ago
Attachment #8570463 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8575475 - Flags: review+
(Assignee)

Comment 16

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

Comment 17

2 years ago
https://hg.mozilla.org/mozilla-central/rev/020545d52adf
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Comment 18

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

Comment 19

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

Comment 20

2 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/
(Assignee)

Comment 21

2 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)
(Assignee)

Comment 22

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

Comment 23

2 years ago
Created attachment 8578072 [details] [diff] [review]
ioservice have speculative connect use proxy-resolve2()
Attachment #8578072 - Flags: review?(hurley)

Comment 24

2 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?
Attachment #8578072 - Flags: review?(hurley) → review+
(Assignee)

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b22d25c82c
https://hg.mozilla.org/mozilla-central/rev/25b22d25c82c
Keywords: dev-doc-needed
(Assignee)

Comment 27

2 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.
Flags: needinfo?(bugs)
(Assignee)

Comment 28

2 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
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
(Assignee)

Comment 30

2 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.
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.
Flags: needinfo?(bugs)
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

2 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

Comment 34

2 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.
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)]:
relnote-firefox: --- → 39+
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.
(Assignee)

Updated

2 years ago
Blocks: 1150136

Comment 37

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

2 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?
https://wiki.mozilla.org/RapidRelease/Calendar
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.
(Assignee)

Comment 42

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

Comment 43

2 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
Doc updated:
https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types
and
https://developer.mozilla.org/en-US/Firefox/Releases/39#HTML
Keywords: dev-doc-needed → dev-doc-complete

Comment 45

2 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?
(Assignee)

Comment 46

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

Comment 47

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

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

Comment 48

2 years ago
sgtm, thanks Patrick!
(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.
Flags: needinfo?(jypenator)
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.
Flags: needinfo?(jypenator)
(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!
You need to log in before you can comment on or make changes to this bug.