1243792 - add fetch Response.redirected and additional security restrictions

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

See:

  https://github.com/whatwg/fetch/commit/e54f6bd1e75f46cd4b8202f5ee3bfa68e9ded906

Comment 1

[Dimi Lee [:dimi]]

I would like to work on this bug

Comment 2

[Dimi Lee [:dimi]]

Attachment: WIP Patch

Comment 3

[Dimi Lee [:dimi]]

Remove myself from assignee because not actively work on this.

Hi Hsinyi,
Please let me know if someone in your team want to work on this bug, I might be able to share some previous work.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

thanks Dimi. I will leave the flag as it is for now, but will talk to the team and find someone to work on it.

Comment 5

[Tom Tung [:tt, :ttung]]

I would like to try on this bug.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks Tom!

Comment 7

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d99cb50ecb32

Comment 8

[Tom Tung [:tt, :ttung]]

Sorry for the disturbance, I sent wrong patch to try server.

Comment 9

[Tom Tung [:tt, :ttung]]

Hi Ben,

I cannot pass the test [1] (it should pass since the redirect mode is follow and it is redirected) after I implemented the step 3-3 ("request's redirect mode is not "follow" and response's url list has more than one item.") in [2]. 

The reason is that "ServiceWorkerEvents.cpp" can only get the request from the "event" which its RedirectMode is set to be "Manual" [3].  

At the same time, if we create another request via url like [4], we can get another request with different RedirectMode(set to be "Follow" (url setting)) and it is handle by "FetchDriver.cpp". 

Thus, the test[1] will be treated as a "not-follow" request (under scope of ServiceWorkerEvents.cpp) and "redirected" response (since the url is redirected). Therefore, it return a network error (testfail).

Could you help me to check the problem is existed or not? Should I file a bug for it?

Thanks,
Tom

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-redirect.https.html#149-154
[2] https://fetch.spec.whatwg.org/#http-fetch
[3] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10820
[4] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-rewrite-worker.js#82-84

Comment 10

[Tom Tung [:tt, :ttung]]

There are two further explanations. I addressed them inline below:
  1. I implement the [1] at function |RespondWithHandler::ResolvedCallback| in "ServiceWorkerEvents.cpp" since similar conditions are also implemented in there [2], too.
  2. In this case, if I get the value "RequestRedirect" from request in "ServiceWorkerEvents.cpp", the value is 2(manual but should be 0, which is follow).

[1] https://fetch.spec.whatwg.org/#http-fetch
[2] https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#571-603

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Yea, this test needs to be updated.  It should now fail.

Please be aware that you also will need to modify Cache API to store/retrieve the redirect state of a Response.  Just FYI in case you hadn't gotten there yet.

Comment 12

[Tom Tung [:tt, :ttung]]

Thanks! I will change the test and the information is useful. I will add it in my patch.

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1243792 - P1 implement response.redirected - remove url and add urllist - dom/workers/, dom/fetch/ & dom/cache/ changed

I split the patch into two parts and this patch is the first one. 
In this patch, I address what I've done below inline:
- implement response.redirected
  a. add urllist and functionalities for urllist in both "InternalResponse" and "InternalRequest". Besides, modify corresponding functions in "Response" and "Request"
  b. remove mUrl in "InternalResponse" since url is redundant
  c. modify functions in "FetchDriver", "ServiceWrokerEvents"
  d. modify cache API(remove url and add urllist, too)
  e. modify testcase in "fetch-request-redirect.html"

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1243792 - P2 tests in wpt/mozilla

This patch implement a simple test for response.redirected after caching response.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8737156 [details] [diff] [review]
Bug 1243792 - P1  implement response.redirected - remove url and add urllist - dom/workers/, dom/fetch/ & dom/cache/ changed

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

::: dom/cache/DBSchema.cpp
@@ +145,5 @@
> +const char* const kTableResponseUrlList =
> +  "CREATE TABLE response_urllist ("
> +    "url TEXT NOT NULL, "
> +    "entry_id INTEGER NOT NULL REFERENCES entries(id) ON DELETE CASCADE"
> +  ")";

I don't think we need to store the complete URL list.  AFAIK the only observable thing to script is if there was more than one URL or not.  So we just need to store a boolean indicating "redirected" or not.

I think the risk of needing the URLs in the future is low given this note in the spec:

  "Except for the last fetch URL, if any, a response's url list cannot be exposed to script. That would violate atomic HTTP redirect handling. "

I would prefer to go with the boolean approach because URLs can be quite large.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8737156 [details] [diff] [review]
Bug 1243792 - P1  implement response.redirected - remove url and add urllist - dom/workers/, dom/fetch/ & dom/cache/ changed

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

::: dom/cache/DBSchema.cpp
@@ +145,5 @@
> +const char* const kTableResponseUrlList =
> +  "CREATE TABLE response_urllist ("
> +    "url TEXT NOT NULL, "
> +    "entry_id INTEGER NOT NULL REFERENCES entries(id) ON DELETE CASCADE"
> +  ")";

Actually, it seems we might need the URL list in order to implement expected future CSP features.  We may want to keep this.

I guess ignore my previous comment and keep the full URL list.  Sorry for the noise here.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8737156 [details] [diff] [review]
Bug 1243792 - P1  implement response.redirected - remove url and add urllist - dom/workers/, dom/fetch/ & dom/cache/ changed

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

Also please note that this needs a schema migration.  Since you are removing a column you probably need to do something like the 16-to-17 migration:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/DBSchema.cpp#2556

Sorry if that was already on your TODO list.

Comment 18

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #17)
Thanks for reminding me! I'll check out that before sending for review. There are still a few things that needed to be done. Sorry for disturbance.

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Ben Kelly [:bkelly] from comment #16)
> Actually, it seems we might need the URL list in order to implement expected
> future CSP features.  We may want to keep this.

Are you referring to https://w3c.github.io/webappsec-csp/2/#source-list-paths-and-redirects or something like it?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Sutherland [:asuth] from comment #19)
> Are you referring to
> https://w3c.github.io/webappsec-csp/2/#source-list-paths-and-redirects or
> something like it?

Thanks for the link!  I couldn't find it, but Anne theorized it was there somewhere.

Yea, it seems the CSP code would need to inspect the origins for all redirect URLs.

Comment 21

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1243791 - P1 implement response.redirected - Cache API & Fetch API & ServiceWorkerEvents changed

Hi Ben, 

Could you help me to review this patch? 

This patch is mainly addressed modifications of adding "redirected" feature in "response" and corresponding changes(replace url by urllist). I changed implementations in cache api comparing to last patch. I addressed implementations below inline.
The modification in |dom/cache/|:
1. In |struct CacheResponse|, the url is replaced by urllist.
2. Replace the functions using url by using urllist.
3. Since the old versions may still use |response_url| in table |entries|, I keep |response_url| in table |entries|.
4. Add a table |response_urllist| and migrate |response_url| to table |response_urllist| when running |MigrateFrom16to17|. (Should I adding another function for migrating |response_url| to table |response_urllist|?)

The modifications in "dom/fetch, dom/workers" are the same as last version:
1. Replace url by urllist
2. Add an attribute |redirected| in |Response|

Comment 22

[Tom Tung [:tt, :ttung]]

Comment on attachment 8737158 [details] [diff] [review]
Bug 1243792 - P2 tests in wpt/mozilla

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

Could you help me to review this one, too? 

This patch is addressed the test for |response.redirected| after caching response.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8738854 [details] [diff] [review]
Bug 1243791 - P1 implement response.redirected - Cache API & Fetch API & ServiceWorkerEvents changed

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

This is a good start, but I'd like to see it again.  Thanks!

Note, I will be out for a spec meeting Monday and Tuesday.  I may have some time to review on Wednesday next week, but my first full day back is Thursday.  I'm sorry for the delay in getting this review back to you!

::: dom/cache/CacheTypes.ipdlh
@@ +78,5 @@
>  
>  struct CacheResponse
>  {
>    ResponseType type;
> +  nsCString[] urllist;

nit: urlList

::: dom/cache/DBSchema.cpp
@@ +143,5 @@
>    "CREATE INDEX response_headers_name_index "
>              "ON response_headers (name)";
>  
> +const char* const kTableResponseUrlList =
> +  "CREATE TABLE response_urllist ("

nit: response_url_list

@@ +1768,5 @@
> +  nsAutoCString url;
> +  if (!responseUrlList.IsEmpty()) {
> +    url = responseUrlList.LastElement();
> +  }
> +  rv = state->BindUTF8StringByName(NS_LITERAL_CSTRING("response_url"), url);

I don't think we need to store the response_url column in the entries table any more.  Can you remove it?

@@ +1893,5 @@
> +    ") VALUES (:url, :entry_id)"
> +  ), getter_AddRefs(state));
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +  //const nsTArray<nsCString>& responseUrlList = aResponse.urllist();

