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

content/child/service_worker/service_worker_dispatcher.cc

Index: content/child/service_worker/service_worker_dispatcher.cc
diff --git a/content/child/service_worker/service_worker_dispatcher.cc b/content/child/service_worker/service_worker_dispatcher.cc
index 9022cddd655ef6cc58e9a286621e28a1856e41b0..f6156a666273b1b36863ea88b174ed3f31519b5e 100644
--- a/content/child/service_worker/service_worker_dispatcher.cc
+++ b/content/child/service_worker/service_worker_dispatcher.cc
@@ -688,8 +688,12 @@ void ServiceWorkerDispatcher::OnSetControllerServiceWorker(
   // 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()) {