Revert of Stop ServiceWorker context when no controllee is associated (and...

Revert of Stop ServiceWorker context when no controllee is associated (and when all refs are dropped) (https://codereview.chromium.org/272183005/)

Reason for revert:
Has memory leaks in
EmbeddedWorkerInstanceTest.InstanceDestroyedBeforeStartFinishes

Original issue's description:
> Stop ServiceWorker context when no controllee is associated (and when all refs are dropped)
> 
> Currently we stop the ServiceWorker thread/context when:
> 1. After all documents that are controlled by the SW are closed, with a delay (5 min)
> 2. All references to the version are dropped (i.e. in dtor)
> 
> In the current impl/codebase a worker is immediately stopped by 2
> in most cases.
> 
> The same stop timer (ScheduleStopWorker()) could be probably called
> each time handling event is finished to stop the worker more 
> aggressively, though this patch doesn't do so.
> 
> BUG=372673
> TEST=tested manually with chrome://serviceworker-internals/
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270971

TBR=michaeln@chromium.org,horo@chromium.org,kinuko@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=372673

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@270976 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/service_worker/embedded_worker_instance.cc

@@ -22,8 +22,6 @@ struct SecondGreater {
 }  // namespace
 
 EmbeddedWorkerInstance::~EmbeddedWorkerInstance() {
-  if (status_ == STARTING || status_ == RUNNING)
-    Stop();
   registry_->RemoveWorker(process_id_, embedded_worker_id_);
 }
 

content/browser/service_worker/embedded_worker_test_helper.cc

@@ -154,8 +154,8 @@ void EmbeddedWorkerTestHelper::SimulateWorkerStarted(
 void EmbeddedWorkerTestHelper::SimulateWorkerStopped(
     int embedded_worker_id) {
   EmbeddedWorkerInstance* worker = registry()->GetWorker(embedded_worker_id);
-  if (worker != NULL)
-    registry()->OnWorkerStopped(worker->process_id(), embedded_worker_id);
+  ASSERT_TRUE(worker != NULL);
+  registry()->OnWorkerStopped(worker->process_id(), embedded_worker_id);
 }
 
 void EmbeddedWorkerTestHelper::SimulateSend(

content/browser/service_worker/service_worker_context_unittest.cc

@@ -13,7 +13,6 @@
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_storage.h"
-#include "content/common/service_worker/embedded_worker_messages.h"
 #include "content/common/service_worker/service_worker_messages.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
@@ -151,15 +150,11 @@ TEST_F(ServiceWorkerContextTest, Register) {
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(called);
 
-  EXPECT_EQ(4UL, helper_->ipc_sink()->message_count());
-  EXPECT_TRUE(helper_->ipc_sink()->GetUniqueMessageMatching(
-      EmbeddedWorkerMsg_StartWorker::ID));
+  EXPECT_EQ(3UL, helper_->ipc_sink()->message_count());
   EXPECT_TRUE(helper_->inner_ipc_sink()->GetUniqueMessageMatching(
       ServiceWorkerMsg_InstallEvent::ID));
   EXPECT_TRUE(helper_->inner_ipc_sink()->GetUniqueMessageMatching(
       ServiceWorkerMsg_ActivateEvent::ID));
-  EXPECT_TRUE(helper_->ipc_sink()->GetUniqueMessageMatching(
-      EmbeddedWorkerMsg_StopWorker::ID));
   EXPECT_NE(kInvalidServiceWorkerRegistrationId, registration_id);
   EXPECT_NE(kInvalidServiceWorkerVersionId, version_id);
 
@@ -194,15 +189,11 @@ TEST_F(ServiceWorkerContextTest, Register_RejectInstall) {
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(called);
 
-  EXPECT_EQ(3UL, helper_->ipc_sink()->message_count());
-  EXPECT_TRUE(helper_->ipc_sink()->GetUniqueMessageMatching(
-      EmbeddedWorkerMsg_StartWorker::ID));
+  EXPECT_EQ(2UL, helper_->ipc_sink()->message_count());
   EXPECT_TRUE(helper_->inner_ipc_sink()->GetUniqueMessageMatching(
       ServiceWorkerMsg_InstallEvent::ID));
   EXPECT_FALSE(helper_->inner_ipc_sink()->GetUniqueMessageMatching(
       ServiceWorkerMsg_ActivateEvent::ID));
-  EXPECT_TRUE(helper_->ipc_sink()->GetUniqueMessageMatching(
-      EmbeddedWorkerMsg_StopWorker::ID));
   EXPECT_NE(kInvalidServiceWorkerRegistrationId, registration_id);
   EXPECT_NE(kInvalidServiceWorkerVersionId, version_id);
 
@@ -237,15 +228,11 @@ TEST_F(ServiceWorkerContextTest, Register_RejectActivate) {
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(called);
 
-  EXPECT_EQ(4UL, helper_->ipc_sink()->message_count());
-  EXPECT_TRUE(helper_->ipc_sink()->GetUniqueMessageMatching(
-      EmbeddedWorkerMsg_StartWorker::ID));
+  EXPECT_EQ(3UL, helper_->ipc_sink()->message_count());
   EXPECT_TRUE(helper_->inner_ipc_sink()->GetUniqueMessageMatching(
       ServiceWorkerMsg_InstallEvent::ID));
   EXPECT_TRUE(helper_->inner_ipc_sink()->GetUniqueMessageMatching(
       ServiceWorkerMsg_ActivateEvent::ID));
-  EXPECT_TRUE(helper_->ipc_sink()->GetUniqueMessageMatching(
-      EmbeddedWorkerMsg_StopWorker::ID));
   EXPECT_NE(kInvalidServiceWorkerRegistrationId, registration_id);
   EXPECT_NE(kInvalidServiceWorkerVersionId, version_id);
 

content/browser/service_worker/service_worker_version.cc

@@ -11,7 +11,6 @@
 #include "content/browser/service_worker/embedded_worker_registry.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_registration.h"
-#include "content/browser/service_worker/service_worker_utils.h"
 #include "content/common/service_worker/service_worker_messages.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
@@ -23,12 +22,6 @@ typedef ServiceWorkerVersion::MessageCallback MessageCallback;
 
 namespace {
 
-// Default delay to stop the worker context after all documents that
-// are associated to the worker are closed.
-// (Note that if all references to the version is dropped the worker
-// is also stopped without delay)
-const int64 kStopWorkerDelay = 30;  // 30 secs.
-
 void RunSoon(const base::Closure& callback) {
   if (!callback.is_null())
     base::MessageLoop::current()->PostTask(FROM_HERE, callback);
@@ -110,10 +103,12 @@ ServiceWorkerVersion::ServiceWorkerVersion(
 }
 
 ServiceWorkerVersion::~ServiceWorkerVersion() {
-  embedded_worker_->RemoveListener(this);
+  if (embedded_worker_) {
+    embedded_worker_->RemoveListener(this);
+    embedded_worker_.reset();
+  }
   if (context_)
     context_->RemoveLiveVersion(version_id_);
-  // EmbeddedWorker's dtor sends StopWorker if it's still running.
 }
 
 void ServiceWorkerVersion::SetStatus(Status status) {
@@ -153,6 +148,7 @@ void ServiceWorkerVersion::StartWorker(const StatusCallback& callback) {
 void ServiceWorkerVersion::StartWorkerWithCandidateProcesses(
     const std::vector<int>& possible_process_ids,
     const StatusCallback& callback) {
+  DCHECK(embedded_worker_);
   switch (running_status()) {
     case RUNNING:
       RunSoon(base::Bind(callback, SERVICE_WORKER_OK));
@@ -177,6 +173,7 @@ void ServiceWorkerVersion::StartWorkerWithCandidateProcesses(
 }
 
 void ServiceWorkerVersion::StopWorker(const StatusCallback& callback) {
+  DCHECK(embedded_worker_);
   if (running_status() == STOPPED) {
     RunSoon(base::Bind(callback, SERVICE_WORKER_OK));
     return;
@@ -193,6 +190,7 @@ void ServiceWorkerVersion::StopWorker(const StatusCallback& callback) {
 
 void ServiceWorkerVersion::SendMessage(
     const IPC::Message& message, const StatusCallback& callback) {
+  DCHECK(embedded_worker_);
   if (running_status() != RUNNING) {
     // Schedule calling this method after starting the worker.
     StartWorker(base::Bind(&RunTaskAfterStartWorker,
@@ -320,8 +318,6 @@ void ServiceWorkerVersion::AddControllee(
   int controllee_id = controllee_by_id_.Add(provider_host);
   controllee_map_[provider_host] = controllee_id;
   AddProcessToWorker(provider_host->process_id());
-  if (stop_worker_timer_.IsRunning())
-    stop_worker_timer_.Stop();
 }
 
 void ServiceWorkerVersion::RemoveControllee(
@@ -331,8 +327,6 @@ void ServiceWorkerVersion::RemoveControllee(
   controllee_by_id_.Remove(found->second);
   controllee_map_.erase(found);
   RemoveProcessFromWorker(provider_host->process_id());
-  if (!HasControllee())
-    ScheduleStopWorker();
   // TODO(kinuko): Fire NoControllees notification when the # of controllees
   // reaches 0, so that a new pending version can be activated (which will
   // deactivate this version).
@@ -571,18 +565,4 @@ void ServiceWorkerVersion::OnPostMessageToDocument(
   provider_host->PostMessage(message, sent_message_port_ids);
 }
 
-void ServiceWorkerVersion::ScheduleStopWorker() {
-  if (running_status() != RUNNING)
-    return;
-  if (stop_worker_timer_.IsRunning()) {
-    stop_worker_timer_.Reset();
-    return;
-  }
-  stop_worker_timer_.Start(
-      FROM_HERE, base::TimeDelta::FromSeconds(kStopWorkerDelay),
-      base::Bind(&ServiceWorkerVersion::StopWorker,
-                 weak_factory_.GetWeakPtr(),
-                 base::Bind(&ServiceWorkerUtils::NoOpStatusCallback)));
-}
-
 }  // namespace content

content/browser/service_worker/service_worker_version.h

@@ -14,7 +14,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
-#include "base/timer/timer.h"
 #include "content/browser/service_worker/embedded_worker_instance.h"
 #include "content/browser/service_worker/service_worker_script_cache_map.h"
 #include "content/common/content_export.h"
@@ -189,7 +188,7 @@ class CONTENT_EXPORT ServiceWorkerVersion
   void AddPendingControllee(ServiceWorkerProviderHost* provider_host);
   void RemovePendingControllee(ServiceWorkerProviderHost* provider_host);
 
-  // Returns if it has controllee.
+  // Returns if it has (non-pending) controllee.
   bool HasControllee() const { return !controllee_map_.empty(); }
 
   // Adds and removes Listeners.
@@ -241,8 +240,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
                                const base::string16& message,
                                const std::vector<int>& sent_message_port_ids);
 
-  void ScheduleStopWorker();
-
   const int64 version_id_;
   int64 registration_id_;
   GURL script_url_;
@@ -264,7 +261,6 @@ class CONTENT_EXPORT ServiceWorkerVersion
   base::WeakPtr<ServiceWorkerContextCore> context_;
   ObserverList<Listener> listeners_;
   ServiceWorkerScriptCacheMap script_cache_map_;
-  base::OneShotTimer<ServiceWorkerVersion> stop_worker_timer_;
 
   base::WeakPtrFactory<ServiceWorkerVersion> weak_factory_;