ServiceWorker: Get registration info and its version attributes in one lock operation

Registration info and its version attributes owned by SWProviderContext can be
accessed from both the main thread and the worker thread, so they are protected
by a lock operation. However, getter functions for those info and attributes are
separated and returned values could be in an invalid state due to interleaved
operations.

This CL merges getter functions (registration() and GetVersionAttributes()) into
GetRegistrationInfoAndVersionAttributes() and avoids such an unexpected state.

BUG=437677
TEST=should pass all existing tests

Review URL: https://codereview.chromium.org/885443006

Cr-Commit-Position: refs/heads/master@{#313470}

content/child/service_worker/service_worker_dispatcher.cc

@@ -585,14 +585,16 @@ void ServiceWorkerDispatcher::SetReadyRegistration(
   if (client == provider_clients_.end())
     return;
 
-  ServiceWorkerRegistrationObjectInfo info =
-      provider->second->registration()->info();
+  ServiceWorkerRegistrationObjectInfo info;
+  ServiceWorkerVersionAttributes attrs;
+  bool found =
+      provider->second->GetRegistrationInfoAndVersionAttributes(&info, &attrs);
+  DCHECK(found);
+
   WebServiceWorkerRegistrationImpl* registration =
       FindServiceWorkerRegistration(info, false);
   if (!registration) {
     registration = CreateServiceWorkerRegistration(info, false);
-    ServiceWorkerVersionAttributes attrs =
-        provider->second->GetVersionAttributes();
     registration->SetInstalling(GetServiceWorker(attrs.installing, false));
     registration->SetWaiting(GetServiceWorker(attrs.waiting, false));
     registration->SetActive(GetServiceWorker(attrs.active, false));

content/child/service_worker/service_worker_provider_context.cc

@@ -43,25 +43,21 @@ ServiceWorkerHandleReference* ServiceWorkerProviderContext::controller() {
   return controller_.get();
 }
 
-ServiceWorkerRegistrationHandleReference*
-ServiceWorkerProviderContext::registration() {
+bool ServiceWorkerProviderContext::GetRegistrationInfoAndVersionAttributes(
+    ServiceWorkerRegistrationObjectInfo* info,
+    ServiceWorkerVersionAttributes* attrs) {
   base::AutoLock lock(lock_);
-  return registration_.get();
-}
-
-ServiceWorkerVersionAttributes
-ServiceWorkerProviderContext::GetVersionAttributes() {
-  base::AutoLock lock(lock_);
-  DCHECK(registration_);
+  if (!registration_)
+    return false;
 
-  ServiceWorkerVersionAttributes attrs;
+  *info = registration_->info();
   if (installing_)
-    attrs.installing = installing_->info();
+    attrs->installing = installing_->info();
   if (waiting_)
-    attrs.waiting = waiting_->info();
+    attrs->waiting = waiting_->info();
   if (active_)
-    attrs.active = active_->info();
-  return attrs;
+    attrs->active = active_->info();
+  return true;
 }
 
 void ServiceWorkerProviderContext::SetVersionAttributes(

content/child/service_worker/service_worker_provider_context.h

@@ -48,9 +48,10 @@ class ServiceWorkerProviderContext
   int provider_id() const { return provider_id_; }
 
   ServiceWorkerHandleReference* controller();
-  ServiceWorkerRegistrationHandleReference* registration();
 
-  ServiceWorkerVersionAttributes GetVersionAttributes();
+  bool GetRegistrationInfoAndVersionAttributes(
+      ServiceWorkerRegistrationObjectInfo* info,
+      ServiceWorkerVersionAttributes* attrs);
   void SetVersionAttributes(ChangedVersionAttributesMask mask,
                             const ServiceWorkerVersionAttributes& attrs);
 

content/child/service_worker/web_service_worker_provider_impl.cc

@@ -49,7 +49,9 @@ void WebServiceWorkerProviderImpl::setClient(
   // for more context)
   GetDispatcher()->AddProviderClient(provider_id_, client);
 
-  if (!context_->registration()) {
+  ServiceWorkerRegistrationObjectInfo info;
+  ServiceWorkerVersionAttributes attrs;
+  if (!context_->GetRegistrationInfoAndVersionAttributes(&info, &attrs)) {
     // This provider is not associated with any registration.
     return;
   }
@@ -57,12 +59,10 @@ void WebServiceWorkerProviderImpl::setClient(
   // Set .ready if the associated registration has the active service worker.
   if (context_->active_handle_id() != kInvalidServiceWorkerHandleId) {
     WebServiceWorkerRegistrationImpl* registration =
-        GetDispatcher()->FindServiceWorkerRegistration(
-            context_->registration()->info(), false);
+        GetDispatcher()->FindServiceWorkerRegistration(info, false);
     if (!registration) {
-      registration = GetDispatcher()->CreateServiceWorkerRegistration(
-          context_->registration()->info(), false);
-      ServiceWorkerVersionAttributes attrs = context_->GetVersionAttributes();
+      registration =
+          GetDispatcher()->CreateServiceWorkerRegistration(info, false);
       registration->SetInstalling(
           GetDispatcher()->GetServiceWorker(attrs.installing, false));
       registration->SetWaiting(

content/renderer/service_worker/embedded_worker_context_client.cc

@@ -180,12 +180,7 @@ void EmbeddedWorkerContextClient::workerContextStarted(
   g_worker_client_tls.Pointer()->Set(this);
   script_context_.reset(new ServiceWorkerScriptContext(this, proxy));
 
-  // This can be nullptr on EmbeddedWorkerBrowserTests.
-  // TODO(nhiroki): Remove this workaround. |registration()| should always be
-  // valid other than the test because the registration association message
-  // arrives before starting the worker context.
-  if (provider_context_->registration())
-    SetRegistrationInServiceWorkerGlobalScope();
+  SetRegistrationInServiceWorkerGlobalScope();
 
   Send(new EmbeddedWorkerHostMsg_WorkerScriptLoaded(
       embedded_worker_id_,
@@ -419,19 +414,21 @@ void EmbeddedWorkerContextClient::SetRegistrationInServiceWorkerGlobalScope() {
   DCHECK(provider_context_);
   DCHECK(script_context_);
 
+  ServiceWorkerRegistrationObjectInfo info;
+  ServiceWorkerVersionAttributes attrs;
+  bool found =
+      provider_context_->GetRegistrationInfoAndVersionAttributes(&info, &attrs);
+  if (!found)
+    return;  // Cannot be associated with a registration in some tests.
+
   ServiceWorkerDispatcher* dispatcher =
       ServiceWorkerDispatcher::GetOrCreateThreadSpecificInstance(
           thread_safe_sender());
 
-  // Register a registration with the dispatcher living on the worker thread.
-  DCHECK(provider_context_->registration());
+  // Register a registration and its version attributes with the dispatcher
+  // living on the worker thread.
   scoped_ptr<WebServiceWorkerRegistrationImpl> registration(
-      dispatcher->CreateServiceWorkerRegistration(
-          provider_context_->registration()->info(), false));
-
-  // Register workers with the dispatcher living on the worker thread.
-  ServiceWorkerVersionAttributes attrs =
-      provider_context_->GetVersionAttributes();
+      dispatcher->CreateServiceWorkerRegistration(info, false));
   registration->SetInstalling(
       dispatcher->GetServiceWorker(attrs.installing, false));
   registration->SetWaiting(