ServiceWorker: Release a reference when it fails to dispatch ExtendableMessageEvent

content/browser/service_worker/service_worker_dispatcher_host_unittest.cc

Index: content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
diff --git a/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc b/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
index 84e1b6e0a35c49efd0b425b388fa04730bcfd8b6..f0cb6740a467cfcc8faf2e83e81c5454f2494f98 100644
--- a/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
+++ b/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
@@ -11,11 +11,13 @@
 #include "base/files/file_path.h"
 #include "base/run_loop.h"
 #include "content/browser/browser_thread_impl.h"
+#include "content/browser/message_port_service.h"
 #include "content/browser/service_worker/embedded_worker_instance.h"
 #include "content/browser/service_worker/embedded_worker_registry.h"
 #include "content/browser/service_worker/embedded_worker_test_helper.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
+#include "content/browser/service_worker/service_worker_handle.h"
 #include "content/common/service_worker/embedded_worker_messages.h"
 #include "content/common/service_worker/service_worker_messages.h"
 #include "content/public/common/content_switches.h"
@@ -35,8 +37,17 @@ static void SaveStatusCallback(bool* called,
   *out = status;
 }
 
+void SetUpDummyMessagePort(std::vector<TransferredMessagePort>* ports) {
+  int port_id = -1;
+  MessagePortService::GetInstance()->Create(MSG_ROUTING_NONE, nullptr,
+                                            &port_id);
+  TransferredMessagePort dummy_port;
+  dummy_port.id = port_id;
+  ports->push_back(dummy_port);
 }
 
