ServiceWorker: Ensure live registration during starting worker

content/browser/service_worker/service_worker_version.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/browser/service_worker/service_worker_version.h"
 
 #include "base/command_line.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
@@ skipping 359 matching lines @@
       embedded_worker()->worker_devtools_agent_route_id());
 }
 
 void ServiceWorkerVersion::StartWorker(const StatusCallback& callback) {
   StartWorker(false, 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));
       return;
     case STOPPING:
     case STOPPED:
     case STARTING:
       if (!timeout_timer_.IsRunning())
         StartTimeoutTimer();
       start_callbacks_.push_back(callback);
       if (running_status() == STOPPED) {
         DCHECK(!cache_listener_.get());
         cache_listener_.reset(new ServiceWorkerCacheListener(this, context_));
         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;
   }
 }
 
 void ServiceWorkerVersion::StopWorker(const StatusCallback& callback) {
   if (running_status() == STOPPED) {
     RunSoon(base::Bind(callback, SERVICE_WORKER_OK));
     return;
   }
@@ skipping 463 matching lines @@
   streaming_url_request_jobs_.clear();
 
   FOR_EACH_OBSERVER(Listener, listeners_, OnRunningStateChanged(this));
 
   // There should be no more communication from/to a stopped worker. Deleting
   // the listener prevents any pending completion callbacks from causing
   // messages to be sent to the stopped worker.
   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(
     const base::string16& error_message,
     int line_number,
     int column_number,
     const GURL& source_url) {
   FOR_EACH_OBSERVER(
       Listener,
       listeners_,
@@ skipping 51 matching lines @@
                         OnSkipWaiting)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_ClaimClients,
                         OnClaimClients)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_Pong, OnPongFromWorker)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void ServiceWorkerVersion::OnStartSentAndScriptEvaluated(
+    scoped_refptr<ServiceWorkerRegistration> protect,
     ServiceWorkerStatusCode status) {
   if (status != SERVICE_WORKER_OK)
     RunCallbacks(this, &start_callbacks_, status);
 }
 
 void ServiceWorkerVersion::DispatchInstallEventAfterStartWorker(
     const StatusCallback& callback) {
   DCHECK_EQ(RUNNING, running_status())
       << "Worker stopped too soon after it was started.";
 
@@ skipping 639 matching lines @@
     int request_id) {
   callbacks->Remove(request_id);
   if (is_doomed_) {
     // The stop should be already scheduled, but try to stop immediately, in
     // order to release worker resources soon.
     StopWorkerIfIdle();
   }
 }
 
 }  // namespace content