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

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

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

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

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 9 years ago

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.

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

(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

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 9 years ago

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.

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

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

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

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.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 9 years ago

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.

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

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

	Henri Sivonen (:hsivonen)
	Comment 12 • 9 years ago

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.

	Henri Sivonen (:hsivonen)
	Comment 13 • 9 years ago

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?

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

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

	Henri Sivonen (:hsivonen)
	Comment 15 • 9 years ago

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.

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

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

	Carsten Book [:Tomcat]
	Comment 18 • 9 years ago

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

	Pulsebot
	Comment 19 • 9 years ago

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

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

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

	u408661
	Comment 23 • 9 years ago

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

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

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

	Carsten Book [:Tomcat]
	Comment 25 • 9 years ago

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

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

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