ServiceWorker: Ensure live registration during starting worker

content/browser/service_worker/service_worker_version.cc

Index: content/browser/service_worker/service_worker_version.cc
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index 53f05f35587fccc65f3b26b5fb4fadef4e6cd0c0..b9fcf7fb08d12f075a78d21fd751cc25ccf3cc1a 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -377,10 +377,32 @@ void ServiceWorkerVersion::StartWorker(const StatusCallback& callback) {
 void ServiceWorkerVersion::StartWorker(
     bool pause_after_download,
     const StatusCallback& callback) {
-  if (is_doomed()) {
+  if (!context_) {
+    RunSoon(base::Bind(callback, SERVICE_WORKER_ERROR_START_WORKER_FAILED));
+    return;
+  }
+
+  // Ensure live registration during starting worker so that the worker can get
+  // associated with registration on SWDispatcherHost::OnSetHostedVersionId().
+  context_->storage()->FindRegistrationForId(
+      registration_id_,
+      scope_.GetOrigin(),
+      base::Bind(&ServiceWorkerVersion::DidEnsureLiveRegistration,
+                 weak_factory_.GetWeakPtr(),
+                 pause_after_download,
+                 callback));
+}
+
+void ServiceWorkerVersion::DidEnsureLiveRegistration(
+    bool pause_after_download,
+    const StatusCallback& callback,
+    ServiceWorkerStatusCode status,
+    const scoped_refptr<ServiceWorkerRegistration>& protect) {
+  if (status != SERVICE_WORKER_OK || is_doomed()) {
     RunSoon(base::Bind(callback, SERVICE_WORKER_ERROR_START_WORKER_FAILED));
     return;
   }
+
   switch (running_status()) {
     case RUNNING:
       RunSoon(base::Bind(callback, SERVICE_WORKER_OK));
@@ -397,7 +419,8 @@ void ServiceWorkerVersion::StartWorker(
         embedded_worker_->Start(
             version_id_, scope_, script_url_, pause_after_download,
             base::Bind(&ServiceWorkerVersion::OnStartSentAndScriptEvaluated,
-                       weak_factory_.GetWeakPtr()));
+                       weak_factory_.GetWeakPtr(),
+                       protect));

[kinuko, 2015/03/11 14:41:56]

I wonder just keeping this 'protect' in start_callbacks_ (with dummy method)
might be easier? Then you don't need to worry about restarting case..

[michaeln, 2015/03/11 20:18:56]

That sounds pretty good provided it can be made to work with the other
start_callbacks_.push_back() callsite inside of StartTimeoutTimer() which is
also called by SetDevToolsAttached (i don't understand why the timer is started
in that method?).

[nhiroki, 2015/03/12 05:27:11]

Good idea. Modified the CL in this way.
Regarding SetDevToolsAttached, "!timeout_timer_.IsRunning()" means that the
worker has stopped (StopTimeoutTimer called only from OnStopped), so starting
the timer in the method wouldn't make sense.

       }
       return;
   }
@@ -881,17 +904,8 @@ void ServiceWorkerVersion::OnStopped(
   cache_listener_.reset();
 
   // Restart worker if we have any start callbacks and the worker isn't doomed.
-  if (should_restart) {
-    if (embedded_worker_->devtools_attached())
-      ClearTick(&start_time_);
-    else
-      RestartTick(&start_time_);
-    cache_listener_.reset(new ServiceWorkerCacheListener(this, context_));
-    embedded_worker_->Start(
-        version_id_, scope_, script_url_, false /* pause_after_download */,
-        base::Bind(&ServiceWorkerVersion::OnStartSentAndScriptEvaluated,
-                   weak_factory_.GetWeakPtr()));

[nhiroki, 2015/03/11 09:08:08]

kinuko@ or falken@: I'm not really sure whether this block can just be replaced
with StartWorker(). Any thoughts?

[kinuko, 2015/03/11 14:41:57]

It's minor and is probably ok, but it looks just calling StartWorker() slightly
changes the timeout behavior.

[nhiroki, 2015/03/12 05:27:11]

Reverted this because it's no longer necessary to call StartWorker to ensure the
live registration. Instead, start callbacks owns the reference.

-  }
+  if (should_restart)
+    StartWorker(base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
 }
 
 void ServiceWorkerVersion::OnReportException(
@@ -963,6 +977,7 @@ bool ServiceWorkerVersion::OnMessageReceived(const IPC::Message& message) {
 }
 
 void ServiceWorkerVersion::OnStartSentAndScriptEvaluated(
+    scoped_refptr<ServiceWorkerRegistration> protect,
     ServiceWorkerStatusCode status) {
   if (status != SERVICE_WORKER_OK)
     RunCallbacks(this, &start_callbacks_, status);