Commented out code?  I think if you remove the response_url above, though, you will need to uncomment this.

@@ +2026,5 @@
> +  rv = state->BindInt32ByName(NS_LITERAL_CSTRING("entry_id"), aEntryId);
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +  while (NS_SUCCEEDED(state->ExecuteStep(&hasMoreData)) && hasMoreData) {
> +    nsAutoCString url;

Please use nsCString here.  Since its immediately passed to nsTArray<nsCString>::AppendElement() using an nsAutoCString() causes the URL to be copied twice.  Just using nsCString avoids the second copy.

@@ +2652,5 @@
> +      "url TEXT NOT NULL, "
> +      "entry_id INTEGER NOT NULL REFERENCES entries(id) ON DELETE CASCADE"
> +    ")"
> +  ));
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }

Unfortunately you cannot just piggy-back on the existing MigrateFrom16to17() method.  Instead you need to:

1) Increment the kLatestSchemaVersion (right now that means 21).
2) Create a new MigrateFrom20to21() method and hook it into sMigrationList.
3) Write the migration logic in your new method.

In this case, since you are removing response_url you need to do the style of migrate shown in 16to17.  Basically you create the new schema in a temporary table, copy the data across, remove the old table, and then rename your new table.

Of course, you will also need to copy over the response_url from the old table and select it into your new response_urllist table before deleting the old table.

Finally, please update the tests here to check response.url and response.redirected:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/test/xpcshell/test_migration.js#25

::: dom/cache/TypeUtils.cpp
@@ +203,5 @@
>  {
>    aOut.type() = aIn.Type();
>  
> +  aIn.GetUnfilteredUrlList(aOut.urllist());
> +  nsTArray<nsCString> urllist;

AutoTArray<nsCString, 4>

::: dom/fetch/FetchDriver.cpp
@@ +378,5 @@
>                                           bool aFoundOpaqueRedirect)
>  {
>    MOZ_ASSERT(aResponse);
> +
> +  nsTArray<nsCString> reqURLList;

AutoTArray<nsCString, 4>

@@ +380,5 @@
>    MOZ_ASSERT(aResponse);
> +
> +  nsTArray<nsCString> reqURLList;
> +  mRequest->GetURLList(reqURLList);
> +  DebugOnly<nsresult> rv = aResponse->StripFragmentAndSetUrlList(reqURLList);

While you are here, convert this to:

  MOZ_ALWAYS_SUCCEEDS(aResponse->StripFragmentAndSetUrlList(reqUrLList));

@@ +493,2 @@
>    nsCOMPtr<nsIURI> newURI;
>    rv = NS_GetFinalChannelURI(channel, getter_AddRefs(newURI));

Is this necessary?  The URL should already be in the list from AsyncOnChannelRedirect().  It seems we should never call SetURL(), but only append URLs.

@@ +684,5 @@
>    }
>  
> +  // "HTTP-redirect fetch": step 14 "Append locationURL to request's url list."
> +  nsCOMPtr<nsIURI> uri;
> +  aNewChannel->GetURI(getter_AddRefs(uri));

You need to check the return value here.  Or use MOZ_ALWAYS_SUCCEEDS() assertion.

@@ +686,5 @@
> +  // "HTTP-redirect fetch": step 14 "Append locationURL to request's url list."
> +  nsCOMPtr<nsIURI> uri;
> +  aNewChannel->GetURI(getter_AddRefs(uri));
> +
> +  nsAutoCString url;

nsCString since its ultimately going to get added to an nsTArray<nsCString>.  This avoids double copying the url.

::: dom/fetch/InternalRequest.cpp
@@ +24,5 @@
>  InternalRequest::GetRequestConstructorCopy(nsIGlobalObject* aGlobal, ErrorResult& aRv) const
>  {
>    RefPtr<InternalRequest> copy = new InternalRequest();
>    copy->mURL.Assign(mURL);
> +  copy->mURLList.Assign(mURLList);

Actually, the spec only says to copy the original URL or first entry in the list.  So:

  if (r1.redirected) {
    var r2 = new Request(r1);
    assert(r2.redirected === false);
  }

::: dom/fetch/InternalRequest.h
@@ +182,2 @@
>    void
>    GetURL(nsCString& aURL) const

While you are here, can you please make this take an nsACString?

@@ +184,5 @@
>    {
> +    if (mURLList.IsEmpty()) {
> +      aURL.Assign(EmptyCString());
> +    } else {
> +      aURL.Assign(mURLList.LastElement());

nit:  Can you use the short-circuit style here and elsewhere in the patch?

  if (mURLList.IsEmpty()) {
    aURL.Truncate();
    return;
  }

  aURL = mURLList.LastElement();

@@ +190,5 @@
> +  }
> +
> +  // SetURL should set the url in url list.
> +  void
> +  SetURL(const nsACString& aURL)

I think we should remove SetURL().  Is there a particular use case that requires us to keep it?  It seems we should only ever be appending to the list.

@@ +481,1 @@
>    nsCString mURL;

It seems we should not need this any more.  Can we remove it in favor of using the first entry in the mURLList?

I opened a spec bug about this as well:

  https://github.com/whatwg/fetch/issues/275

::: dom/fetch/InternalResponse.cpp
@@ +92,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  if (aUrlList.IsEmpty()) {
> +    return NS_ERROR_HTMLPARSER_BADURL;

Can this actually happen?  I think we should just MOZ_ASSERT(!aUrlList.IsEmpty()) instead.

@@ +118,5 @@
> +    if(NS_WARN_IF(NS_FAILED(rv))){
> +      return rv;
> +    }
> +
> +    AddUrl(spec);

Can you accumulate the nsIURI references in an AutoTArray, and then call AddUrl on all of them after we are sure no errors are thrown?  That way we avoid any partial state on the object.

::: dom/fetch/InternalResponse.h
@@ +74,5 @@
>    {
>      return Type() == ResponseType::Error;
>    }
>  
>    // FIXME(nsm): Return with exclude fragment.

Please remove this comment while you are here.  Its no longer valid since we strip the fragment elsewhere.

@@ +120,5 @@
>    {
> +    if (mURLList.IsEmpty()) {
> +      mURLList.AppendElement(aURL);
> +    } else {
> +      mURLList.ReplaceElementAt(mURLList.Length()-1, aURL);

Again, I think we should get rid of SetURL() if we can.  What case requires it?

::: dom/fetch/Request.h
@@ +45,5 @@
>  
>    void
>    GetUrl(nsAString& aUrl) const
>    {
> +    nsCString url;

nsAutoCString here since its only used locally.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +181,5 @@
>  InterceptedUsedResponseWithURL=Failed to load '%S'. A ServiceWorker passed a used Response to FetchEvent.respondWith(). The body of a Response may only be read once. Use Response.clone() to access the body multiple times.
>  # LOCALIZATION NOTE: Do not translate "ServiceWorker", "opaqueredirect", "Response", "FetchEvent.respondWith()", or "FetchEvent". %s is a URL.
>  BadOpaqueRedirectInterceptionWithURL=Failed to load '%S'. A ServiceWorker passed an opaqueredirect Response to FetchEvent.respondWith() while handling a non-navigation FetchEvent.
> +# LOCALIZATION NOTE: Do not translate "ServiceWorker", "Response", "FetchEvent.respondWith()", "RedirectMode" or "follow". %S is a URL.
> +BadRedirectModeInterceptionWithURL=Failed to load '%S'. A ServiceWorker passed a Response with url list has more than one item to FetchEvent.respondWith() while RedirectMode is not 'follow'.

Maybe just simplify this message with "passed a redirected Response to FetchEvent.respondWith()".

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8737158 [details] [diff] [review]
Bug 1243792 - P2 tests in wpt/mozilla

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

This is a good start, but I'd like to see it again.  Also note that this will need to be rebased since I moved the tests into the upstream directory.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-response-redirected.https.html
@@ +12,5 @@
> +      throw new Error(description + ' - ' + reason.message);
> +  });
> +}
> +
> +function assert_rejects(promise, description) {

I realize the other test case you copied this from uses this self defined function, but you could instead use assert_promise_rejects():

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/testharness-helpers.js#23

@@ +20,5 @@
> +}
> +
> +promise_test(function(t) {
> +    var SCOPE = 'resources/fetch-request-redirect-iframe.html';
> +    var SCRIPT = 'resources/fetch-redirected-worker.js';

Can you enhance fetch-rewrite-worker.js instead of adding a new worker script?

@@ +59,5 @@
> +                    './?url=' + encodeURIComponent(REDIRECT_TO_XHR_URL) +
> +                    '&redirect-mode=error' +
> +                    '&expected_redirected=false' +
> +                    '&cache'),
> +                'Redirected XHR with Request.redirect=error should fail and it should not be redirected'),

Please add a test case that shows .redirected works for a "cors" cross-origin response.

Also please add a test case showing that .redirected always returns false for a "no-cors" cross-origin fetch that results in an opaque response.

Comment 25

[Tom Tung [:tt, :ttung]]

Sorry for the late reply. Thanks for the feedback. I learn a lot from them, but I have some question about part of comments. I address them below inline.

(In reply to Ben Kelly [:bkelly] from comment #23)
> ::: dom/cache/DBSchema.cpp
> @@ +1768,5 @@
> > +  nsAutoCString url;
> > +  if (!responseUrlList.IsEmpty()) {
> > +    url = responseUrlList.LastElement();
> > +  }
> > +  rv = state->BindUTF8StringByName(NS_LITERAL_CSTRING("response_url"), url);
> 
> I don't think we need to store the response_url column in the entries table
> any more.  Can you remove it?

Sure, I forgot to remove it. I'll remove it in next patch.

> @@ +2652,5 @@
> > +      "url TEXT NOT NULL, "
> > +      "entry_id INTEGER NOT NULL REFERENCES entries(id) ON DELETE CASCADE"
> > +    ")"
> > +  ));
> > +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
> Unfortunately you cannot just piggy-back on the existing MigrateFrom16to17()
> method.  Instead you need to:
> 
> 1) Increment the kLatestSchemaVersion (right now that means 21).
> 2) Create a new MigrateFrom20to21() method and hook it into sMigrationList.
> 3) Write the migration logic in your new method.
> 
> In this case, since you are removing response_url you need to do the style
> of migrate shown in 16to17.  Basically you create the new schema in a
> temporary table, copy the data across, remove the old table, and then rename
> your new table.
> 
> Of course, you will also need to copy over the response_url from the old
> table and select it into your new response_urllist table before deleting the
> old table.

