Create ServiceWorkerProviderHost before starting worker

content/browser/service_worker/service_worker_context_unittest.cc

Index: content/browser/service_worker/service_worker_context_unittest.cc
diff --git a/content/browser/service_worker/service_worker_context_unittest.cc b/content/browser/service_worker/service_worker_context_unittest.cc
index ec1e0d4af4fd0528ef12baffad67344960bc237c..b39120b961c1f2bf4294d4410b9105e81d7a6e9e 100644
--- a/content/browser/service_worker/service_worker_context_unittest.cc
+++ b/content/browser/service_worker/service_worker_context_unittest.cc
@@ -21,6 +21,7 @@
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_storage.h"
 #include "content/browser/service_worker/service_worker_test_utils.h"
+#include "content/browser/service_worker/service_worker_version.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"
@@ -178,13 +179,15 @@ class RecordableEmbeddedWorkerInstanceClient
   const std::vector<Message>& events() const { return events_; }
 
  protected:
-  void StartWorker(const EmbeddedWorkerStartParams& params,
-                   mojom::ServiceWorkerEventDispatcherRequest request,
-                   mojom::EmbeddedWorkerInstanceHostAssociatedPtrInfo
-                       instance_host) override {
+  void StartWorker(
+      const EmbeddedWorkerStartParams& params,
+      mojom::ServiceWorkerEventDispatcherRequest request,
+      mojom::EmbeddedWorkerInstanceHostAssociatedPtrInfo instance_host,
+      mojom::ServiceWorkerProviderClientInfoPtr provider_client_info) override {
     events_.push_back(Message::StartWorker);
     EmbeddedWorkerTestHelper::MockEmbeddedWorkerInstanceClient::StartWorker(
-        params, std::move(request), std::move(instance_host));
+        params, std::move(request), std::move(instance_host),
+        std::move(provider_client_info));
   }
 
   void StopWorker() override {
@@ -220,15 +223,15 @@ TEST_F(ServiceWorkerContextTest, Register) {
   EXPECT_TRUE(called);
 
   ASSERT_EQ(2UL, helper_->dispatched_events()->size());
-  ASSERT_EQ(2UL, client->events().size());
+  ASSERT_EQ(1UL, client->events().size());
   EXPECT_EQ(RecordableEmbeddedWorkerInstanceClient::Message::StartWorker,
             client->events()[0]);
   EXPECT_EQ(EmbeddedWorkerTestHelper::Event::Install,
             helper_->dispatched_events()->at(0));
   EXPECT_EQ(EmbeddedWorkerTestHelper::Event::Activate,
             helper_->dispatched_events()->at(1));
-  EXPECT_EQ(RecordableEmbeddedWorkerInstanceClient::Message::StopWorker,
-            client->events()[1]);
+  // StopWorker should not be recorded because hosting ProviderHost has a
+  // reference to the ServiceWorkerVersion.

[falken, 2017/06/06 14:06:33]

This comment is nice for understanding the change in the test, but I'm not sure
it'll be useful when reading the code for an explanation of why something you
didn't expect didn't happen.

The original test called StopWorker because no one had a reference to
ServiceWorkerVersion so it got destructed and called Stop()?

That's kind of weird, I would have thought the ServiceWorkerRegistration held a
reference to the SWVersion?

[shimazu, 2017/06/12 06:08:12]

Thanks, I agree that this comment doesn't describe the behavior itself. Removed
the comments.
Yes, I think releasing SWVersion triggers Stop().
ServiceWorkerRegisterJob tentatively has the reference to the registration which
are about to be registered, but it would be released at the end of the job. The
reference to registration is usually kept by  SWRegistrationHandle which is
created on the association, but previously it's not created on this test. 
After browser-side creation of SWPH, the provider is created before the service
worker starts. The relevant SWRegistrationHandle and SWHandles are created at
the time.

 
   EXPECT_NE(kInvalidServiceWorkerRegistrationId, registration_id);
 
@@ -325,15 +328,15 @@ TEST_F(ServiceWorkerContextTest, Register_RejectActivate) {
   EXPECT_TRUE(called);
 
   ASSERT_EQ(2UL, helper_->dispatched_events()->size());
-  ASSERT_EQ(2UL, client->events().size());
+  ASSERT_EQ(1UL, client->events().size());
   EXPECT_EQ(RecordableEmbeddedWorkerInstanceClient::Message::StartWorker,
             client->events()[0]);
   EXPECT_EQ(EmbeddedWorkerTestHelper::Event::Install,
             helper_->dispatched_events()->at(0));
   EXPECT_EQ(EmbeddedWorkerTestHelper::Event::Activate,
             helper_->dispatched_events()->at(1));
-  EXPECT_EQ(RecordableEmbeddedWorkerInstanceClient::Message::StopWorker,
-            client->events()[1]);
+  // Rejection of activate event should not triggers the shutdown of the service
+  // worker.

[falken, 2017/06/06 14:06:33]

ditto: not sure this comment is worth leaving in the codebase

Why was this previously causing the worker to stop? The registration isn't
holding a reference to the version?

[shimazu, 2017/06/12 06:08:12]

Done.

 
   EXPECT_NE(kInvalidServiceWorkerRegistrationId, registration_id);
 
@@ -609,13 +612,25 @@ TEST_F(ServiceWorkerContextTest, ProviderHostIterator) {
           context()->AsWeakPtr(), &remote_endpoints.back());
   host3->SetDocumentUrl(kOrigin1);
 
-  // Host4 (provider_id=4): process_id=2, origin2, for ServiceWorker.
+  // Host4 (provider_id<-1): process_id=2, origin2, for ServiceWorker.
+  // The provider_id is not fixed because it's unique beyond tests.

[falken, 2017/06/06 14:06:33]

"unique beyond tests" means that other tests influence the value?

[shimazu, 2017/06/12 06:08:12]

Yes, the provider id is unique because it's saved as static. Should it be
initialized when SWContextCore is initialized, for example?

+  scoped_refptr<ServiceWorkerRegistration> registration =
+      new ServiceWorkerRegistration(GURL("http://www.example.com/test/"),
+                                    1L /* registration_id */,
+                                    helper_->context()->AsWeakPtr());
+  scoped_refptr<ServiceWorkerVersion> version = new ServiceWorkerVersion(
+      registration.get(), GURL("http://www.example.com/test/script_url"),
+      1L /* version_id */, helper_->context()->AsWeakPtr());
+  helper_->SimulateAddProcessToPattern(
+      GURL("http://www.example.com/test/script_url"), kRenderProcessId2);
   remote_endpoints.emplace_back();
   std::unique_ptr<ServiceWorkerProviderHost> host4 =
       CreateProviderHostForServiceWorkerContext(
-          kRenderProcessId2, provider_id++, true /* is_parent_frame_secure */,
+          kRenderProcessId2, true /* is_parent_frame_secure */, version.get(),
           context()->AsWeakPtr(), &remote_endpoints.back());
   host4->SetDocumentUrl(kOrigin2);
+  const int host4_provider_id = host4->provider_id();
+  EXPECT_LT(host4_provider_id, kInvalidServiceWorkerProviderId);
 
   ServiceWorkerProviderHost* host1_raw = host1.get();
   ServiceWorkerProviderHost* host2_raw = host2.get();
@@ -661,7 +676,7 @@ TEST_F(ServiceWorkerContextTest, ProviderHostIterator) {
   context()->RemoveProviderHost(kRenderProcessId1, 1);
   context()->RemoveProviderHost(kRenderProcessId2, 2);
   context()->RemoveProviderHost(kRenderProcessId2, 3);
-  context()->RemoveProviderHost(kRenderProcessId2, 4);
+  context()->RemoveProviderHost(kRenderProcessId2, host4_provider_id);
 }
 
 class ServiceWorkerContextRecoveryTest
@@ -708,9 +723,10 @@ TEST_P(ServiceWorkerContextRecoveryTest, DeleteAndStartOver) {
                  true /* expect_active */));
   base::RunLoop().RunUntilIdle();
 
-  // Next handle ids should be 0 (the next call should return 1).
-  EXPECT_EQ(0, context()->GetNewServiceWorkerHandleId());
-  EXPECT_EQ(0, context()->GetNewRegistrationHandleId());
+  // Next handle ids should be 1 (the next call should return 2) because
+  // registered worker should have taken no 0.

[falken, 2017/06/06 14:06:33]

"no 0" means "number 0"? Recommend ID 0.

[shimazu, 2017/06/12 06:08:12]

Done.

+  EXPECT_EQ(1, context()->GetNewServiceWorkerHandleId());
+  EXPECT_EQ(1, context()->GetNewRegistrationHandleId());
 
   context()->ScheduleDeleteAndStartOver();
 
@@ -753,9 +769,10 @@ TEST_P(ServiceWorkerContextRecoveryTest, DeleteAndStartOver) {
                  true /* expect_active */));
   base::RunLoop().RunUntilIdle();
 
-  // The new context should take over next handle ids.
-  EXPECT_EQ(1, context()->GetNewServiceWorkerHandleId());
-  EXPECT_EQ(1, context()->GetNewRegistrationHandleId());
+  // The new context should take over next handle ids. No 2 should have been

[falken, 2017/06/06 14:06:33]

s/No/Number or ID

(No sounds like the opposite of "yes")

[shimazu, 2017/06/12 06:08:12]

Thanks. I felt the same when I read the comment again..

+  // taken by the running registration, so the following method calls return 3.
+  EXPECT_EQ(3, context()->GetNewServiceWorkerHandleId());
+  EXPECT_EQ(3, context()->GetNewRegistrationHandleId());
 
   ASSERT_EQ(3u, notifications_.size());
   EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);