[WIP] [service worker] Fix detach controller and fallback to network mechanism

content/child/service_worker/service_worker_dispatcher.cc

 // Copyright 2013 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/child/service_worker/service_worker_dispatcher.h"
 
 #include "base/lazy_instance.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/thread_task_runner_handle.h"
@@ skipping 670 matching lines @@
     bool should_notify_controllerchange) {
   TRACE_EVENT2("ServiceWorker",
                "ServiceWorkerDispatcher::OnSetControllerServiceWorker",
                "Thread ID", thread_id,
                "Provider ID", provider_id);
 
   // Adopt the reference sent from the browser process and pass it to the
   // provider context if it exists.
   scoped_ptr<ServiceWorkerHandleReference> handle_ref = Adopt(info);
   ProviderContextMap::iterator provider = provider_contexts_.find(provider_id);
-  if (provider != provider_contexts_.end())
-    provider->second->OnSetControllerServiceWorker(handle_ref.Pass());
+  if (provider != provider_contexts_.end()) {
+    if (info.handle_id == kInvalidServiceWorkerVersionId)

[horo, 2015/11/27 10:03:26]

Please add comment about when this happens.
I think this happens only when failed to start SW while loading the main
resource.

[falken, 2015/12/01 03:46:06]

We also call NotifyControllerLost when we fail to read the SW from disk and it
had controllees (in SWReadFromDiskJob). This could happen if a SW started up for
the main resource load, then the thread got terminated (as it can due to idle
timeout), then later we try to wake it up (subresource request, push
notification) and fail to read from disk.

That detach is also probably buggy. I think we'll have to work on that in a
followup patch. Maybe the only safe thing to do in that case is kill the
renderer.

[falken, 2015/12/01 05:08:00]

Actually it happens in several other cases, looking at when
ServiceWorkerMsg_SetControllerServiceWorker is sent. Some notable places are:

(1: Registration failed
ServiceWorkerProviderHost::OnRegistrationFailed
ServiceWorkerProviderHost::DisassociateRegistration
ServiceWorkerProviderHost::SetControllerVersionAttribute
Send ServiceWorkerMsg_SetControllerServiceWorker

(2: claim)
ServiceWorkerProviderHost::ClaimedByRegistration
ServiceWorkerProviderHost::DisassociateRegistration
ditto

(3: after following redirect?)
ServiceWorkerControlleeRequestHandler::PrepareForMainResource
ServiceWorkerProviderHost::DisassociateRegistration
ditto

I think I'll need to follow up on whether these are safe. At a glance, (1)
shouldn't be necessary since we don't associate a registration until after
registration succeeded (although we call OnRegistrationFailed in the
SWReadFromDiskJob case above), (2) should be mostly OK since we call
AssociateRegistration immediately after that (but there's a window of time for
badness between the IPCs arriving), (3) should occur before the main resource
load.

[horo, 2015/12/02 03:10:42]

Thank you for the detailed explanation.

+      provider->second->OnSetControllerServiceWorker(nullptr);

[nhiroki, 2015/11/27 09:46:01]

How about throwing away invalid reference handle concept like this?

  1. ServiceWorkerRegistrationHandleReference::Adopt() returns nullptr if
|info.handle_id| is invalid,
  2. GetOrCreateServiceWorker() returns nullptr if the given handle reference is
nullptr.
  3. GetOrCreateServiceWorker() (d)checks the given handle's id is valid. 

[falken, 2015/12/01 03:46:06]

Seems like a good idea, working on a patch to do this.

+    else
+      provider->second->OnSetControllerServiceWorker(handle_ref.Pass());
+  }
 
   ProviderClientMap::iterator found = provider_clients_.find(provider_id);
   if (found != provider_clients_.end()) {
     // Get the existing worker object or create a new one with a new reference
     // to populate the .controller field.
     scoped_refptr<WebServiceWorkerImpl> worker = GetOrCreateServiceWorker(
         ServiceWorkerHandleReference::Create(info, thread_safe_sender_.get()));
     found->second->setController(WebServiceWorkerImpl::CreateHandle(worker),
                                  should_notify_controllerchange);
   }
@@ skipping 59 matching lines @@
   return ServiceWorkerRegistrationHandleReference::Adopt(
       info, thread_safe_sender_.get());
 }
 
 scoped_ptr<ServiceWorkerHandleReference> ServiceWorkerDispatcher::Adopt(
     const ServiceWorkerObjectInfo& info) {
   return ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_.get());
 }
 
 }  // namespace content