OK! I will create the "MigrateFrom20to21()". Besides, I create response_url_list table and remove response_url column in this function.
 
> Finally, please update the tests here to check response.url and
> response.redirected:
> 
Sure, but I found the value response.url is empty string(""). I found that it's empty without applying my patch, either. Should it be empty string?

> ::: dom/cache/TypeUtils.cpp
> @@ +203,5 @@
> >  {
> >    aOut.type() = aIn.Type();
> >  
> > +  aIn.GetUnfilteredUrlList(aOut.urllist());
> > +  nsTArray<nsCString> urllist;
> 
> AutoTArray<nsCString, 4>

OK but could you explain that why we set 4 elements of inline storage? Did you meant that a request(response) usually redirects three times?

> @@ +493,2 @@
> >    nsCOMPtr<nsIURI> newURI;
> >    rv = NS_GetFinalChannelURI(channel, getter_AddRefs(newURI));
> 
> Is this necessary?  The URL should already be in the list from
> AsyncOnChannelRedirect().  It seems we should never call SetURL(), but only
> append URLs.

It's not necessary after I calling AddURL() at AsyncOnChannelRedirect(). I'll remove it.

> ::: dom/fetch/InternalRequest.cpp
> @@ +24,5 @@
> >  InternalRequest::GetRequestConstructorCopy(nsIGlobalObject* aGlobal, ErrorResult& aRv) const
> >  {
> >    RefPtr<InternalRequest> copy = new InternalRequest();
> >    copy->mURL.Assign(mURL);
> > +  copy->mURLList.Assign(mURLList);
> 
> Actually, the spec only says to copy the original URL or first entry in the
> list.  So:
> 
>   if (r1.redirected) {
>     var r2 = new Request(r1);
>     assert(r2.redirected === false);
>   }
> 
> ::: dom/fetch/InternalRequest.h
> @@ +182,2 @@
> >    void
> >    GetURL(nsCString& aURL) const
> 
> While you are here, can you please make this take an nsACString?

Sure. Sorry for that. :(

> @@ +184,5 @@
> >    {
> > +    if (mURLList.IsEmpty()) {
> > +      aURL.Assign(EmptyCString());
> > +    } else {
> > +      aURL.Assign(mURLList.LastElement());
> 
> nit:  Can you use the short-circuit style here and elsewhere in the patch?
> 
>   if (mURLList.IsEmpty()) {
>     aURL.Truncate();
>     return;
>   }
> 
>   aURL = mURLList.LastElement();
> 
> @@ +190,5 @@
> > +  }
> > +
> > +  // SetURL should set the url in url list.
> > +  void
> > +  SetURL(const nsACString& aURL)
> 
> I think we should remove SetURL().  Is there a particular use case that
> requires us to keep it?  It seems we should only ever be appending to the
> list.

https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp#288
https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Request.cpp#319 
But I think they can be replaced by AddURL(). I will do it in next patch.
  
> @@ +481,1 @@
> >    nsCString mURL;
> 
> It seems we should not need this any more.  Can we remove it in favor of
> using the first entry in the mURLList?
> 
> I opened a spec bug about this as well:
> 
>   https://github.com/whatwg/fetch/issues/275

Thanks!! I was really confused when I implemented it. I'll remove mURL in next patch.

> ::: dom/fetch/InternalResponse.cpp
> @@ +118,5 @@
> > +    if(NS_WARN_IF(NS_FAILED(rv))){
> > +      return rv;
> > +    }
> > +
> > +    AddUrl(spec);
> 
> Can you accumulate the nsIURI references in an AutoTArray, and then call
> AddUrl on all of them after we are sure no errors are thrown?  That way we
> avoid any partial state on the object.

If we need to avoid any partial state on the object, why do not accumulate "spec"(nsCString) rather than "iuri" in an AutoTArray?

Comment 26

[Tom Tung [:tt, :ttung]]

Thanks for your comments and feedback again! I have some questions and address them below inline.

(In reply to Ben Kelly [:bkelly] from comment #24)
> :::
> testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-
> response-redirected.https.html
> @@ +12,5 @@
> > +      throw new Error(description + ' - ' + reason.message);
> > +  });
> > +}
> > +
> > +function assert_rejects(promise, description) {
> 
> I realize the other test case you copied this from uses this self defined
> function, but you could instead use assert_promise_rejects():

I just found that I do not need to add tests for promise rejects because "redirected" cannot be tested. I will remove the tests for promise rejects().
 
> @@ +20,5 @@
> > +}
> > +
> > +promise_test(function(t) {
> > +    var SCOPE = 'resources/fetch-request-redirect-iframe.html';
> > +    var SCRIPT = 'resources/fetch-redirected-worker.js';
> 
> Can you enhance fetch-rewrite-worker.js instead of adding a new worker
> script?

Sure. Should I also put the tests into fetch-request-redirect.https.html instead of adding another file(fetch-response-redirected.https.html)?

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

NI myself to look at these questions.

Comment 28

[Tom Tung [:tt, :ttung]]

Oh, there is one more question:
 
> ::: dom/fetch/InternalRequest.cpp
> @@ +24,5 @@
> >  InternalRequest::GetRequestConstructorCopy(nsIGlobalObject* aGlobal, ErrorResult& aRv) const
> >  {
> >    RefPtr<InternalRequest> copy = new InternalRequest();
> >    copy->mURL.Assign(mURL);
> > +  copy->mURLList.Assign(mURLList);
> 
> Actually, the spec only says to copy the original URL or first entry in the
> list.  So:
> 
>   if (r1.redirected) {
>     var r2 = new Request(r1);
>     assert(r2.redirected === false);
>   }
I cannot catch the meaning. Could you tell me more about that? Did you mean I should add an assertion for it?

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung from comment #25)
> > Finally, please update the tests here to check response.url and
> > response.redirected:
> > 
> Sure, but I found the value response.url is empty string(""). I found that
> it's empty without applying my patch, either. Should it be empty string?

Well, ideally we would update the test file to contain a database with a Response containing a URL.  But I think at least checking that the attributes work as expected with the current data would be ok.

> > AutoTArray<nsCString, 4>
> 
> OK but could you explain that why we set 4 elements of inline storage? Did
> you meant that a request(response) usually redirects three times?

4 is just a reasonable number that probable covers 99% of actual redirect depth.  I just arbitrarily chose it.  You could use another small number if you think its more appropriate.

> > Can you accumulate the nsIURI references in an AutoTArray, and then call
> > AddUrl on all of them after we are sure no errors are thrown?  That way we
> > avoid any partial state on the object.
> 
> If we need to avoid any partial state on the object, why do not accumulate
> "spec"(nsCString) rather than "iuri" in an AutoTArray?

You could do that too.  I just meant to accumulate the results after the fallible bits.  Getting the spec out of the nsIURI is more or less infallible.

> > Can you enhance fetch-rewrite-worker.js instead of adding a new worker
> > script?
> 
> Sure. Should I also put the tests into fetch-request-redirect.https.html
> instead of adding another file(fetch-response-redirected.https.html)?

That would be reasonable, yea.

> > ::: dom/fetch/InternalRequest.cpp
> > @@ +24,5 @@
> > >  InternalRequest::GetRequestConstructorCopy(nsIGlobalObject* aGlobal, ErrorResult& aRv) const
> > >  {
> > >    RefPtr<InternalRequest> copy = new InternalRequest();
> > >    copy->mURL.Assign(mURL);
> > > +  copy->mURLList.Assign(mURLList);

> I cannot catch the meaning. Could you tell me more about that? Did you mean
> I should add an assertion for it?

I mean this line of code is wrong.  We should not be copying the entire request URL list in InternalRequest::GetRequestConstructor().  Step 8 of the Request Constructor says:

"8. Set request to a new request whose url is request's current url, method is request's method, header list is a copy of request's header list, unsafe-request flag is set, client is entry settings object, window is window, origin is "client", omit-Origin-header flag is request's omit-Origin-header flag, same-origin data-URL flag is set, referrer is request's referrer, referrer policy is request's referrer policy, mode is request's mode, credentials mode is request's credentials mode, cache mode is request's cache mode, redirect mode is request's redirect mode, and integrity metadata is request's integrity metadata."

From here:

  https://fetch.spec.whatwg.org/#dom-request

The part saying "request whose url is request's current url" actually means to set the first and only entry in the new request's URL list to be the last entry in the old request's URL list.

So I think we want C++ that looks like this:

  copy->mURLList.AppendElement(mURLList.LastElement());

Thanks!

Comment 30

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1243791 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed. -v2

(In reply to Ben Kelly [:bkelly] from comment #29)

Thanks for your answering and comments. I address them into this patch. Could you help to review it again, Ben? Thanks!

> Well, ideally we would update the test file to contain a database with a
> Response containing a URL.  But I think at least checking that the
> attributes work as expected with the current data would be ok.
I add simple tests for that but I thought it should be changed once the test file is updated. Thus, I left comments for each of them.

Comment 31

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1243792 - P2 testing for response.redirected after caching request. -v2

In this patch, I put my test into fetch-request-redirected.https.html rather than create another file and addressed comments. Could you help me for review this patch, too? Thanks, again.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

FYI, I'm going to be traveling for the next day or so.  I will probably review this on Thursday US time.  Sorry for the delay!

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8740739 [details] [diff] [review]
Bug 1243791 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed. -v2

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

This is looking really good, but I think we should ensure that InternalRequest and InternalResponse objects have at least one URL when they are constructed.  This will help us avoid accidentally accessing information on entries in empty lists.

Thanks!

::: dom/cache/DBSchema.cpp
@@ +2820,5 @@
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  MOZ_ASSERT(aConn);
> +
> +  mozStorageTransaction trans(aConn, true,
> +                              mozIStorageConnection::TRANSACTION_IMMEDIATE);

You don't actually need a transaction here since one is created for you at:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/DBSchema.cpp#421

Can you remove the transactions from the other MigrateFrom* methods as well?

::: dom/cache/TypeUtils.cpp
@@ +203,5 @@
>  {
>    aOut.type() = aIn.Type();
>  
> +  aIn.GetUnfilteredUrlList(aOut.urlList());
> +  AutoTArray<nsCString, 4> url_list;

nit: Camel case style like urlList.

@@ +204,5 @@
>    aOut.type() = aIn.Type();
>  
> +  aIn.GetUnfilteredUrlList(aOut.urlList());
> +  AutoTArray<nsCString, 4> url_list;
> +  aIn.GetUrlList(url_list);

Can you add a MOZ_ASSERT(!urlList.IsEmpty()) here?

@@ +209,3 @@
>  
> +  for (uint32_t i = 0; i < url_list.Length(); i++) {
> +    if (aOut.urlList()[i] != EmptyCString()) {

I know this is code from before that you moved, but can you make this use .IsEmpty() instead?

@@ +211,5 @@
> +    if (aOut.urlList()[i] != EmptyCString()) {
> +      // Pass all Response URL schemes through... The spec only requires we take
> +      // action on invalid schemes for Request objects.
> +      ProcessURL(aOut.urlList()[i], nullptr, nullptr, nullptr, aRv);
> +      if (aRv.Failed()) {

Can you add an NS_WARN_IF() here?

::: dom/fetch/FetchDriver.cpp
@@ +674,5 @@
> +  MOZ_ALWAYS_SUCCEEDS(aNewChannel->GetURI(getter_AddRefs(uri)));
> +
> +  nsCString url;
> +  uri->GetSpec(url);
> +  mRequest->AddURL(url);

I think there is a bug in the old code in that we don't strip the fragment when getting the redirected URL.  I mention this in InterntalRequest as well.

Please call uri->CloneIgnoringRef() and then get the spec from that stripped nsIURI.

I think this also means we can then just setURLList() on the response instead of StripFragmentAndSetURLList().

::: dom/fetch/InternalRequest.cpp
@@ +23,5 @@
>  already_AddRefed<InternalRequest>
>  InternalRequest::GetRequestConstructorCopy(nsIGlobalObject* aGlobal, ErrorResult& aRv) const
>  {
>    RefPtr<InternalRequest> copy = new InternalRequest();
> +  if (!mURLList.IsEmpty()) {

Actually, this should never happen.  Can you add a MOZ_RELEASE_ASSERT(!mURLList.IsEmpty()) before the if-statement here?

::: dom/fetch/InternalRequest.h
@@ +149,5 @@
>      // strip the fragment first.  Since these should be well formed URLs we
>      // can use a simple check for a fragment here.  The full parser is
>      // difficult to use off the main thread.
> +    mURLList.AppendElement(aURL);
> +    MOZ_ASSERT(mURLList.LastElement().Find(NS_LITERAL_CSTRING("#")) == kNotFound);

Please call AddURL() here to avoid duplicating the append operation?  You should also use the comment about stripping fragments and the assertion to AddURL().

Also, this ensures the list has one URL for this constructor, but the default constructor can create an empty list.  I think to avoid bugs we should prevent the default constructor from doing this.

Can you change the default InternalRequest() constructor to take a single URL string parameter?  This then lets us assert that the URL list is non-empty.

To do this you will need to update the callers of the default constructor of course.  Most of them should be pretty straightforward, but you will need to move some code around in Request::Constructor() to make it work.

@@ +182,2 @@
>    {
> +    if (mURLList.IsEmpty()) {

I think you can remove this conditional and replace it with a MOZ_ASSERT_RELEASE(!mURLList.IsEmpty()).

@@ +191,5 @@
> +  // AddURL should append the url into url list
> +  void
> +  AddURL(const nsACString& aURL)
> +  {
> +    mURLList.AppendElement(aURL);

This matches the previous behavior, but it is bugged.  We need to strip the fragment from the URL here.

Can you add an assertion here similar to the constructor?  Then make FetchDriver.cpp strip the fragment before call AddURL()?

::: dom/fetch/InternalResponse.h
@@ +80,4 @@
>    void
>    GetUrl(nsCString& aURL) const
>    {
> +    if (mURLList.IsEmpty()) {

Please change this to a MOZ_RELEASE_ASSERT(!mURLList.IsEmpty()).

Related to this, I think we should make the InternalRequest(status, statusText) constructor require a URL.  Again, this will ensure we always have at least one entry in the URL list.  This will require a bit of code rework in the callers, though.

@@ +116,5 @@
> +
> +  void
> +  SetUrlList(const nsTArray<nsCString>& aURLList)
> +  {
> +    mURLList.Assign(aURLList);

Can you add an #ifdef DEBUG loop here that verifies all the URLs in the list have the fragment stripped?

@@ +122,5 @@
> +
> +  void
> +  AddUrl(const nsACString& aURL)
> +  {
> +    mURLList.AppendElement(aURL);

I'm actually not sure we need this now.  It seems we should only be setting the entire URL list from the request.

::: dom/fetch/Request.cpp
@@ +315,5 @@
>      if (NS_WARN_IF(aRv.Failed())) {
>        return nullptr;
>      }
>  
> +    request->AddURL(NS_ConvertUTF16toUTF8(requestURL));

This is the code that would get moved around so the requestURL is passed directly into new InternalRequest().

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8740740 [details] [diff] [review]
Bug 1243792 - P2 testing for response.redirected after caching request. -v2

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

Thats for merging this with the other redirect test and existing worker.  I think there might be a bug in the test logic, though.  See the comments inline.

Thanks!

::: testing/web-platform/tests/service-workers/service-worker/fetch-request-redirect.https.html
@@ +226,5 @@
> +              // tests for request's mode = no-cors
> +              assert_resolves(frame.contentWindow.xhr(REDIRECT_TO_XHR_URL +
> +                      '&mode=no-cors' +
> +                      '&redirect-mode=follow' +
> +                      '&expected_redirected=false' +

Is this correct?  Shouldn't it be true?

Can you add another case to redirect to a cross-origin?  Then the opaque response should result in a false .redirected.

::: testing/web-platform/tests/service-workers/service-worker/resources/fetch-rewrite-worker.js
@@ +95,5 @@
>              })));
>            }
>  
> +          var expectedRedirected = params['expected_redirected'];
> +          if (expectedRedirected && response.redirected.toString() !==

I think you should check |'expectedRedirected' in params|.  Right now you don't actually check the .redirected attribute if false is expected.

@@ +98,5 @@
> +          var expectedRedirected = params['expected_redirected'];
> +          if (expectedRedirected && response.redirected.toString() !==
> +              expectedRedirected) {
> +            resolve(new Response(JSON.stringify({
> +              reslut: 'failure',

"result"

@@ +99,5 @@
> +          if (expectedRedirected && response.redirected.toString() !==
> +              expectedRedirected) {
> +            resolve(new Response(JSON.stringify({
> +              reslut: 'failure',
> +              detial: 'got '+ response.redirected +

"detail"

Comment 35

[Tom Tung [:tt, :ttung]]

Thanks for comments and feedback! I'll address them in my next patch! I left responses and few questions below.

(In reply to Ben Kelly [:bkelly] from comment #33)
> ::: dom/cache/DBSchema.cpp
> @@ +2820,5 @@
> > +  MOZ_ASSERT(!NS_IsMainThread());
> > +  MOZ_ASSERT(aConn);
> > +
> > +  mozStorageTransaction trans(aConn, true,
> > +                              mozIStorageConnection::TRANSACTION_IMMEDIATE);
> 
> You don't actually need a transaction here since one is created for you at:
> 
>   https://dxr.mozilla.org/mozilla-central/source/dom/cache/DBSchema.cpp#421
> 
> Can you remove the transactions from the other MigrateFrom* methods as well?

Sure.

> ::: dom/cache/TypeUtils.cpp
> @@ +203,5 @@
> >  {
> >    aOut.type() = aIn.Type();
> >  
> > +  aIn.GetUnfilteredUrlList(aOut.urlList());
> > +  AutoTArray<nsCString, 4> url_list;
> 
> nit: Camel case style like urlList.

I found that I should not create another string array and get internal response again. I'll use aOut.urllist() for boundary of for-loop rather than create another array.

> @@ +204,5 @@
> >    aOut.type() = aIn.Type();
> >  
> > +  aIn.GetUnfilteredUrlList(aOut.urlList());
> > +  AutoTArray<nsCString, 4> url_list;
> > +  aIn.GetUrlList(url_list);
> 
> Can you add a MOZ_ASSERT(!urlList.IsEmpty()) here?

Let me check for this. Since the response's url can be empty in original design logic, we need to ensure response's url cannot be empty for all the tests. 

> @@ +209,3 @@
> >  
> > +  for (uint32_t i = 0; i < url_list.Length(); i++) {
> > +    if (aOut.urlList()[i] != EmptyCString()) {
> 
> I know this is code from before that you moved, but can you make this use
> .IsEmpty() instead?
OK

> @@ +211,5 @@
> > +    if (aOut.urlList()[i] != EmptyCString()) {
> > +      // Pass all Response URL schemes through... The spec only requires we take
> > +      // action on invalid schemes for Request objects.
> > +      ProcessURL(aOut.urlList()[i], nullptr, nullptr, nullptr, aRv);
> > +      if (aRv.Failed()) {
> 
> Can you add an NS_WARN_IF() here?
Sure.

> ::: dom/fetch/InternalRequest.cpp
> @@ +23,5 @@
> >  already_AddRefed<InternalRequest>
> >  InternalRequest::GetRequestConstructorCopy(nsIGlobalObject* aGlobal, ErrorResult& aRv) const
> >  {
> >    RefPtr<InternalRequest> copy = new InternalRequest();
> > +  if (!mURLList.IsEmpty()) {
> 
> Actually, this should never happen.  Can you add a
> MOZ_RELEASE_ASSERT(!mURLList.IsEmpty()) before the if-statement here?

Sure, but I am curious that why we need to keep the if-statement if we have assertion?
 
> ::: dom/fetch/InternalRequest.h
> @@ +149,5 @@
> >      // strip the fragment first.  Since these should be well formed URLs we
> >      // can use a simple check for a fragment here.  The full parser is
> >      // difficult to use off the main thread.
> > +    mURLList.AppendElement(aURL);
> > +    MOZ_ASSERT(mURLList.LastElement().Find(NS_LITERAL_CSTRING("#")) == kNotFound);
> 
> Please call AddURL() here to avoid duplicating the append operation?  You
> should also use the comment about stripping fragments and the assertion to
> AddURL().
> 
> Also, this ensures the list has one URL for this constructor, but the
> default constructor can create an empty list.  I think to avoid bugs we
> should prevent the default constructor from doing this.
> 
> Can you change the default InternalRequest() constructor to take a single
> URL string parameter?  This then lets us assert that the URL list is
> non-empty.

OK.

> ::: dom/fetch/InternalResponse.h
> @@ +80,4 @@
> >    void
> >    GetUrl(nsCString& aURL) const
> >    {
> > +    if (mURLList.IsEmpty()) {
> 
> Please change this to a MOZ_RELEASE_ASSERT(!mURLList.IsEmpty()).
> 
> Related to this, I think we should make the InternalRequest(status,
> statusText) constructor require a URL.  Again, this will ensure we always
> have at least one entry in the URL list.  This will require a bit of code
> rework in the callers, though.
> 
> @@ +116,5 @@
> > +
> > +  void
> > +  SetUrlList(const nsTArray<nsCString>& aURLList)
> > +  {
> > +    mURLList.Assign(aURLList);
> 
> Can you add an #ifdef DEBUG loop here that verifies all the URLs in the list
> have the fragment stripped?

Could you tell me more about how to verifies Url haves the fragment stripped? I have no idea about that.

> @@ +122,5 @@
> > +
> > +  void
> > +  AddUrl(const nsACString& aURL)
> > +  {
> > +    mURLList.AppendElement(aURL);
> 
> I'm actually not sure we need this now.  It seems we should only be setting
> the entire URL list from the request.

You are right. Since it is only called by StripFragmentAndSetUrlList(), I will remove the function and add implementation into it.

> ::: dom/fetch/Request.cpp
> @@ +315,5 @@
> >      if (NS_WARN_IF(aRv.Failed())) {
> >        return nullptr;
> >      }
> >  
> > +    request->AddURL(NS_ConvertUTF16toUTF8(requestURL));
> 
> This is the code that would get moved around so the requestURL is passed
> directly into new InternalRequest().
I would like to try move the GetRequestConstructorCopy() from L290[1] to L324[2]. Therefore, we could have a url before calling constructor.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Request.cpp#290-293
[2] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Request.cpp#324

Comment 36

[Tom Tung [:tt, :ttung]]

Thanks again! I'll apply all the comments in next patch.
(In reply to Ben Kelly [:bkelly] from comment #34)
> :::
> testing/web-platform/tests/service-workers/service-worker/fetch-request-
> redirect.https.html
> @@ +226,5 @@
> > +              // tests for request's mode = no-cors
> > +              assert_resolves(frame.contentWindow.xhr(REDIRECT_TO_XHR_URL +
> > +                      '&mode=no-cors' +
> > +                      '&redirect-mode=follow' +
> > +                      '&expected_redirected=false' +
> 
> Is this correct?  Shouldn't it be true?

It should be true. Sorry for that.

> Can you add another case to redirect to a cross-origin?  Then the opaque
> response should result in a false .redirected.

Sure. I will add them in next patch.

> @@ +98,5 @@
> "result"
> "detail"
Sorry for that.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung from comment #35)
> > Actually, this should never happen.  Can you add a
> > MOZ_RELEASE_ASSERT(!mURLList.IsEmpty()) before the if-statement here?
> 
> Sure, but I am curious that why we need to keep the if-statement if we have
> assertion?

You're correct.  With MOZ_RELEASE_ASSERT() the if-statement should be removed.  My text here was from a previous version of my comments where I was going to suggest just MOZ_ASSERT().  I think we should use the stricter release assertion, though.

> > Can you add an #ifdef DEBUG loop here that verifies all the URLs in the list
> > have the fragment stripped?
> 
> Could you tell me more about how to verifies Url haves the fragment
> stripped? I have no idea about that.

Sure.  Since we've already parsed and verified URLs by this point you can just use this assertion:
  
  MOZ_ASSERT(mURL.Find(NS_LITERAL_CSTRING("#")) == kNotFound);

From here:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.h?from=InternalRequest#153

Comment 38

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #37)
Thanks for the hint and responses! 

There are two more things. 
1. Should we add an assertion at GetUrl() in InternalResponse? I got the test failed [1] if I did that. In these kinds of test, the value of response should be hidden and we should get a empty url.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/test_fetch_cors.js#1701

2. If I modify the constructor of InternalResponse from "InternalResponse(aStatus,aStatusText)" to "InternalResponse(aStatus,aStatusText,aURL)", we need to deal with situation like NetworkError() [2]. That means we need to let InternalResponse's ctor accept empty url, right? 

[2] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalResponse.h#40

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Well, I would expect things like NetworkError to pass EmptyCString() for the URL.  But there should always be at least one in the list.  Does that make sense?

Comment 40

[Tom Tung [:tt, :ttung]]

Oh I see, so there is no assertion for checking empty url in InternalResponse's ctor. The purpose for taking a single URL string parameter is to make sure there is at least one url in url list. Thanks!

But I still have no idea to deal with avoiding tests failed after add an assertion(empty url) at GetUrl() in InternalResponse. Could you give me some hints for this?

Comment 41

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung from comment #40)
I just found this problem will be solved if we can ensure there at least having one url in the list. Sorry for disturbance. :(

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Yea, I think it should all work out if we ensure there is always at least one entry in the list.  Its ok for that one entry to be EmptyCString().  So that test should pass.  Thanks!

Comment 43

[Tom Tung [:tt, :ttung]]

Sorry for late reply, I get stuck in making a testcase for cors url. I can pass the similar test [1] but I got fetch error in my own test. I'll upload my patch after running try server. 

When I implement adding a url paramenter for interalResponse's ctor, I found that there may have a conflict in spec for opaque filtered response [2]. In the spec, "... filtered response whose type is "opaque", url list is the empty list ...", but we need to have at least a url when we create a internalResposne(). 

Currently, my solution is to create a internalResponse() with an emptyCString() in OpaqueResponse().

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-event-redirect.https.html#531-544
[2] https://fetch.spec.whatwg.org/#concept-filtered-response-opaque

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

> Currently, my solution is to create a internalResponse() with an emptyCString() in OpaqueResponse().

That seems correct to me.  I think the spec text is not quite right.

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

Actually, after talking with Anne I think I was wrong here.  A Response can have zero entries in its URL list.  I forgot that empty string is technically not a valid URL value.

I still think we should ensure a URL is always passed to Request, though.

So, I guess can you put back the InternalResponse logic to allow an empty URL list, but keep the changes to ensure a non-empty Request url list?

I'm sorry for the churn here. :-(

Comment 46

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #45)
> So, I guess can you put back the InternalResponse logic to allow an empty
> URL list, but keep the changes to ensure a non-empty Request url list?
Sure, I'll do it. 

> I'm sorry for the churn here. :-(
It's OK. Actually, I think it's a great time for me to be much more familiar with how it work in gecko.

Comment 47

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1243791 - v3 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed.

Hi Ben,

I addressed all comments except ensuring at least having one url in InternalResponse(). Can you help me to review this patch again?

Comment 48

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1243792 - wip - P2 testing for response.redirected after caching request

In this patch, I changed the tests to make sure all the parameters being passed and address part of comments. 

However, I cannot passed the test for redirecting to a cross-origin (L88-L95 in this patch). I will receive error event from [1] and thus I got rejected promise. I found there already have test [2] for same case(redirect to a cors) and the promise will been resolved. 

Currently, I change it to assert_reject let the test pass and leave ToDo comment for it.

Can you give me some hints or feedback for why this happen, Ben? 

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/fetch-request-redirect-iframe.html#7
[2] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-event-redirect.https.html#531-544

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Sorry for the delays on the review in this bug.  I'm sick today and don't trust myself to review this in my current state.  I hope to get to it tomorrow if I feel better.

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8743334 [details] [diff] [review]
Bug 1243791 - v3 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed.

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

Thanks for your patience, Tom!  There are a few nits, but overall this looks quite good.  r=me with action items addressed.

Sorry again for the delay in the review.  This stuff is complicated enough that I didn't want to review it while I was sick.

::: dom/cache/TypeUtils.cpp
@@ +209,3 @@
>  
> +  for (uint32_t i = 0; i < aOut.urlList().Length(); i++) {
> +    if (!aOut.urlList()[i].IsEmpty()) {

I think the way we have structured things now, there should not be an empty URL in the list.  An empty response URL is expressed as an empty list, right?

So can we just MOZ_ASSERT(!aOut.urlList()[i].IsEmpty())?

::: dom/fetch/FetchDriver.cpp
@@ +380,5 @@
>    MOZ_ASSERT(aResponse);
> +
> +  AutoTArray<nsCString, 4> reqURLList;
> +  mRequest->GetURLList(reqURLList);
> +  aResponse->SetUrlList(reqURLList);

Please add a MOZ_ASSERT(!reqURLList.IsEmpty()) before calling SetUrlList().

::: dom/fetch/InternalRequest.h
@@ +190,5 @@
> +  // difficult to use off the main thread.
> +  void
> +  AddURL(const nsACString& aURL)
> +  {
> +    mURLList.AppendElement(aURL);

Please add a MOZ_ASSERT(!aURL.IsEmpty()).  I don't think the request should ever see an empty URL.  Right?

::: dom/fetch/InternalResponse.cpp
@@ +133,3 @@
>    RefPtr<InternalResponse> response = OpaqueResponse();
>    response->mType = ResponseType::Opaqueredirect;
> +  response->mURLList.AppendElement(mURLList.LastElement());

This should copy the entire URL list, not just the last element.  An opaque-redirect response should return true from .redirected.

Please add a test case this.  You should be able to add the check to the service worker script used in fetch-request-redirect.https.html.

::: dom/fetch/InternalResponse.h
@@ +89,5 @@
> +    aURL.Assign(mURLList.LastElement());
> +  }
> +
> +  void
> +  GetUrlList(nsTArray<nsCString>& aURLList) const

Please capitalize URL in GetUrlList(), SetUrlList(), etc.  Thats the style this patch uses in Request, so lets be consistent.

@@ +121,5 @@
> +    mURLList.Assign(aURLList);
> +
> +#ifdef DEBUG
> +    for(uint32_t i = 0; i < mURLList.Length(); ++i) {
> +      MOZ_ASSERT(mURLList[i].Find(NS_LITERAL_CSTRING("#")) == kNotFound);

Please also MOZ_ASSERT(!mURLList[i].IsEmpty()).

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8743341 [details] [diff] [review]
Bug 1243792 - wip - P2 testing for response.redirected after caching request

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

Looks good, but it still needs to be fleshed out a bit.  Thanks!

::: testing/web-platform/tests/service-workers/service-worker/fetch-request-redirect.https.html
@@ +204,5 @@
> +              // XMLHttpRequest tests.
> +              assert_resolves(frame.contentWindow.xhr(
> +                              './?url=' + encodeURIComponent(XHR_URL) +
> +                              '&expected_redirected=false' +
> +                              '&cache'),

Can you add variants that include "cache" and variants that don't use "cache"?  I think we should verify both paths.

@@ +214,5 @@
> +                              'Redirected XHR should succeed and response.redirected should be redirected.'),
> +              assert_resolves(
> +                  frame.contentWindow.xhr(
> +                      './?url=' + encodeURIComponent(REDIRECT_TO_XHR_URL) +
> +                      '&redirect-mode=follow' +

Is this test necessary?  The default is follow and we're not testing manual or error modes.  I think we could just skip this variant.

@@ +224,5 @@
> +                      './?url=' + encodeURIComponent(XHR_URL) +
> +                      '&mode=cors' +
> +                      '&expected_redirected=false' +
> +                      '&cache'),
> +                  'XHR should succeed and response.redirected should not set for Normal XHR.'),

Please change the test description strings to indicate you are doing CORS here.

@@ +233,5 @@
> +                      '&expected_redirected=true' +
> +                      '&cache'),
> +                  'Redirected XHR should succeed and response.redirected should be redirected.'),
> +              // tests for request's mode = no-cors
> +              assert_resolves(frame.contentWindow.xhr(

If you add some test cases that use fetch() instead of XHR, then I think you can pass redirect:"manual", get an "opaqueredirect" response, and verify that .redirected is still true.

@@ +247,5 @@
> +                      './?url=' + encodeURIComponent(REDIRECT_TO_CROSS_ORIGIN) +
> +                      '&mode=no-cors' +
> +                      '&redirect-mode=follow' +
> +                      '&expected_redirected=false'),
> +                  'Response.redirected should not be redirected.'),

I think this is better described as Response.redirected should always be false for opaque responses.

Comment 52

[Tom Tung [:tt, :ttung]]

Thanks for your reviews and feedback, Ben. I really learned a lot from them. But I still have somethings that are not clear. I left them and comments below inline.

(In reply to Ben Kelly [:bkelly] from comment #50)
> Sorry again for the delay in the review.  This stuff is complicated enough
> that I didn't want to review it while I was sick.

It's OK. Take care! Please review/reply it when you feel better!

> ::: dom/cache/TypeUtils.cpp
> @@ +209,3 @@
> So can we just MOZ_ASSERT(!aOut.urlList()[i].IsEmpty())?
Sure.

> ::: dom/fetch/FetchDriver.cpp
> @@ +380,5 @@
> >    MOZ_ASSERT(aResponse);
> Please add a MOZ_ASSERT(!reqURLList.IsEmpty()) before calling SetUrlList().

I think "adding assertions for URL_List when we set/add URL and create request" is enough to ensure that there is no any chance to have a request with empty URL_List. Is there any possible that we could have a request with empty url_List?

> ::: dom/fetch/InternalRequest.h
> @@ +190,5 @@
> > +  AddURL(const nsACString& aURL)
> Please add a MOZ_ASSERT(!aURL.IsEmpty()).  I don't think the request should
> ever see an empty URL.  Right?

Yes, we should use that each time when we add a URL. I'll implement it! 

> ::: dom/fetch/InternalResponse.cpp
> @@ +133,3 @@
> >    RefPtr<InternalResponse> response = OpaqueResponse();
> >    response->mType = ResponseType::Opaqueredirect;
> > +  response->mURLList.AppendElement(mURLList.LastElement());
> 
> This should copy the entire URL list, not just the last element.  An
> opaque-redirect response should return true from .redirected.
> 
> Please add a test case this.  You should be able to add the check to the
> service worker script used in fetch-request-redirect.https.html.

I'm curious about why opaque-redirect response will return true as calling response.redirected. It seems that we don't perform http-redirect-fetch when the redirect-mode is not "follow" [2]. And we add redirected url into urlList http-redirect-fetch. So why we will have multiple url(response.redirected == true) in urlList when the redirect-mode is "manual"? Is there anything wrong? Could tell me more about that?

[1] https://fetch.spec.whatwg.org/#concept-filtered-response-opaque-redirect
[2] https://fetch.spec.whatwg.org/#dfnReturnLink-0

(In reply to Ben Kelly [:bkelly] from comment #51)
> Can you add variants that include "cache" and variants that don't use
> "cache"?  I think we should verify both paths.

Sure. I thought cache can test for response and cached response both in the same time, but it's better verify both paths. I'll do it.

> 
> @@ +214,5 @@
> > +                              'Redirected XHR should succeed and response.redirected should be redirected.'),
> > +              assert_resolves(
> > +                  frame.contentWindow.xhr(
> > +                      './?url=' + encodeURIComponent(REDIRECT_TO_XHR_URL) +
> > +                      '&redirect-mode=follow' +
> 
> Is this test necessary?  The default is follow and we're not testing manual
> or error modes.  I think we could just skip this variant.
No, sorry for that.

> @@ +233,5 @@
> > +                      '&expected_redirected=true' +
> > +                      '&cache'),
> > +                  'Redirected XHR should succeed and response.redirected should be redirected.'),
> > +              // tests for request's mode = no-cors
> > +              assert_resolves(frame.contentWindow.xhr(
> 
> If you add some test cases that use fetch() instead of XHR, then I think you
> can pass redirect:"manual", get an "opaqueredirect" response, and verify
> that .redirected is still true.

I left the questions above.

> @@ +247,5 @@
> > +                      './?url=' + encodeURIComponent(REDIRECT_TO_CROSS_ORIGIN) +
> > +                      '&mode=no-cors' +
> > +                      '&redirect-mode=follow' +
> > +                      '&expected_redirected=false'),
> > +                  'Response.redirected should not be redirected.'),
> 
> I think this is better described as Response.redirected should always be
> false for opaque responses.

Sure, but I have one question here. Should promise(response) be rejected here? I thought it should be resolved but I found it have been rejected in this test. Could you tell me more about that?

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung from comment #52)
> > ::: dom/fetch/FetchDriver.cpp
> > @@ +380,5 @@
> > >    MOZ_ASSERT(aResponse);
> > Please add a MOZ_ASSERT(!reqURLList.IsEmpty()) before calling SetUrlList().
> 
> I think "adding assertions for URL_List when we set/add URL and create
> request" is enough to ensure that there is no any chance to have a request
> with empty URL_List. Is there any possible that we could have a request with
> empty url_List?

I guess I wanted to assert that we never call InternalResponse::SetURLList() with an empty list.  But we can't directly do that because the Cache TypeUtils.cpp might call SetURLList(emptyList) if there are no URLs for that response in the database.

I guess we could put the assert in InternalResponse::SetURLList() and Cache TypeUtils.cpp could just conditionally call it if ~list.IsEmpty().

> > ::: dom/fetch/InternalResponse.cpp
> > @@ +133,3 @@
> > >    RefPtr<InternalResponse> response = OpaqueResponse();
> > >    response->mType = ResponseType::Opaqueredirect;
> > > +  response->mURLList.AppendElement(mURLList.LastElement());
> > 
> > This should copy the entire URL list, not just the last element.  An
> > opaque-redirect response should return true from .redirected.
> > 
> > Please add a test case this.  You should be able to add the check to the
> > service worker script used in fetch-request-redirect.https.html.
> 
> I'm curious about why opaque-redirect response will return true as calling
> response.redirected. It seems that we don't perform http-redirect-fetch when
> the redirect-mode is not "follow" [2]. And we add redirected url into
> urlList http-redirect-fetch. So why we will have multiple
> url(response.redirected == true) in urlList when the redirect-mode is
> "manual"? Is there anything wrong? Could tell me more about that?

You're correct.  An opaque redirect should have an isRedirected of false!  I find that confusing :-)

I think we still want to copy the full URL list, though.

Since this part if confusing I think adding a test with a comment about the expected outcome would be good.

> (In reply to Ben Kelly [:bkelly] from comment #51)
> > Can you add variants that include "cache" and variants that don't use
> > "cache"?  I think we should verify both paths.
> 
> Sure. I thought cache can test for response and cached response both in the
> same time, but it's better verify both paths. I'll do it.

This test doesn't previously use the "cache" param at all.  I do think its worth adding it for the isRedirected tests, though, since we are changing the db schema to accomodate this feature.

> > @@ +247,5 @@
> > > +                      './?url=' + encodeURIComponent(REDIRECT_TO_CROSS_ORIGIN) +
> > > +                      '&mode=no-cors' +
> > > +                      '&redirect-mode=follow' +
> > > +                      '&expected_redirected=false'),
> > > +                  'Response.redirected should not be redirected.'),
> > 
> > I think this is better described as Response.redirected should always be
> > false for opaque responses.
> 
> Sure, but I have one question here. Should promise(response) be rejected
> here? I thought it should be resolved but I found it have been rejected in
> this test. Could you tell me more about that?

Oh, this is using an XHR which does a CORS request.  So the original FetchEvent.request is mode='cors'.  You can only FetchEvent.respondWith() an opaque response if mode='no-cors'.  If you did an img request instead of an xhr here the respondWith() should succeed I think (since img gets no-cors mode).

By the way, if you want to add fetch() based tests, it might be easier to do that in fetch-event-redirect.https.html.  That one does all fetch() calls instead of xhr, img, etc.  There are two different test files because google wrote fetch-request-redirect.https.html and we wrote fetch-event-redirect.https.html.

Comment 54

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1243792 - v3 - P2 testing for response.redirected after caching request

Hi Ben, could you help me to review this patch? I addressed all the comments and added few tests in fetch-event-redirect.https.html. Thanks!

Comment 55

[Tom Tung [:tt, :ttung]]

Attachment: [Final]Bug 1243791 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed. r=bkelly.

Thanks for the review and feedback, Ben. I addressed all the comments except putting the assert in InternalResponse::SetURLList(). I believe that it's better to put the assert before calling InternalResponse::SetURLList().

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8746479 [details] [diff] [review]
Bug 1243792 - v3 - P2 testing for response.redirected after caching request

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

That's the right idea, but I think we should just check the expected_redirect on each test case in fetch-event-redirect.https.html.  Lets make it work with expected_type at the same time.  Thanks!

::: testing/web-platform/tests/service-workers/service-worker/fetch-event-redirect.https.html
@@ +1007,5 @@
> +    should_reject: true
> +  });
> +}, 'Non-navigation, manual redirect, same-origin mode Request redirected to ' +
> +   'cors without credentials should fail opaqueredirect interception and ' +
> +   'should not be redirected');

Actually, you don't need new test cases here.  You can just add the expected_redirected argument to the existing cases.  For example, this particular test case is the same conditions as:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-event-redirect.https.html#136

I think just adding the explicit expected_redirect:false or expected_redirected:true to all cases would be best.

::: testing/web-platform/tests/service-workers/service-worker/resources/fetch-event-redirect-iframe.html
@@ +8,5 @@
>        if (response.type === data.expected_type &&
>            (response.type === 'opaque' || response.type === 'opaqueredirect')) {
>          return {result: 'success', detail: ''};
> +      } else if (response.redirected.toString() === data.expected_redirected) {
> +        return {result: 'success', detail: ''};

You need to move this check up before the possible return due to response.type match.  I think we should check both type and redirected in the same test case.

Comment 57

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1243792 - v4 - P2 testing for response.redirected after caching request

Hi Ben,

Sorry for the late reply. Thanks for your feedback! I rewrite the tests in fetch-event-redirect.https.html and let it work with expected_type at the same time. Could you help me to review this patch? Thanks!

Comment 58

[Tom Tung [:tt, :ttung]]

Attachment: [Final]Bug 1243791 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed. r=bkelly.

Update this patch since there are few conflicts in files dom.properties and ServiceWorkerEvents.cpp to m-c.

Comment 59

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8747717 [details] [diff] [review]
Bug 1243792 - v4 - P2 testing for response.redirected after caching request

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

Overall looks good!  Thanks!  Just a few nits.  r=me with comments addressed.

::: testing/web-platform/tests/service-workers/service-worker/fetch-event-redirect.https.html
@@ +107,5 @@
>      name: 'nonav-manual-cors-redirects-to-sameorigin-nocreds',
>      redirect_dest: 'same-origin',
>      url_credentials: false,
>      expected_type: 'opaqueredirect',
> +    expected_redirected: 'false',

Why use the string 'false' instead of just false?

::: testing/web-platform/tests/service-workers/service-worker/resources/fetch-event-redirect-iframe.html
@@ +5,5 @@
>    fetch(new Request(data.url, data.request_init)).then(function(response) {
>      if (data.request_init.mode === 'no-cors' && data.redirect_dest != 'same-origin') {
>        if (response.type === data.expected_type &&
> +          (response.type === 'opaque' || response.type === 'opaqueredirect') &&
> +          response.redirected.toString() === data.expected_redirected) {

If we just pass booleans, can we remove the toString() here?

::: testing/web-platform/tests/service-workers/service-worker/resources/fetch-rewrite-worker.js
@@ +96,5 @@
>            }
>  
> +          var expectedRedirected = params['expected_redirected'];
> +          if (typeof expectedRedirected !== 'undefined' &&
> +              response.redirected.toString() !==

Again, can we get rid of this toString()?

@@ +98,5 @@
> +          var expectedRedirected = params['expected_redirected'];
> +          if (typeof expectedRedirected !== 'undefined' &&
> +              response.redirected.toString() !==
> +              expectedRedirected) {
> +            var expectedRsolves = params['expected_resolves'];

nit: spelling of "execptedResolves"

Also, can you add a comment that this is simply determining how to pass an error to the outer test case?  Some tests expect the overall network request to reject and some expect it to resolve.

Comment 60

[Tom Tung [:tt, :ttung]]

Thanks for review and your time, Ben. I will address all of them, but there is one which I'm not so sure how to do. Could you tell me how to deal with that? Thanks again!

(In reply to Ben Kelly [:bkelly] from comment #59)
> Comment on attachment 8747717 [details] [diff] [review]
> Bug 1243792 - v4 - P2 testing for response.redirected after caching request
> :::
> testing/web-platform/tests/service-workers/service-worker/resources/fetch-
> rewrite-worker.js
> @@ +96,5 @@
> >            }
> >  
> > +          var expectedRedirected = params['expected_redirected'];
> > +          if (typeof expectedRedirected !== 'undefined' &&
> > +              response.redirected.toString() !==
> 
> Again, can we get rid of this toString()?

OK, but I'm not sure about that. Since 'expectedRedirected' used by GET method via url from fetch-request-redirected.https.html, the params['expected_redirected'] is string by default. 
Can I rewrite the code as bellow?
> var expectedRedirected = params['expected_redirected'];
> if (typeof expectedRedirected !== 'undefined') {
>   var expected_redirected = (expectedRedirected === 'true');
>   if(response.redirected !== expected_redirected) {

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

I guess its fine the way it is.  Thanks!

Comment 62

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1243791 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed. r=bkelly.

Sorry for disturbance, I found there are few changes at dom.properties after running try with newest m-c. Update this patch for those changes.

Comment 63

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1243792 - P2 tests for response.redirected with and without cached. r=bkelly.

Thanks for review and feedback, Ben. Address all the comments in this patch.

Comment 64

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a82c0c1994&selectedJob=20275734

Comment 65

[Carsten Book [:Tomcat]]

seems this needs dom peer review:

WebIDL file dom/webidl/Response.webidl altered in changeset 3ea21e1b2271 without DOM peer review

Comment 66

[Tom Tung [:tt, :ttung]]

Sorry for that. I'll ask a dom peer to review my patch.

Comment 67

[Tom Tung [:tt, :ttung]]

Comment on attachment 8748036 [details] [diff] [review]
Bug 1243791 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed. r=bkelly.

Hi Andrea,

Would you mind helping me to review this patch? Thanks!

This patch is mainly for adding a "redirected" to Response in Fetch API and a some security check. This method is to check whether response's URL in URLList is more than one. 

To do this, we need to modify from current setting(only record the latest URL to request/response(mUrl) after redirecting) to new one(record each URL in a list(mUrlList) after redirecting). I also ensure that we need to have at least one url in InternalRequest. To do so, I add modify the ctor for InternalRequest and following few changes on callers. 

Furthermore, since response is relate to cache API, I also do the same things in cache API (modifying from URL to URLList). Besides, I add a new version (21) for migration.

Comment 68

[Andrea Marchesini [:baku]]

Comment on attachment 8748036 [details] [diff] [review]
Bug 1243791 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed. r=bkelly.

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

::: dom/cache/DBSchema.cpp
@@ -2530,5 @@
>    MOZ_ASSERT(!NS_IsMainThread());
>    MOZ_ASSERT(aConn);
>  
> -  mozStorageTransaction trans(aConn, true,
> -                              mozIStorageConnection::TRANSACTION_IMMEDIATE);

why this change? It seems completely unrelated and it should not be part of this patch.

Comment 69

[Tom Tung [:tt, :ttung]]

(In reply to Andrea Marchesini (:baku) from comment #68)
Hi Andrea,

Thanks for your review and your time!

> Comment on attachment 8748036 [details] [diff] [review]
> Bug 1243791 - P1 implement response.redirected - Fetch API & Cache API &
> ServiceWorker changed. r=bkelly.
> ::: dom/cache/DBSchema.cpp
> @@ -2530,5 @@
> >    MOZ_ASSERT(!NS_IsMainThread());
> >    MOZ_ASSERT(aConn);
> >  
> > -  mozStorageTransaction trans(aConn, true,
> > -                              mozIStorageConnection::TRANSACTION_IMMEDIATE);
> 
> why this change? It seems completely unrelated and it should not be part of
> this patch.

Sorry for missing to explain that in comment 67. 

I did that because comment 33. Ben suggested me to remove those transactions since one is created at [1] and we don't need to do that again. Therefore, I decided to remove them in my patch. Do I need to split them into different patch or bug? 

[1] https://dxr.mozilla.org/mozilla-central/source/dom/cache/DBSchema.cpp#421

Comment 70

[Ben Kelly [:bkelly, not reviewing]]

Yea, these transactions were duplicates.  I asked Tom to remove them so people stopped copying these bogus transactions to new migration functions.

Comment 71

[Andrea Marchesini [:baku]]

Tom, Ben, thanks for your comments.

Comment 72

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1243791 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed. r=bkelly, r=baku.

Comment 73

[Pulsebot]

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

Comment 74

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77a81adaa885

Comment 75

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1243792 - P1 implement response.redirected - Fetch API & Cache API & ServiceWorker changed. r=bkelly, r=baku.

Wrong bug number. Sorry for disturbance. :(

Comment 76

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

https://hg.mozilla.org/mozilla-central/rev/ffa78602ee0e
https://hg.mozilla.org/mozilla-central/rev/77a81adaa885

Comment 77

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/fetch-and-request-now-throws-if-redirect-is-detected-when-in-non-follow-redirect-mode/

Comment 78

[Eric Shepherd [:sheppy]]

Added document: https://developer.mozilla.org/en-US/docs/Web/API/Response/redirected

Updated https://developer.mozilla.org/en-US/docs/Web/API/Response to refer to it

Also updated these pages with some corrections and expanded details, as well as changing some terminology:

https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch
https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch

Anne has reviewed the "redirected" page so I'm comfortable calling this done and done.