1174152 - crossorigin attribute for link rel=preconnect

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Patrick McManus [:mcmanus]]

This adds support for crossOrigin="anonymous" in link rel=preconnect as recently added by the resource hints spec

Comment 1

[Patrick McManus [:mcmanus]]

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

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: link rel=preconnect crossorigin=anonymous

Comment 3

[Boris Zbarsky [:bzbarsky]]

Can you point me to the relevant spec here, please?  Because as far as I can tell your implementation always does the "anonymous" thing for the <link> case unless crossorigin="use-credentials" is explicitly set.

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Not doing reviews right now from comment #3)
> Can you point me to the relevant spec here, please?  Because as far as I can
> tell your implementation always does the "anonymous" thing for the <link>
> case unless crossorigin="use-credentials" is explicitly set.

https://w3c.github.io/resource-hints/

The implementation is meant to be the other way around. An anonymous channel is only created when crossOrigin="anonymous" is set. our regular channels allow credentials.

This came about when doing rel=preconnect without the attribute.. we made normal channels but then test cases involving fonts that anticipated using the preconnected channel failed because fonts use only anonymous channels (which I find kind of weird, but that's orthogonal).

I'll double check the behavior of the patch and report back

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  So yeah, the spec has the expected behavior.  The point is, empty string maps to "anonymous", so you can't use empty string to represent "not set" in nsContentSink::ProcessLinkHeader.  You need to use a void string.  See what StringToCORSMode actually does.

Comment 6

[Patrick McManus [:mcmanus]]

thanks boris. that difference between void and empty made the cases without crossOrigin work differently depending on if it was triggered by JS or the http header (and probly the parser too).

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: link rel=preconnect crossorigin=anonymous

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8621847 [details] [diff] [review]
link rel=preconnect crossorigin=anonymous

I think hsivonen should take a look at this.


But I don't understand IsVoid() / IsEmpty() handling in nsContentSink::Preconnect. Empty string should map to anonymous, but void to CORS_NONE. Element::StringToCORSMode seems to deal with that just fine.
But looks like in some cases empty string is passed to Preconnect() as aCrossOrigin parameter when void string actually should be.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The code in nsContentSink::ProcessLinkHeader should be changed to do the right thing with void vs empty.  And we need tests that catch the problem, if those aren't in the patch already.

Comment 10

[Patrick McManus [:mcmanus]]

Attachment: link rel=preconnect crossorigin=anonymous

Comment 11

[Patrick McManus [:mcmanus]]

(In reply to Not doing reviews right now from comment #9)
> The code in nsContentSink::ProcessLinkHeader should be changed to do the
> right thing with void vs empty.  And we need tests that catch the problem,
> if those aren't in the patch already.

The patch in comment 10 accepts that advice. thanks!

Comment 12

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Comment on attachment 8622527 [details] [diff] [review]
link rel=preconnect crossorigin=anonymous

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

::: dom/base/nsDocument.cpp
@@ +9779,5 @@
> +  // nsISpeculativeConnect does not use the path of the URI, we normalize
> +  // it here so the document cache can be partitioned between crossorigin
> +  // anonymous and other.
> +  if (aCORSMode == CORS_ANONYMOUS) {
> +    uri->SetPath(NS_LITERAL_CSTRING("/anonymous"));

The patch looks OK, except this part looks weird.

Can you explain what the deal is with this magic URL path and what makes OK? I don't see this magic path in the spec.

Comment 13

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Needinfoing :mcmanus for the question in comment 12:
Can you explain what the deal is with this magic URL path and what makes it OK?

Comment 14

[Patrick McManus [:mcmanus]]

(In reply to Henri Sivonen (:hsivonen) from comment #13)
> Needinfoing :mcmanus for the question in comment 12:
> Can you explain what the deal is with this magic URL path and what makes it
> OK?

That URI, at that point, is only used for

1]nsISpeculativeConnect which explicitly does not use the path component (it just handles connections, so only the origin is interesting to it)

2] The per document de-deduplication uri hash. As anon vs non-anon result in 2 different kinds of connections on the wire they shouldn't be de-dup'd into a single entry.

Comment 15

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Comment on attachment 8622527 [details] [diff] [review]
link rel=preconnect crossorigin=anonymous

(In reply to Patrick McManus [:mcmanus] from comment #14)
> (In reply to Henri Sivonen (:hsivonen) from comment #13)
> > Needinfoing :mcmanus for the question in comment 12:
> > Can you explain what the deal is with this magic URL path and what makes it
> > OK?
> 
> That URI, at that point, is only used for
> 
> 1]nsISpeculativeConnect which explicitly does not use the path component (it
> just handles connections, so only the origin is interesting to it)
> 
> 2] The per document de-deduplication uri hash. As anon vs non-anon result in
> 2 different kinds of connections on the wire they shouldn't be de-dup'd into
> a single entry.

OK. It would be nice to have a comment explaining that more explicitly. r+ with such a comment added.

Comment 16

[Patrick McManus [:mcmanus]]

Attachment: link rel=preconnect crossorigin=anonymous

Comment 17

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/feb7cd25aafb

Comment 18

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10887932&repo=mozilla-inbound

Comment 19

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb334e7ebeb

Comment 20

[Patrick McManus [:mcmanus]]

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

Comment 21

[Patrick McManus [:mcmanus]]

Attachment: crossorigin attribute for link rel=preconnect

Comment 22

[Patrick McManus [:mcmanus]]

Attachment: update test to be nsIObserver based

Comment 23

[u408661]

Comment on attachment 8625888 [details] [diff] [review]
update test to be nsIObserver based

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

::: netwerk/test/mochitests/test_rel_preconnect.html
@@ +42,5 @@
> +function doTest()
> +{
> +  srv = new TestServer1();
> +  SpecialPowers.setBoolPref("network.http.debug-observations", true);
> +  

nit: whitespace-only line

Comment 24

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c6156a485a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a587393c81c

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7c6156a485a
https://hg.mozilla.org/mozilla-central/rev/7a587393c81c

Comment 26

[Jean-Yves Perrier [:teoli]]

I have updated: 
https://developer.mozilla.org/en-US/Firefox/Releases/41#HTML

I don't think <link> need an update (crossorigin is already described there, and the user will assume it can be used with any rel value).