Create ServiceWorkerProviderHost before starting worker

content/browser/service_worker/service_worker_context_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/public/browser/service_worker_context.h"
 
 #include <stdint.h>
 
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/time/time.h"
 #include "content/browser/browser_thread_impl.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_observer.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_provider_host.h"
 #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"
 #include "content/public/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 
 namespace {
 
@@ skipping 137 matching lines @@
  public:
   enum class Message { StartWorker, StopWorker };
 
   explicit RecordableEmbeddedWorkerInstanceClient(
       base::WeakPtr<EmbeddedWorkerTestHelper> helper)
       : EmbeddedWorkerTestHelper::MockEmbeddedWorkerInstanceClient(helper) {}
 
   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 {
     events_.push_back(Message::StopWorker);
     EmbeddedWorkerTestHelper::MockEmbeddedWorkerInstanceClient::StopWorker();
   }
 
   std::vector<Message> events_;
 
  private:
@@ skipping 15 matching lines @@
       pattern,
       script_url,
       NULL,
       MakeRegisteredCallback(&called, &registration_id));
 
   ASSERT_FALSE(called);
   base::RunLoop().RunUntilIdle();
   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);
 
   ASSERT_EQ(1u, notifications_.size());
   EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);
   EXPECT_EQ(pattern, notifications_[0].pattern);
   EXPECT_EQ(registration_id, notifications_[0].registration_id);
 
   context()->storage()->FindRegistrationForId(
       registration_id,
@@ skipping 76 matching lines @@
   bool called = false;
   context()->RegisterServiceWorker(
       pattern, script_url, NULL,
       MakeRegisteredCallback(&called, &registration_id));
 
   ASSERT_FALSE(called);
   base::RunLoop().RunUntilIdle();
   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);
 
   ASSERT_EQ(1u, notifications_.size());
   EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);
   EXPECT_EQ(pattern, notifications_[0].pattern);
   EXPECT_EQ(registration_id, notifications_[0].registration_id);
 
   context()->storage()->FindRegistrationForId(
       registration_id, pattern.GetOrigin(),
@@ skipping 255 matching lines @@
   host2->SetDocumentUrl(kOrigin2);
 
   // Host3 (provider_id=3): process_id=2, origin1.
   remote_endpoints.emplace_back();
   std::unique_ptr<ServiceWorkerProviderHost> host3 =
       CreateProviderHostForWindow(
           kRenderProcessId2, provider_id++, true /* is_parent_frame_secure */,
           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();
   ServiceWorkerProviderHost* host3_raw = host3.get();
   ServiceWorkerProviderHost* host4_raw = host4.get();
   context()->AddProviderHost(std::move(host1));
   context()->AddProviderHost(std::move(host2));
   context()->AddProviderHost(std::move(host3));
   context()->AddProviderHost(std::move(host4));
 
@@ skipping 25 matching lines @@
   for (auto it = context()->GetClientProviderHostIterator(kOrigin2);
        !it->IsAtEnd(); it->Advance()) {
     results.insert(it->GetProviderHost());
   }
   EXPECT_EQ(1u, results.size());
   EXPECT_TRUE(ContainsKey(results, host2_raw));
 
   context()->RemoveProviderHost(kRenderProcessId1, 1);
   context()->RemoveProviderHost(kRenderProcessId2, 2);
   context()->RemoveProviderHost(kRenderProcessId2, 3);
-  context()->RemoveProviderHost(kRenderProcessId2, 4);
+  context()->RemoveProviderHost(kRenderProcessId2, host4_provider_id);
 }
 
 class ServiceWorkerContextRecoveryTest
     : public ServiceWorkerContextTest,
       public testing::WithParamInterface<bool> {
  public:
   ServiceWorkerContextRecoveryTest() {}
   virtual ~ServiceWorkerContextRecoveryTest() {}
 
  protected:
@@ skipping 26 matching lines @@
 
   context()->storage()->FindRegistrationForId(
       registration_id,
       pattern.GetOrigin(),
       base::Bind(&ExpectRegisteredWorkers,
                  SERVICE_WORKER_OK,
                  false /* expect_waiting */,
                  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();
 
   // The storage is disabled while the recovery process is running, so the
   // operation should be aborted.
   context()->storage()->FindRegistrationForId(
       registration_id, pattern.GetOrigin(),
       base::Bind(&ExpectRegisteredWorkers, SERVICE_WORKER_ERROR_ABORT,
                  false /* expect_waiting */, true /* expect_active */));
   base::RunLoop().RunUntilIdle();
@@ skipping 22 matching lines @@
 
   context()->storage()->FindRegistrationForId(
       registration_id,
       pattern.GetOrigin(),
       base::Bind(&ExpectRegisteredWorkers,
                  SERVICE_WORKER_OK,
                  false /* expect_waiting */,
                  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);
   EXPECT_EQ(pattern, notifications_[0].pattern);
   EXPECT_EQ(registration_id, notifications_[0].registration_id);
   EXPECT_EQ(STORAGE_RECOVERED, notifications_[1].type);
   EXPECT_EQ(REGISTRATION_STORED, notifications_[2].type);
   EXPECT_EQ(pattern, notifications_[2].pattern);
   EXPECT_EQ(registration_id, notifications_[2].registration_id);
 }
 
 INSTANTIATE_TEST_CASE_P(ServiceWorkerContextRecoveryTest,
                         ServiceWorkerContextRecoveryTest,
                         testing::Bool());
 
 }  // namespace content