Fix no-cors mode in FetchDriver

RESOLVED FIXED in Firefox 39

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

unspecified
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

There was a bug where we'd treat a request even with mode no-cors as a cors request.
Created attachment 8578788 [details]
MozReview Request: bz://1144249/nsm

/r/5543 - Bug 1144249 - fix fetch no-cors mode

Pull down this commit:

hg pull review -r a07b9fdd1b7ee25bc06a5dac9a7df20ac81846fc
Attachment #8578788 - Flags: review?(bkelly)

Comment 2

3 years ago
Comment on attachment 8578788 [details]
MozReview Request: bz://1144249/nsm

https://reviewboard.mozilla.org/r/5541/#review4543

::: dom/fetch/FetchDriver.cpp
(Diff revision 1)
> +  // Unless the cors mode is explicitly no-cors, we set up a cors proxy even in

Do we really need the proxy in the same-origin case?  We get some benefit from not using it because then channel retargeting to STS works.

::: dom/tests/mochitest/fetch/test_fetch_cors.js
(Diff revision 1)
> -                    { server: "http://test2.mochi.test:8000",
> +                    { server: "http://test2.mochi.test:8888",

What was the impact of having a different port here?

::: dom/tests/mochitest/fetch/test_fetch_cors.js
(Diff revision 1)
> +    ok(false, "no-cors Request fetch should not error");

This is the reason our tests didn't catch it before?
Attachment #8578788 - Flags: review?(bkelly)

Comment 3

3 years ago
Comment on attachment 8578788 [details]
MozReview Request: bz://1144249/nsm

https://reviewboard.mozilla.org/r/5541/#review4545

Ship It!
Attachment #8578788 - Flags: review+
(In reply to Ben Kelly [:bkelly] from comment #2)
> Comment on attachment 8578788 [details]
> MozReview Request: bz://1144249/nsm
> 
> https://reviewboard.mozilla.org/r/5541/#review4543
> 
> ::: dom/fetch/FetchDriver.cpp
> (Diff revision 1)
> > +  // Unless the cors mode is explicitly no-cors, we set up a cors proxy even in
> 
> Do we really need the proxy in the same-origin case?  We get some benefit
> from not using it because then channel retargeting to STS works.

Unfortunately we do. A same-origin url could follow redirects and end up cross-origin. We can't swap the default listener with a cors proxy in the middle of such a transaction (I don't know if it is supported, but even if it was, we'd need to be sure things were done right!), so we have to have it from the beginning.

> 
> ::: dom/tests/mochitest/fetch/test_fetch_cors.js
> (Diff revision 1)
> > -                    { server: "http://test2.mochi.test:8000",
> > +                    { server: "http://test2.mochi.test:8888",
> 
> What was the impact of having a different port here?

There was no impact before, since most of these tests fail before they try to contact this host. But our 'mochitests fake domains' don't actually have a server running on port 8000, so if gecko did try to connect it would fail.

> 
> ::: dom/tests/mochitest/fetch/test_fetch_cors.js
> (Diff revision 1)
> > +    ok(false, "no-cors Request fetch should not error");
> 
> This is the reason our tests didn't catch it before?

No, it wasn't caught before because I shouldn't have passed the allowOrigin attribute to the sjs. that attribute sends the proper response headers to make it a cors request, and since we always had a cors proxy before, it passed.
https://hg.mozilla.org/mozilla-central/rev/ca19388684ac
Assignee: nobody → nsm.nikhil
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8578788 [details]
MozReview Request: bz://1144249/nsm
Attachment #8578788 - Attachment is obsolete: true
Attachment #8619784 - Flags: review+
Created attachment 8619784 [details]
MozReview Request: Bug 1144249 - fix fetch no-cors mode
You need to log in before you can comment on or make changes to this bug.