+}  // namespace
+
 static const int kRenderFrameId = 1;
 
 class TestingServiceWorkerDispatcherHost : public ServiceWorkerDispatcherHost {
@@ -65,16 +76,27 @@ class TestingServiceWorkerDispatcherHost : public ServiceWorkerDispatcherHost {
   ~TestingServiceWorkerDispatcherHost() override {}
 };
 
+class FailToStartWorkerTestHelper : public EmbeddedWorkerTestHelper {
+ public:
+  FailToStartWorkerTestHelper() : EmbeddedWorkerTestHelper(base::FilePath()) {}
+
+  void OnStartWorker(int embedded_worker_id,
+                     int64_t service_worker_version_id,
+                     const GURL& scope,
+                     const GURL& script_url,
+                     bool pause_after_download) override {
+    EmbeddedWorkerInstance* worker = registry()->GetWorker(embedded_worker_id);
+    registry()->OnWorkerStopped(worker->process_id(), embedded_worker_id);
+  }
+};
+
 class ServiceWorkerDispatcherHostTest : public testing::Test {
  protected:
   ServiceWorkerDispatcherHostTest()
       : browser_thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP) {}
 
   void SetUp() override {
-    helper_.reset(new EmbeddedWorkerTestHelper(base::FilePath()));
-    dispatcher_host_ = new TestingServiceWorkerDispatcherHost(
-        helper_->mock_render_process_id(), context_wrapper(),
-        &resource_context_, helper_.get());
+    Initialize(make_scoped_ptr(new EmbeddedWorkerTestHelper(base::FilePath())));
   }
 
   void TearDown() override { helper_.reset(); }
@@ -84,6 +106,13 @@ class ServiceWorkerDispatcherHostTest : public testing::Test {
     return helper_->context_wrapper();
   }
 
+  void Initialize(scoped_ptr<EmbeddedWorkerTestHelper> helper) {
+    helper_.reset(helper.release());
+    dispatcher_host_ = new TestingServiceWorkerDispatcherHost(
+        helper_->mock_render_process_id(), context_wrapper(),
+        &resource_context_, helper_.get());
+  }
+
   void SendRegister(int64_t provider_id, GURL pattern, GURL worker_url) {
     dispatcher_host_->OnMessageReceived(
         ServiceWorkerHostMsg_RegisterServiceWorker(
@@ -146,6 +175,18 @@ class ServiceWorkerDispatcherHostTest : public testing::Test {
     dispatcher_host_->ipc_sink()->ClearMessages();
   }
 
+  void DispatchExtendableMessageEvent(
+      scoped_refptr<ServiceWorkerVersion> worker,
+      const base::string16& message,
+      const url::Origin& source_origin,
+      const std::vector<TransferredMessagePort>& sent_message_ports,
+      ServiceWorkerProviderHost* sender_provider_host,
+      const ServiceWorkerDispatcherHost::StatusCallback& callback) {
+    dispatcher_host_->DispatchExtendableMessageEvent(
+        std::move(worker), message, source_origin, sent_message_ports,
+        sender_provider_host, callback);
+  }
+
   ServiceWorkerProviderHost* CreateServiceWorkerProviderHost(int provider_id) {
     return new ServiceWorkerProviderHost(
         helper_->mock_render_process_id(), kRenderFrameId, provider_id,
@@ -153,7 +194,6 @@ class ServiceWorkerDispatcherHostTest : public testing::Test {
         dispatcher_host_.get());
   }
 
-
   TestBrowserThreadBundle browser_thread_bundle_;
   content::MockResourceContext resource_context_;
   scoped_ptr<EmbeddedWorkerTestHelper> helper_;
@@ -596,4 +636,146 @@ TEST_F(ServiceWorkerDispatcherHostTest, CleanupOnRendererCrash) {
   EXPECT_EQ(0, new_dispatcher_host->bad_messages_received_count_);
 }
 
+TEST_F(ServiceWorkerDispatcherHostTest, DispatchExtendableMessageEvent) {
+  GURL pattern = GURL("http://www.example.com/");
+  GURL scope = GURL("http://www.example.com/service_worker.js");

[falken, 2016/03/17 06:44:36]

script_url

[nhiroki, 2016/03/17 07:58:17]

Good eye. I wrote this line while I was thinking about whether I should replace
'pattern' with 'scope'. That's the reason why I made a mistake :p

+  int process_id = helper_->mock_render_process_id();
+
+  // Add a provider for a service worker to send a message.
+  const int64_t kProviderId = 99;
+  dispatcher_host_->OnMessageReceived(ServiceWorkerHostMsg_ProviderCreated(
+      kProviderId, MSG_ROUTING_NONE, SERVICE_WORKER_PROVIDER_FOR_CONTROLLER));
+  helper_->SimulateAddProcessToPattern(pattern, process_id);
+  ServiceWorkerProviderHost* sender_provider_host =
+      context()->GetProviderHost(process_id, kProviderId);
+
+  // Set up a registration and service worker to receive a message.
+  scoped_refptr<ServiceWorkerRegistration> registration(
+      new ServiceWorkerRegistration(pattern, 1L,
+                                    helper_->context()->AsWeakPtr()));
+  scoped_refptr<ServiceWorkerVersion> version(new ServiceWorkerVersion(
+      registration.get(), scope, 1L, helper_->context()->AsWeakPtr()));
+  std::vector<ServiceWorkerDatabase::ResourceRecord> records;
+  records.push_back(
+      ServiceWorkerDatabase::ResourceRecord(10, version->script_url(), 100));
+  version->script_cache_map()->SetResources(records);
+
+  // Make the registration findable via storage functions.
+  helper_->context()->storage()->LazyInitialize(base::Bind(&base::DoNothing));
+  base::RunLoop().RunUntilIdle();
+  bool called = false;
+  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
+  helper_->context()->storage()->StoreRegistration(
+      registration.get(), version.get(),
+      base::Bind(&SaveStatusCallback, &called, &status));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(called);
+  ASSERT_EQ(SERVICE_WORKER_OK, status);

[falken, 2016/03/17 06:44:36]

Maybe it's time to refactor lines 640-673 and the similar ones in
CleanupOnRendererCrash and DispatchExtendableMessageEvent_Fail into a helper
function.

[nhiroki, 2016/03/17 07:58:17]

Done.

+
+  // Set the running hosted version so that we can retrieve a valid service
+  // worker object information for the source attribute of the message event.
+  sender_provider_host->running_hosted_version_ = version;
+
+  // Set aside the initial refcount of the worker handle.
+  sender_provider_host->GetOrCreateServiceWorkerHandle(version.get());
+  ServiceWorkerHandle* sender_worker_handle =
+      dispatcher_host_->FindServiceWorkerHandle(
+          sender_provider_host->provider_id(), version->version_id());
+  const int ref_count = sender_worker_handle->ref_count();
+
+  // Dispatch ExtendableMessageEvent.
+  std::vector<TransferredMessagePort> ports;
+  SetUpDummyMessagePort(&ports);
+  called = false;
+  status = SERVICE_WORKER_ERROR_MAX_VALUE;
+  DispatchExtendableMessageEvent(
+      version, base::string16(), url::Origin(version->scope().GetOrigin()),
+      ports, sender_provider_host,
+      base::Bind(&SaveStatusCallback, &called, &status));
+  for (TransferredMessagePort port : ports)
+    EXPECT_TRUE(MessagePortService::GetInstance()->AreMessagesHeld(port.id));
+  EXPECT_EQ(ref_count + 1, sender_worker_handle->ref_count());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(called);
+  EXPECT_EQ(SERVICE_WORKER_OK, status);
+
+  // Messages should be held until ports are created at the destination.
+  for (TransferredMessagePort port : ports)
+    EXPECT_TRUE(MessagePortService::GetInstance()->AreMessagesHeld(port.id));
+
+  EXPECT_EQ(ref_count + 1, sender_worker_handle->ref_count());
+}
+
+TEST_F(ServiceWorkerDispatcherHostTest, DispatchExtendableMessageEvent_Fail) {
+  Initialize(make_scoped_ptr(new FailToStartWorkerTestHelper));
+
+  GURL pattern = GURL("http://www.example.com/");
+  GURL scope = GURL("http://www.example.com/service_worker.js");

[falken, 2016/03/17 06:44:36]

script_url

[nhiroki, 2016/03/17 07:58:17]

Done.

+  int process_id = helper_->mock_render_process_id();
+
+  // Add a provider for a service worker to send a message.
+  const int64_t kProviderId = 99;
+  dispatcher_host_->OnMessageReceived(ServiceWorkerHostMsg_ProviderCreated(
+      kProviderId, MSG_ROUTING_NONE, SERVICE_WORKER_PROVIDER_FOR_CONTROLLER));
+  helper_->SimulateAddProcessToPattern(pattern, process_id);
+  ServiceWorkerProviderHost* sender_provider_host =
+      context()->GetProviderHost(process_id, kProviderId);
+
+  // Set up a registration and service worker to receive a message.
+  scoped_refptr<ServiceWorkerRegistration> registration(
+      new ServiceWorkerRegistration(pattern, 1L,
+                                    helper_->context()->AsWeakPtr()));
+  scoped_refptr<ServiceWorkerVersion> version(new ServiceWorkerVersion(
+      registration.get(), scope, 1L, helper_->context()->AsWeakPtr()));
+  std::vector<ServiceWorkerDatabase::ResourceRecord> records;
+  records.push_back(
+      ServiceWorkerDatabase::ResourceRecord(10, version->script_url(), 100));
+  version->script_cache_map()->SetResources(records);
+
+  // Make the registration findable via storage functions.
+  helper_->context()->storage()->LazyInitialize(base::Bind(&base::DoNothing));
+  base::RunLoop().RunUntilIdle();
+  bool called = false;
+  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
+  helper_->context()->storage()->StoreRegistration(
+      registration.get(), version.get(),
+      base::Bind(&SaveStatusCallback, &called, &status));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(called);
+  ASSERT_EQ(SERVICE_WORKER_OK, status);
+
+  // Set the running hosted version so that we can retrieve a valid service
+  // worker object information for the source attribute of the message event.
+  sender_provider_host->running_hosted_version_ = version;
+
+  // Set aside the initial refcount of the worker handle.
+  sender_provider_host->GetOrCreateServiceWorkerHandle(version.get());
+  ServiceWorkerHandle* sender_worker_handle =
+      dispatcher_host_->FindServiceWorkerHandle(
+          sender_provider_host->provider_id(), version->version_id());
+  const int ref_count = sender_worker_handle->ref_count();
+
+  // Try to dispatch ExtendableMessageEvent. This should fail to start the
+  // worker and to dispatch the event.
+  std::vector<TransferredMessagePort> ports;
+  SetUpDummyMessagePort(&ports);
+  called = false;
+  status = SERVICE_WORKER_ERROR_MAX_VALUE;
+  DispatchExtendableMessageEvent(
+      version, base::string16(), url::Origin(version->scope().GetOrigin()),
+      ports, sender_provider_host,
+      base::Bind(&SaveStatusCallback, &called, &status));
+  for (TransferredMessagePort port : ports)
+    EXPECT_TRUE(MessagePortService::GetInstance()->AreMessagesHeld(port.id));
+  EXPECT_EQ(ref_count + 1, sender_worker_handle->ref_count());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(called);
+  EXPECT_EQ(SERVICE_WORKER_ERROR_START_WORKER_FAILED, status);
+
+  // The error callback should clean up the ports and handle.
+  for (TransferredMessagePort port : ports)
+    EXPECT_FALSE(MessagePortService::GetInstance()->AreMessagesHeld(port.id));
+  EXPECT_EQ(ref_count, sender_worker_handle->ref_count());
+}
+
 }  // namespace content