Keep alive ServiceWorkers when devtools is attached

content/browser/service_worker/embedded_worker_instance.cc

Index: content/browser/service_worker/embedded_worker_instance.cc
diff --git a/content/browser/service_worker/embedded_worker_instance.cc b/content/browser/service_worker/embedded_worker_instance.cc
index bd5c9cfe0f4aaf96f7b24e384ac1b6fb9b375e8b..66e30f59c20ec6514db14b574ed945d41d8436a8 100644
--- a/content/browser/service_worker/embedded_worker_instance.cc
+++ b/content/browser/service_worker/embedded_worker_instance.cc
@@ -57,6 +57,20 @@ 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)) {

[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,
@@ -155,6 +169,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 +204,7 @@ EmbeddedWorkerInstance::EmbeddedWorkerInstance(
       process_id_(-1),
       thread_id_(kInvalidEmbeddedWorkerThreadId),
       worker_devtools_agent_route_id_(MSG_ROUTING_NONE),
+      devtools_attached_(false),
       weak_factory_(this) {
 }