Don't send the redirected request once the request was fallbacked to the network.

Don't send the redirected request once the request was fallbacked to the network.

According to the fetch spec (https://fetch.spec.whatwg.org/), once the request goes to the network, skip-service-worker flag must be set.
So if the server returns a redirect response, the next request must not be handled by service worker.

BUG=517364, 512764
Committed: https://crrev.com/eeff087680dd4fc41c1a2f92fffdc96546053b74
Cr-Commit-Position: refs/heads/master@{#342386}

[horo, 2015-08-06 08:04:13]

horo@chromium.org changed reviewers:
+ falken@chromium.org

[horo, 2015-08-06 08:04:14]

falken@
Could you please review?

[falken, 2015-08-07 04:38:11]

On 2015/08/06 08:04:14, horo wrote:
> falken@
> Could you please review?

looks good... can you edit the CL description to say it's per spec like the bug
says? "According to the fetch spec (https://fetch.spec.whatwg.org/), once the
request goes to the network..." 

I think this has some chance of breaking sites. Do you know if it's an old bug
of ours or did the spec change?

Can you add a test?

[horo, 2015-08-07 05:00:03]

On 2015/08/07 04:38:11, falken wrote:
> On 2015/08/06 08:04:14, horo wrote:
> > falken@
> > Could you please review?
> 
> looks good... can you edit the CL description to say it's per spec like the
bug
> says? "According to the fetch spec (https://fetch.spec.whatwg.org/), once the
> request goes to the network..." 

Done

> I think this has some chance of breaking sites. Do you know if it's an old bug
> of ours or did the spec change?

I think this is an old bug of ours.
"service worker default flag" was introduced in Jun 3, 2014
(https://github.com/whatwg/fetch/commit/4bab01d2914b1abeec2435f03047966f4ca28ba3)
when Fetch API spec was integrated with Service Worker.
And it was renamed to "skip service worker flag" in Jun 5, 2014
(https://github.com/whatwg/fetch/commit/d0ee97d5467ddde98cd5495ecb417c3e7728c7e2).

> Can you add a test?

Yes. I will add LayoutTests.

[falken, 2015-08-07 05:04:05]

On 2015/08/07 05:00:03, horo wrote:
> On 2015/08/07 04:38:11, falken wrote:
> > On 2015/08/06 08:04:14, horo wrote:
> > > falken@
> > > Could you please review?
> > 
> > looks good... can you edit the CL description to say it's per spec like the
> bug
> > says? "According to the fetch spec (https://fetch.spec.whatwg.org/), once
the
> > request goes to the network..." 
> 
> Done
> 
> > I think this has some chance of breaking sites. Do you know if it's an old
bug
> > of ours or did the spec change?
> 
> I think this is an old bug of ours.
> "service worker default flag" was introduced in Jun 3, 2014
>
(https://github.com/whatwg/fetch/commit/4bab01d2914b1abeec2435f03047966f4ca28ba3)
> when Fetch API spec was integrated with Service Worker.
> And it was renamed to "skip service worker flag" in Jun 5, 2014
>
(https://github.com/whatwg/fetch/commit/d0ee97d5467ddde98cd5495ecb417c3e7728c7e2).
> 
> > Can you add a test?
> 
> Yes. I will add LayoutTests.

OK lgtm assuming tests are added. We should be sure to publicize this in release
notes.

[horo, 2015-08-07 05:38:57]

horo@chromium.org changed reviewers:
+ jam@chromium.org

[horo, 2015-08-07 05:38:58]

jam@
Could you please review content/child/web_url_loader_impl.cc?

[jam, 2015-08-07 15:03:46]

lgtm

[horo, 2015-08-07 15:07:10]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-08-07 15:07:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1274973002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1274973002/20001

[commit-bot: I haz the power, 2015-08-07 17:02:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-07 17:02:04]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[horo, 2015-08-07 17:41:24]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-08-07 17:42:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1274973002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1274973002/20001

[commit-bot: I haz the power, 2015-08-07 18:23:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-07 18:24:16]

Patchset 2 (id:??) landed as
https://crrev.com/eeff087680dd4fc41c1a2f92fffdc96546053b74
Cr-Commit-Position: refs/heads/master@{#342386}

[michaeln, 2015-08-07 19:40:23]

michaeln@chromium.org changed reviewers:
+ michaeln@chromium.org

[michaeln, 2015-08-07 19:40:23]

https://codereview.chromium.org/1274973002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/1274973002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_controllee_request_handler.cc:90:
// must be set and the request shoud not go to the service worker.
Whoa, this can't be right?

User clicks link to bitly url (or googlesearch result link) which redirects to a
serviceworker enabled site for the actual/final location.

Desired result: The registered serviceworker handles the request for the final
location and does the right thing.

Actual result produced by this CL: The request for the final location skips the
registered serviceworker because... reasons.

That actual result has got to be a BUG?


I havent' been following the gory details of how redirection and serviceworkers
fold/collide together, but the inability to redirect from the network into a
serviceworker controlled urlspace seems like it could be a source of great
confusion and frustration?

Maybe I'm missing something?

https://codereview.chromium.org/1274973002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_controllee_request_handler.cc:111:
if (job_->ShouldFallbackToNetwork()) {
Also, is the difference in behavior for this fallback case intentional?

[horo, 2015-08-08 09:00:45]

Thank you for pointing it out.
I created a CL to fix the bug.
https://codereview.chromium.org/1280133002/

https://codereview.chromium.org/1274973002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/1274973002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_controllee_request_handler.cc:90:
// must be set and the request shoud not go to the service worker.
On 2015/08/07 19:40:23, michaeln wrote:
> Whoa, this can't be right?
> 
> User clicks link to bitly url (or googlesearch result link) which redirects to
a
> serviceworker enabled site for the actual/final location.
> 
> Desired result: The registered serviceworker handles the request for the final
> location and does the right thing.
> 
> Actual result produced by this CL: The request for the final location skips
the
> registered serviceworker because... reasons.
> 
> That actual result has got to be a BUG?
> 
> 
> I havent' been following the gory details of how redirection and
serviceworkers
> fold/collide together, but the inability to redirect from the network into a
> serviceworker controlled urlspace seems like it could be a source of great
> confusion and frustration?
> 
> Maybe I'm missing something?

Yes you are right!

Navigation request's redirect mode is "manual".
https://github.com/whatwg/fetch/issues/101#issuecomment-128269549
So, we should not set |skip_service_worker_| for main resource loads.

https://codereview.chromium.org/1274973002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_controllee_request_handler.cc:111:
if (job_->ShouldFallbackToNetwork()) {
On 2015/08/07 19:40:23, michaeln wrote:
> Also, is the difference in behavior for this fallback case intentional?

No. We should also set here.

If the original request's domain (ex: bitly) doesn't have registered SW,
|skip_service_worker_| wasn't set.
So I didn't notice the bug.