crossorigin attribute for link rel=preconnect

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

({dev-doc-complete})

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

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Assignee

Description

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

Comment 2

4 years ago
Attachment #8621566 - Flags: review?(bugs)
Assignee

Updated

4 years ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
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.
Assignee

Comment 4

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

Comment 6

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

Updated

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

Comment 7

4 years ago
Attachment #8621847 - Flags: review?(bugs)
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.
Attachment #8621847 - Flags: review?(bugs) → review-
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.
Assignee

Comment 10

4 years ago
Attachment #8622527 - Flags: review?(hsivonen)
Assignee

Updated

4 years ago
Attachment #8621847 - Attachment is obsolete: true
Assignee

Comment 11

4 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!
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.
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?
Flags: needinfo?(mcmanus)
Assignee

Comment 14

4 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.
Flags: needinfo?(mcmanus)
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.
Attachment #8622527 - Flags: review?(hsivonen) → review+
Assignee

Updated

4 years ago
Attachment #8622527 - Attachment is obsolete: true
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10887932&repo=mozilla-inbound
Flags: needinfo?(mcmanus)

Comment 23

4 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
Attachment #8625888 - Flags: review?(hurley) → review+
https://hg.mozilla.org/mozilla-central/rev/a7c6156a485a
https://hg.mozilla.org/mozilla-central/rev/7a587393c81c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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).
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.