Closed
Bug 1144249
Opened 10 years ago
Closed 10 years ago
Fix no-cors mode in FetchDriver
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: nsm, Assigned: nsm)
References
Details
Attachments
(1 file, 1 obsolete file)
There was a bug where we'd treat a request even with mode no-cors as a cors request.
Assignee | ||
Comment 1•10 years ago
|
||
/r/5543 - Bug 1144249 - fix fetch no-cors mode
Pull down this commit:
hg pull review -r a07b9fdd1b7ee25bc06a5dac9a7df20ac81846fc
Attachment #8578788 -
Flags: review?(bkelly)
Assignee | ||
Updated•10 years ago
|
Comment 2•10 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•10 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+
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
Comment 6•10 years ago
|
||
Assignee: nobody → nsm.nikhil
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8578788 -
Attachment is obsolete: true
Attachment #8619784 -
Flags: review+
Assignee | ||
Comment 8•9 years ago
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•