service worker: Slim down semantics of provider host's "associated registration".

This should really just be the registration of the controller. Slim down
some of the meaning so it's more like that.

Bug: 866353
Change-Id: I98a412dd4456e5600125ea7fccb44d6e0e049faf
Reviewed-on: https://chromium-review.googlesource.com/1149439Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577829}

content/browser/service_worker/service_worker_controllee_request_handler.cc

@@ -362,6 +362,7 @@ void ServiceWorkerControlleeRequestHandler::
         url_job_.get(), "Info", "No Provider");
     return;
   }
+  provider_host_->AddMatchingRegistration(registration.get());
 
   if (!context_) {
     url_job_->FallbackToNetwork();
@@ -416,14 +417,6 @@ void ServiceWorkerControlleeRequestHandler::
   scoped_refptr<ServiceWorkerVersion> active_version =
       registration->active_version();
   if (!active_version) {
-    // Although there is no active version, a registration exists, so associate
-    // it, so it can be used for .ready.
-    // TODO(falken): There's no need to associate the registration just for
-    // .ready. Change this to AddMatchingRegistration instead, and call it
-    // unconditionally if there is a |provider_host_|.
-    provider_host_->AssociateRegistration(registration.get(),
-                                          false /* notify_controllerchange */);
-
     url_job_->FallbackToNetwork();
     TRACE_EVENT_ASYNC_END1(
         "ServiceWorker",

content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc

@@ -279,13 +279,14 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, InstallingRegistration) {
   base::RunLoop().RunUntilIdle();
 
   // The handler should have fallen back to network and destroyed the job. The
-  // registration should be associated with the provider host, although it is
-  // not controlled since there is no active version.
+  // registration should not be associated with the provider host, since it is
+  // not controlled. However it should be a matching registration so it can be
+  // used for .ready and claim().
   EXPECT_FALSE(job);
-  EXPECT_EQ(registration_.get(), provider_host_->associated_registration());
-  EXPECT_EQ(version_.get(), provider_host_->installing_version());
+  EXPECT_FALSE(provider_host_->associated_registration());
   EXPECT_FALSE(version_->HasControllee());
   EXPECT_FALSE(provider_host_->controller());
+  EXPECT_EQ(registration_.get(), provider_host_->MatchRegistration());
 }
 
 // Test to not regress crbug/414118.

content/browser/service_worker/service_worker_provider_host.cc

@@ -503,9 +503,10 @@ void ServiceWorkerProviderHost::AssociateRegistration(
   DCHECK(registration);
   DCHECK(!associated_registration_);
   DCHECK(allow_association_);
+  DCHECK(base::ContainsKey(matching_registrations_,
+                           registration->pattern().spec().size()));
 
   associated_registration_ = registration;
-  AddMatchingRegistration(registration);
   SetControllerVersionAttribute(registration->active_version(),
                                 notify_controllerchange);
 }

content/browser/service_worker/service_worker_provider_host.h

@@ -200,12 +200,9 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
 
   // Returns this provider's controller. The controller is typically the same as
   // active_version() but can differ in the following cases:
-  // (1) The client was created before the registration existed or had an active
-  // version (in spec language, it is not "using" the registration).
-  // (2) The client had a controller but NotifyControllerLost() was called due
-  // to an exceptional circumstance (here also it is not "using" the
-  // registration).
-  // (3) During algorithms such as the update, skipWaiting(), and claim() steps,
+  // (1) The client had a controller but NotifyControllerLost() was called due
+  // to an exceptional circumstance.
+  // (2) During algorithms such as the update, skipWaiting(), and claim() steps,
   // the active version and controller may temporarily differ. For example, to
   // perform skipWaiting(), the registration's active version is updated first
   // and then the provider host's controlling version is updated to match it.
@@ -221,20 +218,9 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
                : nullptr;
   }
 
-  ServiceWorkerVersion* waiting_version() const {
-    return associated_registration_.get()
-               ? associated_registration_->waiting_version()
-               : nullptr;
-  }
-
-  ServiceWorkerVersion* installing_version() const {
-    return associated_registration_.get()
-               ? associated_registration_->installing_version()
-               : nullptr;
-  }
-
-  // Returns the associated registration. The provider host listens to this
-  // registration to resolve the .ready promise and set its controller.
+  // Returns the associated registration. This is typically the registration of
+  // the controller() if it exists. It may also exist when controller() is null
+  // after NotifyControllerLost().
   ServiceWorkerRegistration* associated_registration() const {
     // Only clients can have an associated registration.
     DCHECK(!associated_registration_ || IsProviderForClient());
@@ -315,14 +301,13 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   // Can only be called when IsProviderForClient() is true.
   blink::mojom::ServiceWorkerClientType client_type() const;
 
-  // For service worker clients. Associates to |registration| to listen for its
-  // version change events and sets the controller. If |notify_controllerchange|
-  // is true, instructs the renderer to dispatch a 'controllerchange' event.
+  // For service worker clients. Associates to |registration| to set the
+  // controller. If |notify_controllerchange| is true, instructs the renderer to
+  // dispatch a 'controllerchange' event.
   void AssociateRegistration(ServiceWorkerRegistration* registration,
                              bool notify_controllerchange);
 
-  // For service worker clients. Clears the associated registration and stops
-  // listening to it.
+  // For service worker clients. Clears the controller.
   void DisassociateRegistration();
 
   // Returns a handler for a request. May return nullptr if the request doesn't
@@ -415,6 +400,16 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   // After this is called, is_execution_ready() returns true.
   void CompleteSharedWorkerPreparation();
 
+  // For service worker clients. The host keeps track of all the prospective
+  // longest-matching registrations, in order to resolve .ready or respond to
+  // claim() attempts.
+  //
+  // This is subtle: it doesn't keep all registrations (e.g., from storage) in
+  // memory, but just the ones that are possibly the longest-matching one. The
+  // best match from storage is added at load time. That match can't uninstall
+  // while this host is a controllee, so all the other stored registrations can
+  // be ignored. Only a newly installed registration can claim it, and new
+  // installing registrations are added as matches.
   void AddMatchingRegistration(ServiceWorkerRegistration* registration);
   void RemoveMatchingRegistration(ServiceWorkerRegistration* registration);
 
@@ -513,8 +508,6 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   void SetControllerVersionAttribute(ServiceWorkerVersion* version,
                                      bool notify_controllerchange);
 
-  void SendAssociateRegistrationMessage();
-
   // Syncs matching registrations with live registrations.
   void SyncMatchingRegistrations();
 
@@ -614,6 +607,8 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   GURL document_url_;
   GURL topmost_frame_url_;
 
+  // The registration of |controller_|, if it exists. This might also be
+  // set even if |controller_| is null due to NotifyControllerLost.
   scoped_refptr<ServiceWorkerRegistration> associated_registration_;
 
   // Keyed by registration scope URL length.