Keep alive ServiceWorkers when devtools is attached

- Plumbed from EmbeddedWorkerDevToolsAgentHost to
 ServiceWorkerVersion to flip 'DevToolsAttached' flag
- Added EmbeddedWorkerInstance::StopIfIdle() which doesn't stop
 the worker if devtools is attached
- Changed stop-worker-timer code to call StopIfIdle() instead of Stop()
- Added a hook in EmbeddedDevToolsManager that is called when we
 didn't stop the worker (to show an educational console message)

BUG=429582
TEST=EmbeddedWorkerInstanceTest.StopWhenDevToolsAttached

Review URL: https://codereview.chromium.org/761923004

Cr-Commit-Position: refs/heads/master@{#308332}

content/browser/devtools/embedded_worker_devtools_agent_host.cc

@@ -35,8 +35,18 @@ void TerminateServiceWorkerOnIO(
   }
 }
 
+void SetDevToolsAttachedOnIO(
+    base::WeakPtr<ServiceWorkerContextCore> context_weak,
+    int64 version_id,
+    bool attached) {
+  if (ServiceWorkerContextCore* context = context_weak.get()) {
+    if (ServiceWorkerVersion* version = context->GetLiveVersion(version_id))
+      version->SetDevToolsAttached(attached);
+  }
 }
 
+}  // namespace
+
 EmbeddedWorkerDevToolsAgentHost::EmbeddedWorkerDevToolsAgentHost(
     WorkerId worker_id,
     const SharedWorkerInstance& shared_worker)
