ServiceWorker: OnSimpleEventFinished could be called after timed out

content/browser/service_worker/embedded_worker_test_helper.cc

Index: content/browser/service_worker/embedded_worker_test_helper.cc
diff --git a/content/browser/service_worker/embedded_worker_test_helper.cc b/content/browser/service_worker/embedded_worker_test_helper.cc
index b2f802133e3304c61d69a9c05e8eebff7d25e177..a3b254a42d41611ccea0738ccc1e7cb4d95a5d8a 100644
--- a/content/browser/service_worker/embedded_worker_test_helper.cc
+++ b/content/browser/service_worker/embedded_worker_test_helper.cc
@@ -61,26 +61,17 @@ class MockMessagePortMessageFilter : public MessagePortMessageFilter {
 class EmbeddedWorkerTestHelper::MockEmbeddedWorkerSetup
     : public mojom::EmbeddedWorkerSetup {
  public:
-  explicit MockEmbeddedWorkerSetup(
-      const base::WeakPtr<EmbeddedWorkerTestHelper>& helper)
-      : helper_(helper) {}
-
   static void Create(const base::WeakPtr<EmbeddedWorkerTestHelper>& helper,
                      mojom::EmbeddedWorkerSetupRequest request) {
-    mojo::MakeStrongBinding(base::MakeUnique<MockEmbeddedWorkerSetup>(helper),
-                            std::move(request));
+    // TODO(shimazu): Remove this mock after EmbeddedWorkerSetup is removed.
+    NOTREACHED();
   }
 
   void AttachServiceWorkerEventDispatcher(
       int32_t thread_id,
       mojom::ServiceWorkerEventDispatcherRequest request) override {
-    if (!helper_)
-      return;
-    helper_->OnSetupMojo(thread_id, std::move(request));
+    NOTREACHED();
   }
-
- private:
-  base::WeakPtr<EmbeddedWorkerTestHelper> helper_;
 };
 
 EmbeddedWorkerTestHelper::MockEmbeddedWorkerInstanceClient::
@@ -104,15 +95,7 @@ void EmbeddedWorkerTestHelper::MockEmbeddedWorkerInstanceClient::StartWorker(
   ASSERT_TRUE(worker != NULL);
   EXPECT_EQ(EmbeddedWorkerStatus::STARTING, worker->status());
 
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::Bind(&EmbeddedWorkerTestHelper::OnStartWorker, helper_->AsWeakPtr(),
-                 params.embedded_worker_id, params.service_worker_version_id,
-                 params.scope, params.script_url, params.pause_after_download));
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::Bind(&EmbeddedWorkerTestHelper::OnSetupMojo, helper_->AsWeakPtr(),
-                 worker->thread_id(), base::Passed(&dispatcher_request)));
+  helper_->OnStartWorkerStub(params, std::move(dispatcher_request));
 }
 
 void EmbeddedWorkerTestHelper::MockEmbeddedWorkerInstanceClient::StopWorker(
@@ -127,7 +110,7 @@ void EmbeddedWorkerTestHelper::MockEmbeddedWorkerInstanceClient::StopWorker(
   // is removed right after sending StopWorker.
   if (worker)
     EXPECT_EQ(EmbeddedWorkerStatus::STOPPING, worker->status());
-  callback.Run();
+  helper_->OnStopWorkerStub(callback);
 }
 
 // static
@@ -256,10 +239,8 @@ bool EmbeddedWorkerTestHelper::Send(IPC::Message* message) {
 bool EmbeddedWorkerTestHelper::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(EmbeddedWorkerTestHelper, message)
-    IPC_MESSAGE_HANDLER(EmbeddedWorkerMsg_StartWorker, OnStartWorkerStub)
     IPC_MESSAGE_HANDLER(EmbeddedWorkerMsg_ResumeAfterDownload,
                         OnResumeAfterDownloadStub)
-    IPC_MESSAGE_HANDLER(EmbeddedWorkerMsg_StopWorker, OnStopWorkerStub)
     IPC_MESSAGE_HANDLER(EmbeddedWorkerContextMsg_MessageToWorker,
                         OnMessageToWorkerStub)
     IPC_MESSAGE_UNHANDLED(handled = false)
@@ -298,11 +279,18 @@ net::HttpResponseInfo EmbeddedWorkerTestHelper::CreateHttpResponseInfo() {
   return info;
 }
 
-void EmbeddedWorkerTestHelper::OnStartWorker(int embedded_worker_id,
-                                             int64_t service_worker_version_id,
-                                             const GURL& scope,
-                                             const GURL& script_url,
-                                             bool pause_after_download) {
+void EmbeddedWorkerTestHelper::OnStartWorker(
+    int embedded_worker_id,
+    int64_t service_worker_version_id,
+    const GURL& scope,
+    const GURL& script_url,
+    bool pause_after_download,
+    mojom::ServiceWorkerEventDispatcherRequest request) {
+  EmbeddedWorkerInstance* worker = registry()->GetWorker(embedded_worker_id);
+  ASSERT_TRUE(worker != nullptr);

[falken, 2017/01/11 06:31:55]

nit: Can drop the != nullptr

[shimazu, 2017/01/12 01:45:24]

Done.

+  MockServiceWorkerEventDispatcher::Create(AsWeakPtr(), worker->thread_id(),
+                                           std::move(request));
+
   embedded_worker_id_service_worker_version_id_map_[embedded_worker_id] =
       service_worker_version_id;
   SimulateWorkerReadyForInspection(embedded_worker_id);
@@ -318,9 +306,9 @@ void EmbeddedWorkerTestHelper::OnResumeAfterDownload(int embedded_worker_id) {
   SimulateWorkerStarted(embedded_worker_id);
 }
 
-void EmbeddedWorkerTestHelper::OnStopWorker(int embedded_worker_id) {
-  // By default just notify the sender that the worker is stopped.
-  SimulateWorkerStopped(embedded_worker_id);
+void EmbeddedWorkerTestHelper::OnStopWorker(
+    const mojom::EmbeddedWorkerInstanceClient::StopWorkerCallback& callback) {

[falken, 2017/01/11 06:31:55]

Can you comment why this does nothing but call the callback?

[shimazu, 2017/01/12 01:45:24]

Calling the callback means sending the response back.

[falken, 2017/01/12 02:29:05]

OK I see now the old SimulateWorkerStopped just sent the response back. I guess
we should have a similar comment.

[shimazu, 2017/01/12 03:48:26]

Done.

+  callback.Run();
 }
 
 bool EmbeddedWorkerTestHelper::OnMessageToWorker(int thread_id,
@@ -339,13 +327,6 @@ bool EmbeddedWorkerTestHelper::OnMessageToWorker(int thread_id,
   return handled;
 }
 
-void EmbeddedWorkerTestHelper::OnSetupMojo(
-    int thread_id,
-    mojom::ServiceWorkerEventDispatcherRequest dispatcher_request) {
-  MockServiceWorkerEventDispatcher::Create(AsWeakPtr(), thread_id,
-                                           std::move(dispatcher_request));
-}
-
 void EmbeddedWorkerTestHelper::OnActivateEvent(int embedded_worker_id,
                                                int request_id) {
   SimulateSend(new ServiceWorkerHostMsg_ActivateEventFinished(
@@ -459,8 +440,8 @@ void EmbeddedWorkerTestHelper::SimulateWorkerStarted(int embedded_worker_id) {
 
 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);

[falken, 2017/01/11 06:31:55]

ASSERT_TRUE(worker) or else change to nullptr

[shimazu, 2017/01/12 01:45:24]

Oops, that's not my intended modification. Restored.

+  registry()->OnWorkerStopped(worker->process_id(), embedded_worker_id);
 }
 
 void EmbeddedWorkerTestHelper::SimulateSend(IPC::Message* message) {
@@ -469,7 +450,8 @@ void EmbeddedWorkerTestHelper::SimulateSend(IPC::Message* message) {
 }
 
 void EmbeddedWorkerTestHelper::OnStartWorkerStub(
-    const EmbeddedWorkerStartParams& params) {
+    const EmbeddedWorkerStartParams& params,
+    mojom::ServiceWorkerEventDispatcherRequest request) {
   EmbeddedWorkerInstance* worker =
       registry()->GetWorker(params.embedded_worker_id);
   ASSERT_TRUE(worker != NULL);
@@ -478,7 +460,8 @@ void EmbeddedWorkerTestHelper::OnStartWorkerStub(
       FROM_HERE,
       base::Bind(&EmbeddedWorkerTestHelper::OnStartWorker, AsWeakPtr(),
                  params.embedded_worker_id, params.service_worker_version_id,
-                 params.scope, params.script_url, params.pause_after_download));
+                 params.scope, params.script_url, params.pause_after_download,
+                 base::Passed(&request)));
 }
 
 void EmbeddedWorkerTestHelper::OnResumeAfterDownloadStub(
@@ -490,12 +473,11 @@ void EmbeddedWorkerTestHelper::OnResumeAfterDownloadStub(
                             AsWeakPtr(), embedded_worker_id));
 }
 
-void EmbeddedWorkerTestHelper::OnStopWorkerStub(int embedded_worker_id) {
-  EmbeddedWorkerInstance* worker = registry()->GetWorker(embedded_worker_id);
-  ASSERT_TRUE(worker != NULL);
+void EmbeddedWorkerTestHelper::OnStopWorkerStub(
+    const mojom::EmbeddedWorkerInstanceClient::StopWorkerCallback& callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&EmbeddedWorkerTestHelper::OnStopWorker,
-                            AsWeakPtr(), embedded_worker_id));
+                            AsWeakPtr(), callback));
 }
 
 void EmbeddedWorkerTestHelper::OnMessageToWorkerStub(