1144249 - Fix no-cors mode in FetchDriver

Status: RESOLVED FIXED

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

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

There was a bug where we'd treat a request even with mode no-cors as a cors request.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: bz://1144249/nsm

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

Pull down this commit:

hg pull review -r a07b9fdd1b7ee25bc06a5dac9a7df20ac81846fc

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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

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

Ship It!

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca19388684ac

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1144249 - fix fetch no-cors mode