@@ -125,6 +135,13 @@ void EmbeddedWorkerDevToolsAgentHost::Attach() {
 
 void EmbeddedWorkerDevToolsAgentHost::OnClientAttached() {
   DevToolsAgentHostImpl::NotifyCallbacks(this, true);
+  if (service_worker_) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+        base::Bind(&SetDevToolsAttachedOnIO,
+                   service_worker_->context_weak(),
+                   service_worker_->version_id(),
+                   true));
+  }
 }
 
 void EmbeddedWorkerDevToolsAgentHost::OnClientDetached() {
@@ -135,6 +152,13 @@ void EmbeddedWorkerDevToolsAgentHost::OnClientDetached() {
     state_ = WORKER_UNINSPECTED;
   }
   DevToolsAgentHostImpl::NotifyCallbacks(this, false);
+  if (service_worker_) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+        base::Bind(&SetDevToolsAttachedOnIO,
+                   service_worker_->context_weak(),
+                   service_worker_->version_id(),
+                   false));
+  }
 }
 
 bool EmbeddedWorkerDevToolsAgentHost::OnMessageReceived(

content/browser/devtools/embedded_worker_devtools_manager.cc

@@ -139,6 +139,13 @@ void EmbeddedWorkerDevToolsManager::WorkerDestroyed(int worker_process_id,
   DevToolsManager::GetInstance()->AgentHostChanged(agent_host);
 }
 
+void EmbeddedWorkerDevToolsManager::WorkerStopIgnored(int worker_process_id,
+                                                      int worker_route_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // TODO(pfeldman): Show a console message to tell the user that UA didn't
+  // terminate the worker because devtools is attached.
+}
+
 void EmbeddedWorkerDevToolsManager::WorkerReadyForInspection(
     int worker_process_id,
     int worker_route_id) {

content/browser/devtools/embedded_worker_devtools_manager.h

@@ -75,6 +75,7 @@ class CONTENT_EXPORT EmbeddedWorkerDevToolsManager {
                             const ServiceWorkerIdentifier& service_worker_id);
   void WorkerReadyForInspection(int worker_process_id, int worker_route_id);
   void WorkerDestroyed(int worker_process_id, int worker_route_id);
+  void WorkerStopIgnored(int worker_process_id, int worker_route_id);
 
   void set_debug_service_worker_on_start(bool debug_on_start) {
     debug_service_worker_on_start_ = debug_on_start;

content/browser/service_worker/embedded_worker_instance.cc

@@ -34,6 +34,7 @@ struct SecondGreater {
 void NotifyWorkerReadyForInspection(int worker_process_id,
                                     int worker_route_id) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
     BrowserThread::PostTask(BrowserThread::UI,
                             FROM_HERE,
                             base::Bind(NotifyWorkerReadyForInspection,
@@ -47,6 +48,7 @@ void NotifyWorkerReadyForInspection(int worker_process_id,
 
 void NotifyWorkerDestroyed(int worker_process_id, int worker_route_id) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
@@ -57,6 +59,21 @@ void NotifyWorkerDestroyed(int worker_process_id, int worker_route_id) {
       worker_process_id, worker_route_id);
 }
 
+void NotifyWorkerStopIgnored(int worker_process_id, int worker_route_id) {
+  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    BrowserThread::PostTask(BrowserThread::UI,
+                            FROM_HERE,
+                            base::Bind(NotifyWorkerStopIgnored,
+                                       worker_process_id,
+                                       worker_route_id));
+    return;
+  }
+  EmbeddedWorkerDevToolsManager::GetInstance()->WorkerStopIgnored(
+      worker_process_id, worker_route_id);
+}
+
+
 void RegisterToWorkerDevToolsManager(
     int process_id,
     const ServiceWorkerContextCore* service_worker_context,
@@ -66,6 +83,7 @@ void RegisterToWorkerDevToolsManager(
     const base::Callback<void(int worker_devtools_agent_route_id,
                               bool wait_for_debugger)>& callback) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
     BrowserThread::PostTask(BrowserThread::UI,
                             FROM_HERE,
                             base::Bind(RegisterToWorkerDevToolsManager,
@@ -155,6 +173,14 @@ ServiceWorkerStatusCode EmbeddedWorkerInstance::Stop() {
   return status;
 }
 
+void EmbeddedWorkerInstance::StopIfIdle() {
+  if (devtools_attached_) {
+    NotifyWorkerStopIgnored(process_id_, worker_devtools_agent_route_id_);
+    return;
+  }
+  Stop();
+}
+
 void EmbeddedWorkerInstance::ResumeAfterDownload() {
   DCHECK_EQ(STARTING, status_);
   registry_->Send(
@@ -182,6 +208,7 @@ EmbeddedWorkerInstance::EmbeddedWorkerInstance(
       process_id_(-1),
       thread_id_(kInvalidEmbeddedWorkerThreadId),
       worker_devtools_agent_route_id_(MSG_ROUTING_NONE),
+      devtools_attached_(false),
       weak_factory_(this) {
 }
 

content/browser/service_worker/embedded_worker_instance.h

@@ -86,6 +86,11 @@ class CONTENT_EXPORT EmbeddedWorkerInstance {
   // IPC couldn't be sent to the worker.
   ServiceWorkerStatusCode Stop();
 
+  // Stops the worker if the worker is not being debugged (i.e. devtools is
+  // not attached). This method is called by a stop-worker timer to kill
+  // idle workers.
+  void StopIfIdle();
+
   // Sends |message| to the embedded worker running in the child process.
   // It is invalid to call this while the worker is not in RUNNING status.
   ServiceWorkerStatusCode SendMessage(const IPC::Message& message);
@@ -104,6 +109,8 @@ class CONTENT_EXPORT EmbeddedWorkerInstance {
   void AddListener(Listener* listener);
   void RemoveListener(Listener* listener);
 
+  void set_devtools_attached(bool attached) { devtools_attached_ = attached; }
+
  private:
   typedef ObserverList<Listener> ListenerList;
 
@@ -192,6 +199,9 @@ class CONTENT_EXPORT EmbeddedWorkerInstance {
   int thread_id_;
   int worker_devtools_agent_route_id_;
 
+  // Whether devtools is attached or not.
+  bool devtools_attached_;
+
   StatusCallback start_callback_;
 
   ListenerList listener_list_;

content/browser/service_worker/embedded_worker_instance_unittest.cc

@@ -17,7 +17,18 @@
 
 namespace content {
 
-static const int kRenderProcessId = 11;
+namespace {
+
+const int kRenderProcessId = 11;
+
+void SaveStatusAndCall(ServiceWorkerStatusCode* out,
+                       const base::Closure& callback,
+                       ServiceWorkerStatusCode status) {
+  *out = status;
+  callback.Run();
+}
+
+}  // namespace
 
 class EmbeddedWorkerInstanceTest : public testing::Test {
  protected:
@@ -30,6 +41,18 @@ class EmbeddedWorkerInstanceTest : public testing::Test {
 
   void TearDown() override { helper_.reset(); }
 
+  ServiceWorkerStatusCode StartWorker(EmbeddedWorkerInstance* worker,
+                                      int id, const GURL& pattern,
+                                      const GURL& url) {
+    ServiceWorkerStatusCode status;
+    base::RunLoop run_loop;
+    worker->Start(id, pattern, url, false,
+                  base::Bind(&SaveStatusAndCall, &status,
+                             run_loop.QuitClosure()));
+    run_loop.Run();
+    return status;
+  }
+
   ServiceWorkerContextCore* context() { return helper_->context(); }
 
   EmbeddedWorkerRegistry* embedded_worker_registry() {
@@ -46,13 +69,6 @@ class EmbeddedWorkerInstanceTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(EmbeddedWorkerInstanceTest);
 };
 
-static void SaveStatusAndCall(ServiceWorkerStatusCode* out,
-                              const base::Closure& callback,
-                              ServiceWorkerStatusCode status) {
-  *out = status;
-  callback.Run();
-}
-
 TEST_F(EmbeddedWorkerInstanceTest, StartAndStop) {
   scoped_ptr<EmbeddedWorkerInstance> worker =
       embedded_worker_registry()->CreateWorker();
@@ -99,6 +115,50 @@ TEST_F(EmbeddedWorkerInstanceTest, StartAndStop) {
       EmbeddedWorkerMsg_StopWorker::ID));
 }
 
+TEST_F(EmbeddedWorkerInstanceTest, StopWhenDevToolsAttached) {
+  scoped_ptr<EmbeddedWorkerInstance> worker =
+      embedded_worker_registry()->CreateWorker();
+  EXPECT_EQ(EmbeddedWorkerInstance::STOPPED, worker->status());
+
+  const int64 service_worker_version_id = 55L;
+  const GURL pattern("http://example.com/");
+  const GURL url("http://example.com/worker.js");
+
+  // Simulate adding one process to the pattern.
+  helper_->SimulateAddProcessToPattern(pattern, kRenderProcessId);
+
+  // Start the worker and then call StopIfIdle().
+  EXPECT_EQ(SERVICE_WORKER_OK,
+            StartWorker(worker.get(), service_worker_version_id, pattern, url));
+  EXPECT_EQ(EmbeddedWorkerInstance::RUNNING, worker->status());
+  EXPECT_EQ(kRenderProcessId, worker->process_id());
+  worker->StopIfIdle();
+  EXPECT_EQ(EmbeddedWorkerInstance::STOPPING, worker->status());
+  base::RunLoop().RunUntilIdle();
+
+  // The worker must be stopped now.
+  EXPECT_EQ(EmbeddedWorkerInstance::STOPPED, worker->status());
+
+  // Set devtools_attached to true, and do the same.
+  worker->set_devtools_attached(true);
+
+  EXPECT_EQ(SERVICE_WORKER_OK,
+            StartWorker(worker.get(), service_worker_version_id, pattern, url));
+  EXPECT_EQ(EmbeddedWorkerInstance::RUNNING, worker->status());
+  EXPECT_EQ(kRenderProcessId, worker->process_id());
+  worker->StopIfIdle();
+  base::RunLoop().RunUntilIdle();
+
+  // The worker must not be stopped this time.
+  EXPECT_EQ(EmbeddedWorkerInstance::RUNNING, worker->status());
+
+  // Calling Stop() actually stops the worker regardless of whether devtools
+  // is attached or not.
+  EXPECT_EQ(SERVICE_WORKER_OK, worker->Stop());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(EmbeddedWorkerInstance::STOPPED, worker->status());
+}
+
 TEST_F(EmbeddedWorkerInstanceTest, InstanceDestroyedBeforeStartFinishes) {
   scoped_ptr<EmbeddedWorkerInstance> worker =
       embedded_worker_registry()->CreateWorker();

content/browser/service_worker/service_worker_version.cc

@@ -193,10 +193,6 @@ void ServiceWorkerVersion::SetStatus(Status status) {
   if (status_ == status)
     return;
 
-  // Schedule to stop worker after registration successfully completed.
-  if (status_ == ACTIVATING && status == ACTIVATED && !HasControllee())
-    ScheduleStopWorker();
-
   status_ = status;
 
   if (skip_waiting_ && status_ == ACTIVATED) {
@@ -599,11 +595,19 @@ void ServiceWorkerVersion::Doom() {
     DoomInternal();
 }
 
+void ServiceWorkerVersion::SetDevToolsAttached(bool attached) {
+  embedded_worker()->set_devtools_attached(attached);
+  if (!attached && !stop_worker_timer_.IsRunning()) {
+    // If devtools is detached from this version and stop-worker-timer is not
+    // running, try scheduling stop-worker-timer now.
+    ScheduleStopWorker();
+  }
+}
+
 void ServiceWorkerVersion::OnStarted() {
   DCHECK_EQ(RUNNING, running_status());
   DCHECK(cache_listener_.get());
-  if (status() == ACTIVATED && !HasControllee())
-    ScheduleStopWorker();
+  ScheduleStopWorker();
   // Fire all start callbacks.
   RunCallbacks(this, &start_callbacks_, SERVICE_WORKER_OK);
   FOR_EACH_OBSERVER(Listener, listeners_, OnWorkerStarted(this));
@@ -1003,7 +1007,9 @@ void ServiceWorkerVersion::DidSkipWaiting(int request_id) {
 }
 
 void ServiceWorkerVersion::ScheduleStopWorker() {
-  if (running_status() != RUNNING)
+  // TODO(kinuko): Currently we don't schedule stop-time-worker when the SW has
+  // controllee, but we should change this default behavior (crbug.com/440259)
+  if (running_status() != RUNNING || HasControllee())
     return;
   if (stop_worker_timer_.IsRunning()) {
     stop_worker_timer_.Reset();
@@ -1011,9 +1017,32 @@ void ServiceWorkerVersion::ScheduleStopWorker() {
   }
   stop_worker_timer_.Start(
       FROM_HERE, base::TimeDelta::FromSeconds(kStopWorkerDelay),
-      base::Bind(&ServiceWorkerVersion::StopWorker,
-                 weak_factory_.GetWeakPtr(),
-                 base::Bind(&ServiceWorkerUtils::NoOpStatusCallback)));
+      base::Bind(&ServiceWorkerVersion::StopWorkerIfIdle,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void ServiceWorkerVersion::StopWorkerIfIdle() {
+  // Reschedule the stop the worker while there're inflight requests.
+  // (Note: we'll probably need to revisit this so that we can kill 'bad' SW.
+  // See https://github.com/slightlyoff/ServiceWorker/issues/527)
+  if (HasInflightRequests()) {
+    ScheduleStopWorker();
+    return;
+  }
+  if (running_status() == STOPPED || !stop_callbacks_.empty())
+    return;
+  embedded_worker_->StopIfIdle();
+}
+
+bool ServiceWorkerVersion::HasInflightRequests() const {
+  return
+    !activate_callbacks_.IsEmpty() ||
+    !install_callbacks_.IsEmpty() ||
+    !fetch_callbacks_.IsEmpty() ||
+    !sync_callbacks_.IsEmpty() ||
+    !notification_click_callbacks_.IsEmpty() ||
+    !push_callbacks_.IsEmpty() ||
+    !geofencing_callbacks_.IsEmpty();
 }
 
 void ServiceWorkerVersion::DoomInternal() {

content/browser/service_worker/service_worker_version.h

@@ -253,6 +253,8 @@ class CONTENT_EXPORT ServiceWorkerVersion
 
   bool skip_waiting() const { return skip_waiting_; }
 
+  void SetDevToolsAttached(bool attached);
+
  private:
   class GetClientDocumentsCallback;
   class GetClientInfoCallback;
@@ -261,6 +263,7 @@ class CONTENT_EXPORT ServiceWorkerVersion
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerControlleeRequestHandlerTest,
                            ActivateWaitingVersion);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest, ScheduleStopWorker);
+  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest, KeepAlive);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerVersionTest, ListenerAvailability);
   typedef ServiceWorkerVersion self;
   typedef std::map<ServiceWorkerProviderHost*, int> ControlleeMap;
@@ -319,6 +322,9 @@ class CONTENT_EXPORT ServiceWorkerVersion
   void OnFocusClientFinished(int request_id, bool result);
   void DidSkipWaiting(int request_id);
   void ScheduleStopWorker();
+  void StopWorkerIfIdle();
+  bool HasInflightRequests() const;
+
   void DoomInternal();
 
   const int64 version_id_;
@@ -332,7 +338,8 @@ class CONTENT_EXPORT ServiceWorkerVersion
   std::vector<StatusCallback> stop_callbacks_;
   std::vector<base::Closure> status_change_callbacks_;
 
-  // Message callbacks.
+  // Message callbacks. (Update HasInflightRequests() too when you update this
+  // list.)
   IDMap<StatusCallback, IDMapOwnPointer> activate_callbacks_;
   IDMap<StatusCallback, IDMapOwnPointer> install_callbacks_;
   IDMap<FetchCallback, IDMapOwnPointer> fetch_callbacks_;

content/browser/service_worker/service_worker_version_unittest.cc

@@ -363,14 +363,11 @@ TEST_F(ServiceWorkerVersionTest, ScheduleStopWorker) {
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
   EXPECT_FALSE(version_->stop_worker_timer_.IsRunning());
 
-  // Verify the timer is running when version status changes frome ACTIVATING
-  // to ACTIVATED.
+  // Verify the timer is running after the worker is started.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
   version_->StartWorker(CreateReceiverOnCurrentThread(&status));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(SERVICE_WORKER_OK, status);
-  version_->SetStatus(ServiceWorkerVersion::ACTIVATING);
-  version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
   EXPECT_TRUE(version_->stop_worker_timer_.IsRunning());
 
   // The timer should be running if the worker is restarted without controllee.