1188822 - Make service-workers/service-worker/fetch-request-resources.https.html pass

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Turns out there are some tests for this in the blink wpt tests:

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-resources.https.html#171

But it seems we currently fail the test:

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/meta/service-workers/service-worker/fetch-request-resources.https.html.ini

I'm working on getting these working over in bug 1189023, but starting with a different part of the tests.  If you want to help get the fetch-request-resources.https.html test running that would be great.

Comment 2

[Dimi Lee [:dimi]]

(In reply to Ben Kelly [:bkelly] from comment #1)
> I'm working on getting these working over in bug 1189023, but starting with
> a different part of the tests.  If you want to help get the
> fetch-request-resources.https.html test running that would be great.

I will check if i can fix fetch-request-resources.https.html test fail

Comment 4

[Josh Matthews [:jdm]]

In case it helps, you'll probably want to change the uses of HTTP_ORIGIN and HTTP_REMOTE_ORIGIN to HTTPS_ORIGIN to avoid mixed-content blocking.

Comment 5

[Josh Matthews [:jdm]]

HTTPS_ORIGIN AND HTTPS_REMOTE_ORIGIN, that is.

Comment 6

[Dimi Lee [:dimi]]

(In reply to Josh Matthews [:jdm] from comment #4)
> In case it helps, you'll probably want to change the uses of HTTP_ORIGIN and
> HTTP_REMOTE_ORIGIN to HTTPS_ORIGIN to avoid mixed-content blocking.

It helps, thanks!

Comment 7

[Dimi Lee [:dimi]]

Attachment: Patch describing why the error occurs

Hi Ben,
I upload this patch to describe which part cause error, it did fix current testing but the solution is just temporarily.

I might need help to check if my assumptions for each part is correct and also if it is correct, what is the right way to fix it.

Thanks for help!

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8644293 [details] [diff] [review]
Patch describing why the error occurs

Review of attachment 8644293 [details] [diff] [review]:
-----------------------------------------------------------------

I see the general issue with the LOAD_ANONYMOUS flag, but generally that is handled by nsCORSListenerProxy.

Ultimately, I think we should be checking the new securityFlags on the nsILoadInfo:

  https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#90

Basically, instead of LOAD_ANONYMOUS, we should be looking to see if securityFlags has SEQ_REQUIRE_CORS_DATA_INHERITS and SEC_REQUIRE_CORS_WITH_CREDENTIALS.

Unfortunately securityFlags is not set everywhere yet, so we probably need to check securityFlags and if its SEC_NORMAL (0), then do our current LOAD_ANONYMOUS thing.  Ultimately we will want to stop using LOAD_ANONYMOUS altogether in favor of the securityFlags.

Switching to securityFlags instead of LOAD_ANONYMOUS is similar to bug 1189945, but we probably want a new bug to describe that.

::: image/imgLoader.cpp
@@ +2095,5 @@
> +     *
> +     * When loading <img>, We did not set channel flags to |nsIRequest::LOAD_ANONYMOUS| 
> +     * when aLoadFlags contains imgILoader::LOAD_CORS_ANONYMOUS
> +     */
> +    requestFlags |= nsIRequest::LOAD_ANONYMOUS;

Hmm.  It seems this should be getting set by the nsCORSListenerProxy:

  https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#998

The imgLoader does set the nsCORSListenerProxy as a listener.  So do nsScriptLoader.cpp and Loader.cpp.

Do we just intercept before the nsCORSListenerProxy can set the value?

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-resources.https.html
@@ +214,5 @@
> +         * and then return
> +         *
> +         * So fetch event will not be called
> +         */
> +/*

If these are placed in a separate test case then they could be marked "expected fail".

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-request-resources-iframe.https.html
@@ +39,5 @@
> +   * Test timeout because of fetch event is not triggered, when we use
> +   * url as FontFamily, it will show error in :
> +   * https://dxr.mozilla.org/mozilla-central/source/layout/style/FontFace.cpp?from=FontFace.cpp#208
> +   */  
> +  var fontFace = new FontFace("One", 'url(' + url + ')');

Is this behavior defined by a spec somewhere?

Comment 9

[Dimi Lee [:dimi]]

(In reply to Ben Kelly [:bkelly] from comment #8)
> Comment on attachment 8644293 [details] [diff] [review]
> Patch describing why the error occurs
> 
> Review of attachment 8644293 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/imgLoader.cpp
> @@ +2095,5 @@
> > +     *
> > +     * When loading <img>, We did not set channel flags to |nsIRequest::LOAD_ANONYMOUS| 
> > +     * when aLoadFlags contains imgILoader::LOAD_CORS_ANONYMOUS
> > +     */
> > +    requestFlags |= nsIRequest::LOAD_ANONYMOUS;
> 
> Hmm.  It seems this should be getting set by the nsCORSListenerProxy:
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/security/
> nsCORSListenerProxy.cpp#998
> 
> The imgLoader does set the nsCORSListenerProxy as a listener.  So do
> nsScriptLoader.cpp and Loader.cpp.
> 
> Do we just intercept before the nsCORSListenerProxy can set the value?

For imgLoader we did intercept before creating nsCORSListenerProxy.
For scrip loader and loader.cpp, we call |nsCORSListenerProxy::UpdateChannel|
before intercepting, but we will return at
https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#933

Comment 10

[Dimi Lee [:dimi]]

I am wrong about saying sw intercept imgLoader before creating nsCORSListenerProxy in Comment 9.

I just move 
https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#998
before
https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#933
and all image, script, css tests pass

Hi Ben,
Do you think it is ok to move the code block mentioned above ? or there is a reason we put it in the end of |nsCORSListenerProxy::UpdateChannel|.

O I should solve this by checking securityFlags as you mentioned in Comment 8

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Hmm, this would change how we handle cookies for same origin requests, right?  I don't think we want to make that change.

Jonas, can you confirm that the change suggested in comment 10 is not ok?

The real problem here is that we're setting RequestCredentials based on whether we actually anonymize cookies or not.  Instead RequestCredentials needs to reflect the CORS policy selected (regardless of what would have been done on the wire).

So I recommend writing the code to use securityFlags, although it may not make these tests pass until all the network call sites have been updated to set securityFlags properly.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, I think that would mean, for example, that XHR would never send cookies. Which would break a lot of websites.

Comment 13

[Dimi Lee [:dimi]]

Hi Ben,
For img, script and css, we have 3 different tests with crossOrigin attribute set to:
1.''
2.'anonymous'
3.'use-credentials'

so the expected security flag will be 
1. SEC_NORMAL
2. SEC_REQUIRE_CORS_DATA_INHERITS
3. SEC_REQUIRE_CORS_DATA_INHERITS and SEC_REQUIRE_CORS_WITH_CREDENTIALS

Is that correct ?

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

I need to look closer at this later, but SEC_NORMAL is going to go away.  So we need another value there.  SEC_NORMAL basically means that the securityMode is not set in the securityFlags.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

A normal <img>, with no crossOrigin attribute set, will use SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

It looks like <script> and <style> should also get some variant of SEC_ALLOW_CROSS_ORIGIN_* when .crossOrigin attribute is not set, right?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Correct. They should both get SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS

Comment 18

[Dimi Lee [:dimi]]

Attachment: Patch v1

Hi Ben,
This patch update the security flag for img, script and style, but I did not change SEC_NORMAL to SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS because I am not sure if I should do it in this patch or it belongs to other bugs.

And also about font_face_test, I don't know what is correct expect result, following is current testcase
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-resources.https.html?offset=0#180

From my test, two tests will both create nsCORSListenerProxy hence request mode will both be 'cors', and what should be the expected credentials ?

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8657789 [details] [diff] [review]
Patch v1

Review of attachment 8657789 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good.  I made some notes about changes that are needed, though.

::: dom/base/nsScriptLoader.cpp
@@ +283,5 @@
>    if (mDocument->GetSandboxFlags() & SANDBOXED_SCRIPTS) {
>      return NS_OK;
>    }
>  
> +  nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;

I believe this should default to SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS.

::: dom/workers/ServiceWorkerManager.cpp
@@ +3569,5 @@
>            MOZ_CRASH("Unexpected CORS mode");
>        }
>  
> +      nsSecurityFlags securityFlags = loadInfo->GetSecurityMode();
> +      if (securityFlags & nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) {

Please rebase as this method has changed.  In particular, this code now looks at securityFlags to set the mode:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.cpp?offset=600#268

@@ +3580,3 @@
>            mRequestCredentials = RequestCredentials::Include;
> +        } else {
> +          mRequestCredentials = RequestCredentials::Omit;

You're correct, though, we should also use securityFlags to set RequestCredentials if its not SEC_NORMAL.  We probably need another helper method like I made for the RequestMode (linked above).

::: image/imgLoader.cpp
@@ +1592,5 @@
>  
>      return NS_SUCCEEDED(rv);
>  
>    } else {
> +    nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;

This should SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS.

@@ +2092,5 @@
>      // Propagate background loading...
>      requestFlags |= nsIRequest::LOAD_BACKGROUND;
>    }
>  
> +  nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;

SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS

::: layout/style/Loader.cpp
@@ +1546,1 @@
>    nsLoadFlags securityFlags = nsILoadInfo::SEC_NORMAL;

SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Regarding font face, I believe they should always be loaded via cors with anonymous credentials:

 https://drafts.csswg.org/css-fonts/#font-fetching-requirements

So the RequestCredentials should be set to "omit".

Anne, do you agree?

Dimi, so I think you should change the test to expect the 'cors' RequestMode and 'omit' RequestCredentials.

Comment 21

[Anne (:annevk)]

HTML's crossorigin=anonymous maps to credentials mode "same-origin" as far as I know. I would have liked for it to be "omit", but I don't think it's implemented as such.

Comment 22

[(no longer active)]

Dimi, are you still working on this?

Comment 23

[Dimi Lee [:dimi]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #22)
> Dimi, are you still working on this?

Sorry because of PTO and other issues so this is pending for a while.
I am not working on it right now but I plan to work on this next week.

Please feel free to take it.

Comment 24

[(no longer active)]

Given that bug 1216687 hasn't been fixed yet, there is a good chance that I won't have time to work on this.

Comment 25

[Dimi Lee [:dimi]]

I would like to take this bug

Comment 26

[Dimi Lee [:dimi]]

Attachment: WIP patch

For backup

Comment 27

[Dimi Lee [:dimi]]

Hi Ben,
I have update the patch based on the fix in Bug 1216687, but I still have some questions,
1. If I update |SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS| as default security flag for image, and stylesheet, It will fail at "contentPolicyType not supported yet"[1] in AsyncOpen2
So should i just add cookie policy to security flag for now ?

2. After applying this patch, the test case fetch-request-xhr.https.html will fail when xhr send to a remote origin and |withCredentials| is not set (The expected credentials is 'omit' but will get 'same-origin'). Originally, 'omit' is set when there is |LOAD_ANONYMOUS|, and in this case, |LOAD_ANONYMOUS| is set in |nsContentSecurityManager::CheckChannel|[2].
But after replacing |LOAD_ANONYMOUS| with security flags, the cookie policy in this case is still 'same-origin', do you have any suggestion ?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#150
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#452

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #27)
> 1. If I update |SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS| as default security
> flag for image, and stylesheet, It will fail at "contentPolicyType not
> supported yet"[1] in AsyncOpen2
> So should i just add cookie policy to security flag for now ?

I'm not sure.  Jonas, is it safe to use the new SEC_COOKIES_* flags without AsyncOpen2?  Or is there a better fix for the issue Dimi is running into here?

> 2. After applying this patch, the test case fetch-request-xhr.https.html
> will fail when xhr send to a remote origin and |withCredentials| is not set
> (The expected credentials is 'omit' but will get 'same-origin').

That test is wrong, then.  XHR should be using 'same-origin'.  We had no way to do 'omit' before the fetch() API was added.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Ben Kelly [:bkelly] from comment #28)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #27)
> > 1. If I update |SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS| as default security
> > flag for image, and stylesheet, It will fail at "contentPolicyType not
> > supported yet"[1] in AsyncOpen2
> > So should i just add cookie policy to security flag for now ?
> 
> I'm not sure.  Jonas, is it safe to use the new SEC_COOKIES_* flags without
> AsyncOpen2?

The SEC_COOKIES_* flags only work if you use AsyncOpen2. Otherwise they have no effect.

> Or is there a better fix for the issue Dimi is running into here?

If you are getting to that line, then the call was changed to use AsyncOpen2. Dimi, is it correct that you changed to use AsyncOpen2?

Note though that you should not change to use AsyncOpen2 unless you make sure to pass a correct loadingPrincipal and triggeringPrincipal. However especially for image loads that is a lot of work. It's work that would be great to get help with though, but it's definitely work that should be done in a separate bug focused only on the image code.


All that said, I'm not sure what problem we are trying to solve in this bug. I.e. I don't know why fetch-request-resources.https.html is failing. So I can't really help with advice for how to fix it.

Comment 30

[Dimi Lee [:dimi]]

(In reply to Jonas Sicking (:sicking) from comment #29)
> If you are getting to that line, then the call was changed to use
> AsyncOpen2. Dimi, is it correct that you changed to use AsyncOpen2?

Yes, in the WIP patch I use AyncOpen2 to pass SEC_COOKIES_* flags.

> Note though that you should not change to use AsyncOpen2 unless you make
> sure to pass a correct loadingPrincipal and triggeringPrincipal. However
> especially for image loads that is a lot of work. It's work that would be
> great to get help with though, but it's definitely work that should be done
> in a separate bug focused only on the image code.

Hi Ben,
For now only ScriptLoader.cpp use AsyncOpen2, stylesheet, font, and image are all using AsyncOpen.
According to jonas's comment, it could be lots effort to change to AsyncOpen2.

I think we could use security flags to map to request crendential first and enable only script test.
As for stylesheet, font, and image, these could be enabled after AsynOpen2 is supported accordingly.
How do you think ?

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #30)
> I think we could use security flags to map to request crendential first and
> enable only script test.
> As for stylesheet, font, and image, these could be enabled after AsynOpen2
> is supported accordingly.
> How do you think ?

I think that works.  What credentials would we supply for stylesheet, font, and image requests?  Are they more restrictive than the correct values?

I just want to make sure we don't create a security issue.

Comment 32

[Dimi Lee [:dimi]]

(In reply to Ben Kelly [:bkelly] from comment #31)
> I think that works.  What credentials would we supply for stylesheet, font,
> and image requests?  Are they more restrictive than the correct values?
> 
> I just want to make sure we don't create a security issue.

[1]Current implementation:
"no-cors" request         , correct value will be 'include' , got 'same origin'
"anonymouse" request      , correct value will be 'omit'    , got 'same origin'
"use-credentials" request , correct value will be 'include' , got 'include'

[2]Remove current implementation and map security flag to crendential mode
"no-cors" request         , correct value will be 'include' , got 'include'
"anonymouse" request      , correct value will be 'omit'    , got 'include'
"use-credentials" request , correct value will be 'include' , got 'include'

[3]Check SEC_NORMAL flag first, if exists, apply original implemetation,otherwise
map security flag to crendential mode.

In this approach, stylesheet, font and image will get original result[1], for script, it
will get correct crendential mode.

Do you think we can use approach 3 ? because all credential mode become 'include' may have security issue.

Comment 33

[Anne (:annevk)]

For crossorigin=anonymous the expected credentials mode is "same-origin" due to legacy. ("no-cors" should be "include" though.)

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

I don't think we can do (2), so I guess (3) is the best bet.  Also, double check the expectations against Anne's info in comment 33.  Thanks!  Sorry for the delay in responding.

Comment 35

[Dimi Lee [:dimi]]

(In reply to Anne (:annevk) from comment #33)
> For crossorigin=anonymous the expected credentials mode is "same-origin" due
> to legacy. ("no-cors" should be "include" though.)

Thanks for the information!

Comment 36

[Dimi Lee [:dimi]]

Attachment: Fix fetch-request-resources.https.html v1

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8710326 [details] [diff] [review]
Fix fetch-request-resources.https.html v1

Review of attachment 8710326 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good improvement.  I think we may end up still failing the tests until AsyncOpen2() is complete, but this is still worth doing.  r=me with comments addressed.

::: dom/base/nsScriptLoader.cpp
@@ +297,5 @@
>      aRequest->mCORSMode == CORS_NONE
>      ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL
>      : nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> +  if (aRequest->mCORSMode == CORS_ANONYMOUS) {
> +    securityFlags |= nsILoadInfo::SEC_COOKIES_OMIT;

This should be SEC_COOKIES_SAME_ORIGIN, no?

::: dom/fetch/InternalRequest.cpp
@@ +342,5 @@
> +    uint32_t loadFlags;
> +    aChannel->GetLoadFlags(&loadFlags);
> +
> +    if (loadFlags & nsIRequest::LOAD_ANONYMOUS) {
> +      return RequestCredentials::Same_origin;

I think in this case its safer to just use "omit" here.  Once we set LOAD_ANONYMOUS we should ensure its never removed from the request.

We could accidentally map a cross-origin request with same-origin credentials to omit here, but thats ok because omit and same-origin are the same effect for cross-origin urls; don't send credentials.

@@ +360,5 @@
> +
> +  if (cookiePolicy == nsILoadInfo::SEC_COOKIES_INCLUDE) {
> +    return RequestCredentials::Include;
> +  } else if (cookiePolicy == nsILoadInfo::SEC_COOKIES_OMIT) {
> +    return RequestCredentials::Same_origin;

This should be RequestCredentials::Omit.

@@ +367,5 @@
> +  } else {
> +    MOZ_ASSERT_UNREACHABLE("Unexpected cookie policy!");
> +  }
> +
> +  return RequestCredentials::Same_origin;

Just put the MOZ_ASSERT_UNREACHABLE() here instead of returning.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-css-images.https.html
@@ +86,5 @@
> +        css_image_test(f, LOCAL_URL, 'backgroundImage', 'no-cors', 'include');
> +        css_image_test(f, REMOTE_URL, 'backgroundImage', 'no-cors', 'include');
> +
> +        css_image_test(f, LOCAL_URL, 'shapeOutside', 'cors', 'omit');
> +        css_image_test(f, REMOTE_URL, 'shapeOutside', 'cors', 'omit');

These should probably be 'same-origin'.  Only fetch() can produce an 'omit' legitimately.  All pre-existing APIs should result in either 'include' or 'same-origin'.

@@ +89,5 @@
> +        css_image_test(f, LOCAL_URL, 'shapeOutside', 'cors', 'omit');
> +        css_image_test(f, REMOTE_URL, 'shapeOutside', 'cors', 'omit');
> +
> +        css_image_set_test(f, LOCAL_URL, 'backgroundImage', 'no-cors', 'same-origin');
> +        css_image_set_test(f, REMOTE_URL, 'backgroundImage', 'no-cors', 'same-origin');

Similarly, these should probably be 'include' for no-cors requests.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-request-resources-iframe.https.html
@@ +30,5 @@
>    document.body.appendChild(link);
>  }
>  
>  function load_font(url) {
> +  var fontFace = new FontFace('test', 'url(' + url + ')');

Why this change?

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-request-xhr-iframe.https.html
@@ +136,5 @@
>          return xhr_send(host_info['HTTPS_REMOTE_ORIGIN'], 'GET', '', false);
>        })
>      .then(function(response){
>          assert_equals(response.mode, 'cors');
> +        assert_equals(response.credentials, 'same-origin');

This should indeed be 'same-origin', but until we convert everything to AsyncOpen2() we may need to leave this returning 'omit'.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #37)
> :::
> testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> fetch-request-xhr-iframe.https.html
> @@ +136,5 @@
> >          return xhr_send(host_info['HTTPS_REMOTE_ORIGIN'], 'GET', '', false);
> >        })
> >      .then(function(response){
> >          assert_equals(response.mode, 'cors');
> > +        assert_equals(response.credentials, 'same-origin');
> 
> This should indeed be 'same-origin', but until we convert everything to
> AsyncOpen2() we may need to leave this returning 'omit'.

To clarify, I think we should change the test to 'same-origin' to be correct, but we will need to keep failing the test until AsyncOpen2() is done.

Comment 39

[Dimi Lee [:dimi]]

Thanks for your review!
ni because I have a question about xhr.

(In reply to Ben Kelly [:bkelly] from comment #37)
> ::: dom/base/nsScriptLoader.cpp
> @@ +297,5 @@
> >      aRequest->mCORSMode == CORS_NONE
> >      ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL
> >      : nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> > +  if (aRequest->mCORSMode == CORS_ANONYMOUS) {
> > +    securityFlags |= nsILoadInfo::SEC_COOKIES_OMIT;
> 
> This should be SEC_COOKIES_SAME_ORIGIN, no?
>

Ah...I misunderstand Anne's info in comment 33. I thought CORS_ANONYMOUS maps to SEC_COOKIES_OMIT, and SEC_COOKIES_OMIT maps RequestCredentials::Same_origin.
I will fix this in next patch

> testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> fetch-request-resources-iframe.https.html
> @@ +30,5 @@
> >    document.body.appendChild(link);
> >  }
> >  
> >  function load_font(url) {
> > +  var fontFace = new FontFace('test', 'url(' + url + ')');
> 
> Why this change?
> 

Pass first argument with url cause error return at[1]
So fetch event will not be triggered.
I just grep testcases that use FontFace and found many of them use 'test'
I guess maybe pass url make parsing error somewhere.

> :::
> testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> fetch-request-xhr-iframe.https.html
> @@ +136,5 @@
> >          return xhr_send(host_info['HTTPS_REMOTE_ORIGIN'], 'GET', '', false);
> >        })
> >      .then(function(response){
> >          assert_equals(response.mode, 'cors');
> > +        assert_equals(response.credentials, 'same-origin');
> 
> This should indeed be 'same-origin', but until we convert everything to
> AsyncOpen2() we may need to leave this returning 'omit'.

Sorry, maybe a dumb question here, xhr request already use AsyncOpen2, why do we need everything change to use AsyncOpen2 ?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/FontFace.cpp?from=FontFace.cpp#208
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#2909

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #39)
> > ::: dom/base/nsScriptLoader.cpp
> > @@ +297,5 @@
> > >      aRequest->mCORSMode == CORS_NONE
> > >      ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL
> > >      : nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> > > +  if (aRequest->mCORSMode == CORS_ANONYMOUS) {
> > > +    securityFlags |= nsILoadInfo::SEC_COOKIES_OMIT;
> > 
> > This should be SEC_COOKIES_SAME_ORIGIN, no?
> >
> 
> Ah...I misunderstand Anne's info in comment 33. I thought CORS_ANONYMOUS
> maps to SEC_COOKIES_OMIT, and SEC_COOKIES_OMIT maps
> RequestCredentials::Same_origin.
> I will fix this in next patch

Yea, SEC_COOKIES_* and RequestCredentials::* values should map one-to-one I believe:

  SEC_COOKIES_INCLUDE = RequestCredentials::Include
  SEC_COOKIES_SAME_ORIGIN = RequestCredentials::SameOrigin
  SEC_COOKIES_OMIT = RequestCredentials::Omit

The spec concept of "cors anonymous" maps to RequestCredentials::SameOrigin (and SEC_COOKIES_SAME_ORIGIN).  All other legacy APIs use RequestCredentials::Include (and SEC_COOKIES_INCLUDE).

The fetch() API is the one API that can now do a request with RequestCredentials::Omit.  This is only possible because fetch uses the AsyncOpen2() and SEC_COOKIES_OMIT.

Thats the desired long term situation with AsyncOpen2().  For now obviously we have to infer RequestCredentials values from LOAD_ANONYMOUS.  This is ambiguous since both same-origin and omit can result in LOAD_ANONYMOUS being set.

> > :::
> > testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> > fetch-request-xhr-iframe.https.html
> > @@ +136,5 @@
> > >          return xhr_send(host_info['HTTPS_REMOTE_ORIGIN'], 'GET', '', false);
> > >        })
> > >      .then(function(response){
> > >          assert_equals(response.mode, 'cors');
> > > +        assert_equals(response.credentials, 'same-origin');
> > 
> > This should indeed be 'same-origin', but until we convert everything to
> > AsyncOpen2() we may need to leave this returning 'omit'.
> 
> Sorry, maybe a dumb question here, xhr request already use AsyncOpen2, why
> do we need everything change to use AsyncOpen2 ?

Oh, ok.  For some reason I thought this was still using the old path.  If its using AsyncOpen2(), then yes it should pass.

Thanks again!

Comment 41

[Dimi Lee [:dimi]]

(In reply to Ben Kelly [:bkelly] from comment #40)
> 
> Yea, SEC_COOKIES_* and RequestCredentials::* values should map one-to-one I
> believe:
> 
>   SEC_COOKIES_INCLUDE = RequestCredentials::Include
>   SEC_COOKIES_SAME_ORIGIN = RequestCredentials::SameOrigin
>   SEC_COOKIES_OMIT = RequestCredentials::Omit
> 
> The spec concept of "cors anonymous" maps to RequestCredentials::SameOrigin
> (and SEC_COOKIES_SAME_ORIGIN).  All other legacy APIs use
> RequestCredentials::Include (and SEC_COOKIES_INCLUDE).
> 
> The fetch() API is the one API that can now do a request with
> RequestCredentials::Omit.  This is only possible because fetch uses the
> AsyncOpen2() and SEC_COOKIES_OMIT.
> 
> Thats the desired long term situation with AsyncOpen2().  For now obviously
> we have to infer RequestCredentials values from LOAD_ANONYMOUS.  This is
> ambiguous since both same-origin and omit can result in LOAD_ANONYMOUS being
> set.

Thanks for the clear information :D

Comment 42

[Dimi Lee [:dimi]]

Attachment: Fix fetch-request-resources.https.html v2

Comment 43

[Dimi Lee [:dimi]]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=095218cb6ddf&selectedJob=15878081

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74ab992b6952

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74ab992b6952

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74ab992b6952

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8711598 [details] [diff] [review]
Fix fetch-request-resources.https.html v2

Approval Request Comment
[Feature/regressing bug #]: Service workers.
[User impact if declined]: A large website has reported their users are seeing failures due cookies not be sent via service worker pass through requests.
[Describe test coverage new/current, TreeHerder]: WPT tests included.  I tested locally on a beta 45 and verified it fixes the issue.
[Risks and why]: Minimal.  Should only effect service workers.
[String/UUID change made/needed]: None.

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Attachment: beta uplift patch

I had to rebase slightly for beta branch.

Comment 49

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8711598 [details] [diff] [review]
Fix fetch-request-resources.https.html v2

Improve the feature, have plenty of tests, taking it.
Should be in 45 beta 7

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b6f032d96fd6

Comment 51

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd6f38ef9fd3

Comment 52

[Boris Zbarsky [:bzbarsky]]

service-workers/service-worker/fetch-request-css-images.https.html didn't get added to the test manifest, so afaict it's not actually running in automation.  Is that intended?

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

I recommend filing a separate bug to update the manifest for this test.  This bug was uplifted, but we probably don't need to uplift the manifest change.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Note that as khuey discovered when he accidentally added the test to the manifest today, the test totally doesn't pass.

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

I repeat, this should be fixed in a new bug.  I opened bug 1251792.

The failure of the test is not a crisis.  It was broken out from a larger test file with known expected failures.  It probably just needs an appropriate .ini file.