ServiceWorker: OnSimpleEventFinished could be called after timed out

content/browser/service_worker/embedded_worker_test_helper.h

Index: content/browser/service_worker/embedded_worker_test_helper.h
diff --git a/content/browser/service_worker/embedded_worker_test_helper.h b/content/browser/service_worker/embedded_worker_test_helper.h
index 108327d1f66b3f185b198b5921e54e7cf8d12e97..17eee4dec9e0d8b85ae902172ea83450792afbd3 100644
--- a/content/browser/service_worker/embedded_worker_test_helper.h
+++ b/content/browser/service_worker/embedded_worker_test_helper.h
@@ -154,29 +154,25 @@ class EmbeddedWorkerTestHelper : public IPC::Sender,
   static net::HttpResponseInfo CreateHttpResponseInfo();
 
  protected:
-  // Called when StartWorker, StopWorker and SendMessageToWorker message
-  // is sent to the embedded worker. Override if necessary. By default
-  // they verify given parameters and:
-  // - OnStartWorker calls SimulateWorkerStarted
-  // - OnStopWorker calls SimulateWorkerStoped
-  // - OnSendMessageToWorker calls the message's respective On*Event handler
-  virtual void OnStartWorker(int embedded_worker_id,
-                             int64_t service_worker_version_id,
-                             const GURL& scope,
-                             const GURL& script_url,
-                             bool pause_after_download);
+  // StartWorker IPC handler. This simulates each IPC sent from the renderer and
+  // binds |request| to MockServiceWorkerEventDispatcher by default.
+  virtual void 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);
   virtual void OnResumeAfterDownload(int embedded_worker_id);
-  virtual void OnStopWorker(int embedded_worker_id);
+  // StopWorker IPC handler. This calls StopWorkerCallback by default.

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

The Mojo and IPC mixing is a bit confusing. Can the comments clarify it more? If
I understand correctly, OnStartWorker is an IPC handler (since StartWorker is
not mojofied), and OnStopWorker is a Mojo thing. So should this say "IPC
handler"?

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

I think it might be a bit confusing, too...

Currently StartWorker/StopWorker are dispatched through mojo IPC, but the
responses of StartWorker are sent back via the chromium IPC.
In this patch, messages on both IPC systems finally reach to
EmbeddedWorkerTestHelper::OnXXX methods. That's why I thought OnStopWorker was
an IPC handler. 

// I'm using a word "IPC" to mention about "mojo IPC" and ("chromium IPC" or
"legacy IPC"), but is it confusing?

[falken, 2017/01/12 04:14:47]

I find it confusing because I think people often talk about the "Mojo" vs "IPC"
implementation instead of "Mojo IPC" vs "legacy IPC". If we always said "Mojo
IPC" or "Chromium/legacy IPC" it'd be fine. I guess if there's no clearer
terminology to use here, and that's what other people in the codebase are doing,
I guess it's OK to use IPC to refer to both. I haven't really heard "Mojo IPC"
before.

I'll leave it up to your judgement.

Also let's choose one of "legacy IPC" or "Chromium IPC" and use it consistently.

[shimazu, 2017/01/12 05:25:20]

Thanks for clear explanation. 
There are several docs comparing "chromium IPC" vs "mojo IPC" actually:
https://www.chromium.org/developers/design-documents/mojo/chrome-ipc-to-mojo-...
and I prefer "chromium IPC" vs "mojo IPC" because mojo is also an IPC system. 

I'll use "legacy IPC" and "mojo IPC" here.

+  virtual void OnStopWorker(
+      const mojom::EmbeddedWorkerInstanceClient::StopWorkerCallback& callback);
+  // The legacy IPC mesasge handler. This passes the messages to their
+  // respective On*Event handler by default.

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

Why is this called "legacy IPC" and the ones above are just "IPC"? Is there a
difference?

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

OnMessageToWorker handles only the legacy IPC's messages while the
Start/StopWorker handles both IPCs' messages (though now chromium IPC is turned
off for the messages).

[falken, 2017/01/12 04:14:47]

OK... Let's fix s/mesasge/message.

[shimazu, 2017/01/12 05:25:20]

Done.

   virtual bool OnMessageToWorker(int thread_id,
                                  int embedded_worker_id,
                                  const IPC::Message& message);
 
-  // Called to setup mojo for a new embedded worker. Override to register
-  // interfaces the worker should expose to the browser.
-  virtual void OnSetupMojo(
-      int thread_id,
-      mojom::ServiceWorkerEventDispatcherRequest dispatcher_request);
-
   // On*Event handlers. Called by the default implementation of
   // OnMessageToWorker when events are sent to the embedded
   // worker. By default they just return success via
@@ -196,8 +192,8 @@ class EmbeddedWorkerTestHelper : public IPC::Sender,
                            int request_id,
                            const PushEventPayload& payload);
 
-  // These functions simulate sending an EmbeddedHostMsg message to the
-  // browser.
+  // These functions simulate sending an EmbeddedHostMsg message through the
+  // legacy IPC system to the browser.
   void SimulateWorkerReadyForInspection(int embedded_worker_id);
   void SimulateWorkerScriptCached(int embedded_worker_id);
   void SimulateWorkerScriptLoaded(int embedded_worker_id);
@@ -213,9 +209,11 @@ class EmbeddedWorkerTestHelper : public IPC::Sender,
   class MockEmbeddedWorkerSetup;
   class MockServiceWorkerEventDispatcher;
 
-  void OnStartWorkerStub(const EmbeddedWorkerStartParams& params);
+  void OnStartWorkerStub(const EmbeddedWorkerStartParams& params,
+                         mojom::ServiceWorkerEventDispatcherRequest request);
   void OnResumeAfterDownloadStub(int embedded_worker_id);
-  void OnStopWorkerStub(int embedded_worker_id);
+  void OnStopWorkerStub(
+      const mojom::EmbeddedWorkerInstanceClient::StopWorkerCallback& callback);
   void OnMessageToWorkerStub(int thread_id,
                              int embedded_worker_id,
                              const IPC::Message& message);