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

content/browser/service_worker/service_worker_controllee_request_handler.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_controllee_request_handler.h"
 
 #include "base/trace_event/trace_event.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_metrics.h"
 #include "content/browser/service_worker/service_worker_provider_host.h"
@@ skipping 25 matching lines @@
                                   provider_host,
                                   blob_storage_context,
                                   resource_type),
       is_main_resource_load_(
           ServiceWorkerUtils::IsMainResourceType(resource_type)),
       request_mode_(request_mode),
       credentials_mode_(credentials_mode),
       request_context_type_(request_context_type),
       frame_type_(frame_type),
       body_(body),
-      weak_factory_(this) {
-}
+      skip_service_worker_(false),
+      weak_factory_(this) {}
 
 ServiceWorkerControlleeRequestHandler::
     ~ServiceWorkerControlleeRequestHandler() {
   // Navigation triggers an update to occur shortly after the page and
   // its initial subresources load.
   if (provider_host_ && provider_host_->active_version()) {
     if (is_main_resource_load_)
       provider_host_->active_version()->ScheduleUpdate();
     else
       provider_host_->active_version()->DeferScheduledUpdate();
@@ skipping 21 matching lines @@
 
   // This may get called multiple times for original and redirect requests:
   // A. original request case: job_ is null, no previous location info.
   // B. redirect or restarted request case:
   //  a) job_ is non-null if the previous location was forwarded to SW.
   //  b) job_ is null if the previous location was fallback.
   //  c) job_ is non-null if additional restart was required to fall back.
 
   // We've come here by restart, we already have original request and it
   // tells we should fallback to network. (Case B-c)
-  if (job_.get() && job_->ShouldFallbackToNetwork()) {
+  // Once the request was fallbacked to the network, skip-service-worker flag
+  // must be set and the request shoud not go to the service worker.

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

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?

[horo, 2015/08/08 09:00:44]

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.

+  if ((job_.get() && job_->ShouldFallbackToNetwork()) || skip_service_worker_) {
+    skip_service_worker_ = true;
     job_ = NULL;
     return NULL;
   }
 
   // It's for original request (A) or redirect case (B-a or B-b).
   DCHECK(!job_.get() || job_->ShouldForwardToServiceWorker());
 
   job_ = new ServiceWorkerURLRequestJob(
       request, network_delegate, provider_host_, blob_storage_context_,
       resource_context, request_mode_, credentials_mode_,
       is_main_resource_load_, request_context_type_, frame_type_, body_);
   resource_context_ = resource_context;
 
   if (is_main_resource_load_)
     PrepareForMainResource(request);
   else
     PrepareForSubResource();
 
   if (job_->ShouldFallbackToNetwork()) {

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

Also, is the difference in behavior for this fallback case intentional?

[horo, 2015/08/08 09:00:44]

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.

     // If we know we can fallback to network at this point (in case
     // the storage lookup returned immediately), just return NULL here to
     // fallback to network.
     job_ = NULL;
     return NULL;
   }
 
   return job_.get();
 }
 
@@ skipping 148 matching lines @@
 }
 
 void ServiceWorkerControlleeRequestHandler::PrepareForSubResource() {
   DCHECK(job_.get());
   DCHECK(context_);
   DCHECK(provider_host_->active_version());
   job_->ForwardToServiceWorker();
 }
 
 }  // namespace content