preload scanner support for rel=preconnect

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

32 Branch
mozilla41
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
over in bug 1135160 we implemented support for link rel=preconnect for composed documents.

It turns out to be important for some use cases, e.g. those involving hostnames that will found inside later loaded css's, to also do preconnect during the preload scanner before the document is composed too.

After composition, during dom manipulations, preload should be limited to isincomposeddoc() elements.

Comment 1

3 years ago
For convenience, copying reply from parent thread:

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

Comment 2

3 years ago
the test case in comment 1 turns out to be tricky because it is trying to preconnect for a font load.

by spec font loads are done in anonymous mode, but the preconnect syntax doesn't provide any indication of anonymous or not - indeed anonymous connections are hashed separately from normal connections. So the preconnect to non-anonymous does happen, but it isn't used by the font loader.

I'm not entirely sure how to square that circle - one of the differences between the two types is whether they use ssl client certs, and iirc there may be important differences in the ssl resumption cache partitions that they use.. so it can't be late bound other than to guess at one and then see if its the right one at time of use. Which is currently what happens.

Ilya, how does chrome deal with that?

If I change the preconnect logic to open both an anonymous and non-anonymous connection then things work fine.. but that's not really optimal :)
(Assignee)

Updated

3 years ago
Flags: needinfo?(igrigorik)

Comment 3

3 years ago
Yep, we're trying to square the same circle wrt "anonymous" requests:
- https://github.com/w3c/resource-hints/issues/32
- https://code.google.com/p/chromium/issues/detail?id=468005#c14

To unpack this, I think there are several different issues at hand:

a) Preload + preconnect is still an open and valid issue. I've updated the test case to avoid the anonymous case: https://igrigorik.github.io/resourcehints.info/preconnect/preload-support.html

b) We may need to add "crossorigin" hint support for preconnect to account for the anonymous cases.

c) As an orthogonal discussion: I think we need to revisit why/whether fonts should be "anonymous".
Flags: needinfo?(igrigorik)
(Assignee)

Comment 4

3 years ago
Ilya, your new test still doesn't work, at least in ff, for the same reason. It uses a CORS without .withCredentials = true, which will again give it an anonymous channel.

This is incredibly frustrating, "anonymous" has essentially become a 4th element of the origin, at least for transport purposes, and its pretty hard to figure out whether or not its being used.

The good news is that I can make your testcase work by adding .withCredentials=true and making it async xhr (withCredentials is not allowed with sync.)

Other good news is that I implemented crossorigin=anonymous for rel=preconnect just in case the spec decides to do that. I'll stash the patch in this bug after cleaning it up a bit, and if you update the spec please ping me and we can get that landed.
(Assignee)

Comment 5

3 years ago
Created attachment 8602266 [details] [diff] [review]
rel=preconnect from html parser
Attachment #8602266 - Flags: review?(bugs)
(Assignee)

Comment 6

3 years ago
Created attachment 8602315 [details] [diff] [review]
0003-link-rel-preconnect-crossorigin-anonymous-r-smaug.patch

this is fyi right now - just in anticipation of crossorigin=anonymous support being added to the rel=preconnect specification

Comment 7

3 years ago
Re, XHR: Doh.. yeah, this is a minefield. Updated the test case once more [1], this time using vanilla img tag -- pretty sure this should work. 

Re, crossorigin: awesome. I'll try to land the spec update next week and I'll ping you once that's done.


[1] https://resourcehints.info/preconnect/preload-support.html
(Assignee)

Comment 8

3 years ago
ok - the patch under review works with the testcase in comment 7
(Assignee)

Comment 10

3 years ago
Created attachment 8602798 [details] [diff] [review]
0003-link-rel-preconnect-crossorigin-anonymous-r-smaug.patch
Attachment #8602315 - Attachment is obsolete: true
Comment on attachment 8602266 [details] [diff] [review]
rel=preconnect from html parser

>-  // For both PrefetchDNS() and Preconnect() aHref can either be the usual
>+  // For PrefetchDNS() aHref can either be the usual
>   // URI format or of the form "//www.hostname.com" without a scheme.
>   void PrefetchDNS(const nsAString &aHref);
>-  void Preconnect(const nsAString &aHref);
...
> 
>+  // For PrefetchDNS() aHref can either be the usual
>+  // URI format or of the form "//www.hostname.com" without a scheme.
>+  void Preconnect(const nsAString &aHref);

Why this move? And the latter comment is wrong. The method is Preconnect, not PrefetchDNS




