Keep alive ServiceWorkers when devtools is attached

content/browser/service_worker/embedded_worker_instance.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/embedded_worker_instance.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind_helpers.h"
@@ skipping 39 matching lines @@
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(NotifyWorkerDestroyed, worker_process_id, worker_route_id));
     return;
   }
   EmbeddedWorkerDevToolsManager::GetInstance()->WorkerDestroyed(
       worker_process_id, worker_route_id);
 }
 
+void NotifyWorkerStopIgnored(int worker_process_id, int worker_route_id) {
+  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {

[pfeldman, 2014/12/15 05:57:04]

This implies it can be called multiple threads, but the call site touches local
variable.

[pfeldman, 2014/12/15 06:11:08]

I can see you are following a pattern here, but the pattern is unhealthy. I can
also see this code under shared_worker.

[kinuko, 2014/12/15 06:39:14]

This pattern is used pretty widely in content/browser. Why do you think it's
unhealthy? (We can start discussion on chromium-dev if it really should be
avoided)

(Separately I added an assertion line to make the thread restriction a  bit
clearer)

[pfeldman, 2014/12/15 07:05:04]

Hm, I am surprised how widely it has spread. I think it is Ok where
notifications originate from different threads and the design is complex enough
to let go and fall back to this solution. But in cases where code still allows
tracking the threads situation, one should do so.

[pfeldman, 2014/12/15 07:18:42]

I looked at the usages of the pattern and found out that they do indeed belong
either to non-thread-safe APIs or to internal methods that absolutely need to be
non-thread-safe. We definitely should not be doing it just in case. I can
initiate a chromium-dev discussion.

[kinuko, 2014/12/15 07:41:18]

Is the reason you think so because this pattern gives the wrong impression that
the method is thread-safe while it's not really the case?  I think we can
probably introduce a very explicit helper template if it helps getting rid of
this kind of confusion. Anyways I found your email on chromium-dev (thanks) so
let's see how we conclude.

+    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,
     base::WeakPtr<ServiceWorkerContextCore> service_worker_context_weak,
     int64 service_worker_version_id,
     const GURL& url,
     const base::Callback<void(int worker_devtools_agent_route_id,
                               bool wait_for_debugger)>& callback) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(BrowserThread::UI,
@@ skipping 78 matching lines @@
 
 ServiceWorkerStatusCode EmbeddedWorkerInstance::Stop() {
   DCHECK(status_ == STARTING || status_ == RUNNING);
   ServiceWorkerStatusCode status =
       registry_->StopWorker(process_id_, embedded_worker_id_);
   if (status == SERVICE_WORKER_OK)
     status_ = STOPPING;
   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(
       process_id_,
       new EmbeddedWorkerMsg_ResumeAfterDownload(embedded_worker_id_));
 }
 
 ServiceWorkerStatusCode EmbeddedWorkerInstance::SendMessage(
     const IPC::Message& message) {
   DCHECK_NE(kInvalidEmbeddedWorkerThreadId, thread_id_);
   if (status_ != RUNNING && status_ != STARTING)
     return SERVICE_WORKER_ERROR_IPC_FAILED;
   return registry_->Send(process_id_,
                          new EmbeddedWorkerContextMsg_MessageToWorker(
                              thread_id_, embedded_worker_id_, message));
 }
 
 EmbeddedWorkerInstance::EmbeddedWorkerInstance(
     base::WeakPtr<ServiceWorkerContextCore> context,
     int embedded_worker_id)
     : context_(context),
       registry_(context->embedded_worker_registry()),
       embedded_worker_id_(embedded_worker_id),
       status_(STOPPED),
       process_id_(-1),
       thread_id_(kInvalidEmbeddedWorkerThreadId),
       worker_devtools_agent_route_id_(MSG_ROUTING_NONE),
+      devtools_attached_(false),
       weak_factory_(this) {
 }
 
 // static
 void EmbeddedWorkerInstance::RunProcessAllocated(
     base::WeakPtr<EmbeddedWorkerInstance> instance,
     base::WeakPtr<ServiceWorkerContextCore> context,
     scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params,
     const EmbeddedWorkerInstance::StatusCallback& callback,
     ServiceWorkerStatusCode status,
@@ skipping 153 matching lines @@
 
 void EmbeddedWorkerInstance::AddListener(Listener* listener) {
   listener_list_.AddObserver(listener);
 }
 
 void EmbeddedWorkerInstance::RemoveListener(Listener* listener) {
   listener_list_.RemoveObserver(listener);
 }
 
 }  // namespace content