Fix crash during EmbeddedWorkerInstance startup sequence failures

Once EWInstance startup calls the callback, it's possible that the
underlying ServiceWorkerVersion is destroyed, hence destroying
|this|. We must guard against that.

Also some failure points in the startup sequence weren't calling
OnStopped() as expected.

BUG=529520, 531345

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

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

3 files changed, 102 insertions, 19 deletions

diff --git a/content/browser/service_worker/embedded_worker_instance.cc b/content/browser/service_worker/embedded_worker_instance.cc
index ef03475..c508e02 100644
--- a/content/browser/service_worker/embedded_worker_instance.cc
+++ b/content/browser/service_worker/embedded_worker_instance.cc
@@ -155,6 +155,7 @@ void EmbeddedWorkerInstance::Start(int64 service_worker_version_id,
                                    const StatusCallback& callback) {
   if (!context_) {
     callback.Run(SERVICE_WORKER_ERROR_ABORT);
+    // |this| may be destroyed by the callback.
     return;
   }
   DCHECK(status_ == STOPPED);
@@ -284,11 +285,7 @@ void EmbeddedWorkerInstance::ProcessAllocated(
                          params.get(),
                          "Status", status);
   if (status != SERVICE_WORKER_OK) {
-    Status old_status = status_;
-    status_ = STOPPED;
-    service_registry_.reset();
-    callback.Run(status);
-    FOR_EACH_OBSERVER(Listener, listener_list_, OnStopped(old_status));
+    OnStartFailed(callback, status);
     return;
   }
   const int64 service_worker_version_id = params->service_worker_version_id;
@@ -318,8 +315,7 @@ void EmbeddedWorkerInstance::SendStartWorker(
   // process, making process_id_ and other state invalid. If that happened,
   // abort instead of trying to send the IPC.
   if (status_ != STARTING) {
-    callback.Run(SERVICE_WORKER_ERROR_ABORT);
-    ReleaseProcess();
+    OnStartFailed(callback, SERVICE_WORKER_ERROR_ABORT);
     return;
   }
 
@@ -354,7 +350,7 @@ void EmbeddedWorkerInstance::SendStartWorker(
   ServiceWorkerStatusCode status =
       registry_->SendStartWorker(params.Pass(), process_id_);
   if (status != SERVICE_WORKER_OK) {
-    callback.Run(status);
+    OnStartFailed(callback, status);
     return;
   }
   DCHECK(start_callback_.is_null());
@@ -414,9 +410,11 @@ void EmbeddedWorkerInstance::OnScriptEvaluated(bool success) {
     UMA_HISTOGRAM_TIMES("EmbeddedWorkerInstance.ScriptEvaluate",
                         base::TimeTicks::Now() - start_timing_);
   }
-  start_callback_.Run(success ? SERVICE_WORKER_OK
-                              : SERVICE_WORKER_ERROR_SCRIPT_EVALUATE_FAILED);
+  StatusCallback callback = start_callback_;
   start_callback_.Reset();
+  callback.Run(success ? SERVICE_WORKER_OK
+                       : SERVICE_WORKER_ERROR_SCRIPT_EVALUATE_FAILED);
+  // |this| may be destroyed by the callback.
 }
 
 void EmbeddedWorkerInstance::OnStarted() {
@@ -508,6 +506,17 @@ void EmbeddedWorkerInstance::ReleaseProcess() {
   start_callback_.Reset();
 }
 
+void EmbeddedWorkerInstance::OnStartFailed(const StatusCallback& callback,
+                                           ServiceWorkerStatusCode status) {
+  Status old_status = status_;
+  ReleaseProcess();
+  base::WeakPtr<EmbeddedWorkerInstance> weak_this = weak_factory_.GetWeakPtr();
+  callback.Run(status);
+  if (weak_this && old_status != STOPPED)
+    FOR_EACH_OBSERVER(Listener, weak_this->listener_list_,
+                      OnStopped(old_status));
+}
+
 // static
 std::string EmbeddedWorkerInstance::StatusToString(Status status) {
   switch (status) {
diff --git a/content/browser/service_worker/embedded_worker_instance.h b/content/browser/service_worker/embedded_worker_instance.h
index edbec81..dc8e599 100644
--- a/content/browser/service_worker/embedded_worker_instance.h
+++ b/content/browser/service_worker/embedded_worker_instance.h
@@ -236,7 +236,13 @@ class CONTENT_EXPORT EmbeddedWorkerInstance {
                               int line_number,
                               const GURL& source_url);
 
+  // Resets all running state. After this function is called, |status_| is
+  // STOPPED.
   void ReleaseProcess();
+  // Called when the startup sequence failed. Calls ReleaseProcess() and invokes
+  // |callback| with |status|. May destroy |this|.
+  void OnStartFailed(const StatusCallback& callback,
+                     ServiceWorkerStatusCode status);
 
   base::WeakPtr<ServiceWorkerContextCore> context_;
   scoped_refptr<EmbeddedWorkerRegistry> registry_;
diff --git a/content/browser/service_worker/embedded_worker_instance_unittest.cc b/content/browser/service_worker/embedded_worker_instance_unittest.cc
index 727fcca..9afe0ee 100644
--- a/content/browser/service_worker/embedded_worker_instance_unittest.cc
+++ b/content/browser/service_worker/embedded_worker_instance_unittest.cc
@@ -21,8 +21,11 @@ namespace {
 
 const int kRenderProcessId = 11;
 
-void SaveStatus(ServiceWorkerStatusCode* out, ServiceWorkerStatusCode status) {
-  *out = status;
+void DestroyWorker(scoped_ptr<EmbeddedWorkerInstance> worker,
+                   ServiceWorkerStatusCode* out_status,
+                   ServiceWorkerStatusCode status) {
+  *out_status = status;
+  worker.reset();
 }
 
 void SaveStatusAndCall(ServiceWorkerStatusCode* out,
@@ -40,6 +43,11 @@ class EmbeddedWorkerInstanceTest : public testing::Test,
   EmbeddedWorkerInstanceTest()
       : thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP) {}
 
+  void OnStopped(EmbeddedWorkerInstance::Status old_status) override {
+    stopped_ = true;
+    stopped_old_status_ = old_status;
+  }
+
   void OnDetached(EmbeddedWorkerInstance::Status old_status) override {
     detached_ = true;
     detached_old_status_ = old_status;
@@ -79,11 +87,26 @@ class EmbeddedWorkerInstanceTest : public testing::Test,
   bool detached_ = false;
   EmbeddedWorkerInstance::Status detached_old_status_ =
       EmbeddedWorkerInstance::STOPPED;
+  bool stopped_ = false;
+  EmbeddedWorkerInstance::Status stopped_old_status_ =
+      EmbeddedWorkerInstance::STOPPED;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(EmbeddedWorkerInstanceTest);
 };
 
+class FailToSendIPCHelper : public EmbeddedWorkerTestHelper {
+ public:
+  FailToSendIPCHelper()
+      : EmbeddedWorkerTestHelper(base::FilePath(), kRenderProcessId) {}
+  ~FailToSendIPCHelper() override {}
+
+  bool Send(IPC::Message* message) override {
+    delete message;
+    return false;
+  }
+};
+
 TEST_F(EmbeddedWorkerInstanceTest, StartAndStop) {
   scoped_ptr<EmbeddedWorkerInstance> worker =
       embedded_worker_registry()->CreateWorker();
@@ -232,14 +255,34 @@ TEST_F(EmbeddedWorkerInstanceTest, RemoveWorkerInSharedProcess) {
 TEST_F(EmbeddedWorkerInstanceTest, DetachDuringStart) {
   scoped_ptr<EmbeddedWorkerInstance> worker =
       embedded_worker_registry()->CreateWorker();
+  worker->AddListener(this);
+
   scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
       new EmbeddedWorkerMsg_StartWorker_Params());
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
-  // Pretend we had a process allocated but then got detached before
-  // the start sequence reached SendStartWorker.
-  worker->process_id_ = -1;
-  worker->SendStartWorker(params.Pass(), base::Bind(&SaveStatus, &status), true,
-                          -1, false);
+
+  // Pretend the worker got stopped before the start sequence reached
+  // SendStartWorker.
+  worker->status_ = EmbeddedWorkerInstance::STOPPED;
+  base::RunLoop run_loop;
+  worker->SendStartWorker(params.Pass(), base::Bind(&SaveStatusAndCall, &status,
+                                                    run_loop.QuitClosure()),
+                          true, -1, false);
+  run_loop.Run();
+  EXPECT_EQ(SERVICE_WORKER_ERROR_ABORT, status);
+  // Don't expect SendStartWorker() to dispatch an OnStopped/Detached() message
+  // since the worker was already stopped.
+  EXPECT_FALSE(stopped_);
+  EXPECT_FALSE(detached_);
+
+  // Repeat, this time have the start callback destroy the worker, as is
+  // usual when starting a worker fails, and ensure a crash doesn't occur.
+  worker->status_ = EmbeddedWorkerInstance::STOPPED;
+  EmbeddedWorkerInstance* worker_ptr = worker.get();
+  worker_ptr->SendStartWorker(
+      params.Pass(), base::Bind(&DestroyWorker, base::Passed(&worker), &status),
+      true, -1, false);
+  // No crash.
   EXPECT_EQ(SERVICE_WORKER_ERROR_ABORT, status);
 }
 
@@ -249,8 +292,6 @@ TEST_F(EmbeddedWorkerInstanceTest, StopDuringStart) {
   scoped_ptr<EmbeddedWorkerInstance> worker =
       embedded_worker_registry()->CreateWorker();
   worker->AddListener(this);
-  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
-      new EmbeddedWorkerMsg_StartWorker_Params());
   // Pretend we stop during starting before we got a process allocated.
   worker->status_ = EmbeddedWorkerInstance::STARTING;
   worker->process_id_ = -1;
@@ -260,4 +301,31 @@ TEST_F(EmbeddedWorkerInstanceTest, StopDuringStart) {
   EXPECT_EQ(EmbeddedWorkerInstance::STARTING, detached_old_status_);
 }
 
+// Test for when sending the start IPC failed.
+TEST_F(EmbeddedWorkerInstanceTest, FailToSendStartIPC) {
+  helper_.reset(new FailToSendIPCHelper());
+
+  const int64 version_id = 55L;
+  const GURL pattern("http://example.com/");
+  const GURL url("http://example.com/worker.js");
+
+  scoped_ptr<EmbeddedWorkerInstance> worker =
+      embedded_worker_registry()->CreateWorker();
+  helper_->SimulateAddProcessToPattern(pattern, kRenderProcessId);
+  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
+  worker->AddListener(this);
+
+  // Attempt to start the worker.
+  base::RunLoop run_loop;
+  worker->Start(
+      version_id, pattern, url,
+      base::Bind(&SaveStatusAndCall, &status, run_loop.QuitClosure()));
+  run_loop.Run();
+
+  // The callback should have run, and we should have got an OnStopped message.
+  EXPECT_EQ(SERVICE_WORKER_ERROR_IPC_FAILED, status);
+  EXPECT_TRUE(stopped_);
+  EXPECT_EQ(EmbeddedWorkerInstance::STARTING, stopped_old_status_);
+}
+
 }  // namespace content