>   if (aDocument && !GetContainingShadow()) {
>     aDocument->RegisterPendingLinkUpdate(this);
>   }
> 
>-  if (IsInComposedDoc()) {
>+  if ((mFromParser == NOT_FROM_PARSER) && IsInComposedDoc()) {
>     UpdatePreconnect();
>   }

so after we unbind the element and bind it back to tree, we should preconnect again, right?
So, perhaps set mFromParser in UnbindFromTree?



I hope hsivonen could review the parser bits. If not, please ask review from me again.
Attachment #8602266 - Flags: review?(bugs) → review?(hsivonen)
(Assignee)

Comment 12

3 years ago
(In reply to Olli Pettay [:smaug] from comment #11)
> >+  // For PrefetchDNS() aHref can either be the usual
> >+  // URI format or of the form "//www.hostname.com" without a scheme.
> >+  void Preconnect(const nsAString &aHref);
> 
> Why this move? 

so that method can now be called from nsHtml5SpeculativeLoad.cpp

> And the latter comment is wrong.

ack
  
> so after we unbind the element and bind it back to tree, we should
> preconnect again, right?
> So, perhaps set mFromParser in UnbindFromTree?
>

ack

> 
> I hope hsivonen could review the parser bits. If not, please ask review from
> me again.

Thanks!
Comment on attachment 8602266 [details] [diff] [review]
rel=preconnect from html parser

Review of attachment 8602266 [details] [diff] [review]:
-----------------------------------------------------------------

Not marking r+ yet pending answers to questions below. Marking r- duo to the oddity with missing "else" between "}" and "if".

::: dom/html/HTMLLinkElement.cpp
@@ +154,1 @@
>      UpdatePreconnect();

This bit seems to trust that the parser performs precise work instead of an optimization.

::: parser/html/nsHtml5SpeculativeLoad.cpp
@@ +67,5 @@
>                                                 intSource);
>        }
>        break;
> +    case eSpeculativeLoadPreconnect:
> +      aExecutor->Preconnect(mUrl);

The Preconnect() method on nsContentSink does not seem to account for being called again and again for the same <link> element when a speculation has failed and the parser reparses the same <link> again. Is there some mechanism somewhere that makes this OK?

::: parser/html/nsHtml5TreeBuilderCppSupplement.h
@@ +186,5 @@
>                  InitStyle(*url,
>                            (charset) ? *charset : EmptyString(),
>                            (crossOrigin) ? *crossOrigin : NullString());
>              }
> +          } if (rel && rel->LowerCaseEqualsASCII("preconnect")) {

"if" is on the same line with previous "}" without "else". That's weird.

I suggest null-checking "rel" only once earlier (not visible in the patch) and then having 
if (rel->LowerCaseEqualsASCII("stylesheet")) {
  ..
} else if (rel->LowerCaseEqualsASCII("preconnect")) {
  ..
}
inside the null-check "if".

Note that not splitting the value of "rel" on spaces and just checking the overall value is bogus. When preloads are just an optimization, that's OK. However, in this case, it looks like code in the link element doesn't do some updates if the element is from the parser, so in that case what the parser does here may not be just an optimization. Is there a reason why this is OK?
Attachment #8602266 - Flags: review?(hsivonen) → review-
(Assignee)

Comment 14

3 years ago
Henri, thanks for this.. it took me a few days to pop it back onto my stack but I'm looking forward to getting this landed.

(In reply to Henri Sivonen (:hsivonen) from comment #13)
> This bit seems to trust that the parser performs precise work instead of an
> optimization.

Thanks. I learned a bit about how speculative this is. I changed the rules that said "don't execute if from parser" to be "don't execute (again) if from parser and already executed the preconnect", which ought to take care of both the case of the parser calling the preconnect multiple times and the case where the parser failed to execute it at all.

> "if" is on the same line with previous "}" without "else". That's weird.

thanks. fixed that up.
(Assignee)

Comment 15

3 years ago
Created attachment 8607272 [details] [diff] [review]
rel=preconnect from html parser

ended up taking a slightly different tact from comment 14 and stored a
hash of urls in the document, similar to how the preloader for images
works. Filtering out duplicates is advisable.
Attachment #8607272 - Flags: review?(hsivonen)
(Assignee)

Updated

3 years ago
Attachment #8602798 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
Created attachment 8607273 [details] [diff] [review]
link rel=preconnect crossorigin=anonymous
(Assignee)

Updated

3 years ago
Attachment #8602266 - Attachment is obsolete: true
Attachment #8607272 - Flags: review?(hsivonen) → review+

Comment 18

3 years ago
https://hg.mozilla.org/mozilla-central/rev/f0312c7f7e88
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Updated

3 years ago
Blocks: 1174152
You need to log in before you can comment on or make changes to this bug.