service worker: Wait for inflight requests before activating

content/browser/service_worker/service_worker_registration.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/browser/service_worker/service_worker_registration.h"
 
 #include <vector>
 
 #include "content/browser/service_worker/embedded_worker_status.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
@@ skipping 89 matching lines @@
       pattern(), registration_id_,
       is_deleted_ ? ServiceWorkerRegistrationInfo::IS_DELETED
                   : ServiceWorkerRegistrationInfo::IS_NOT_DELETED,
       GetVersionInfo(active_version_.get()),
       GetVersionInfo(waiting_version_.get()),
       GetVersionInfo(installing_version_.get()), resources_total_size_bytes_);
 }
 
 void ServiceWorkerRegistration::SetActiveVersion(
     const scoped_refptr<ServiceWorkerVersion>& version) {
-  should_activate_when_ready_ = false;
   if (active_version_ == version)

[nhiroki, 2016/07/13 04:29:30]

(This comment is not related to this CL. You don't have to work on this in this
CL)

I suspect this condition is never satisfied. In production, SetActiveVersion()
can be called from ServiceWorkerStorage::GetOrCreateRegistration() or
ServiceWorkerRegistration::ActivateWaitingVersion(). On the former callsite, a
registration is newly created and its |active_version_| should always be
nullptr. On the latter callsite, it would be strange if |activating_version| is
equal to |exiting_version|.

If these are true, we could replace this if-statement with
DCHECK_NE(active_version_, version) etc.

[falken, 2016/07/14 13:52:07]

Acknowledged. (OTOH, it may be reasonable to write complex setters like this to
be idempotent, even if current code doesn't use them that way.)

     return;
 
+  should_activate_when_ready_ = false;
+
   ChangedVersionAttributesMask mask;
   if (version)
     UnsetVersionInternal(version.get(), &mask);
   if (active_version_)
     active_version_->RemoveListener(this);
   active_version_ = version;
   if (active_version_)
     active_version_->AddListener(this);
   mask.add(ChangedVersionAttributesMask::ACTIVE_VERSION);
 
   NotifyVersionAttributesChanged(mask);
 }
 
 void ServiceWorkerRegistration::SetWaitingVersion(
     const scoped_refptr<ServiceWorkerVersion>& version) {
-  should_activate_when_ready_ = false;
   if (waiting_version_ == version)

[nhiroki, 2016/07/13 04:29:30]

ditto.

     return;
 
+  should_activate_when_ready_ = false;
+
   ChangedVersionAttributesMask mask;
   if (version)
     UnsetVersionInternal(version.get(), &mask);
   waiting_version_ = version;
   mask.add(ChangedVersionAttributesMask::WAITING_VERSION);
 
   NotifyVersionAttributesChanged(mask);
 }
 
 void ServiceWorkerRegistration::SetInstallingVersion(
@@ skipping 21 matching lines @@
 
 void ServiceWorkerRegistration::UnsetVersionInternal(
     ServiceWorkerVersion* version,
     ChangedVersionAttributesMask* mask) {
   DCHECK(version);
   if (installing_version_.get() == version) {
     installing_version_ = NULL;
     mask->add(ChangedVersionAttributesMask::INSTALLING_VERSION);
   } else if (waiting_version_.get() == version) {
     waiting_version_ = NULL;
+    should_activate_when_ready_ = false;
     mask->add(ChangedVersionAttributesMask::WAITING_VERSION);
   } else if (active_version_.get() == version) {
     active_version_->RemoveListener(this);
     active_version_ = NULL;
     mask->add(ChangedVersionAttributesMask::ACTIVE_VERSION);
   }
 }
 
 void ServiceWorkerRegistration::ActivateWaitingVersionWhenReady() {
   DCHECK(waiting_version());
   should_activate_when_ready_ = true;
-
-  if (!active_version() || !active_version()->HasControllee() ||
-      waiting_version()->skip_waiting()) {
+  if (IsReadyToActivate())
     ActivateWaitingVersion(false);

[kinuko, 2016/07/13 02:28:34]

nit: add '/* delay */' comment

[falken, 2016/07/14 13:52:07]

Done.

-  }
 }
 
 void ServiceWorkerRegistration::ClaimClients() {
   DCHECK(context_);
   DCHECK(active_version());
 
   for (std::unique_ptr<ServiceWorkerContextCore::ProviderHostIterator> it =
            context_->GetProviderHostIterator();
        !it->IsAtEnd(); it->Advance()) {
     ServiceWorkerProviderHost* host = it->GetProviderHost();
@@ skipping 46 matching lines @@
                  callback,
                  most_recent_version));
 }
 
 void ServiceWorkerRegistration::OnNoControllees(ServiceWorkerVersion* version) {
   if (!context_)
     return;
   DCHECK_EQ(active_version(), version);
   if (is_uninstalling_)
     Clear();
-  else if (should_activate_when_ready_)
-    ActivateWaitingVersion(true);
+  else if (IsReadyToActivate())
+    ActivateWaitingVersion(true /* delay */);
+}
+
+void ServiceWorkerRegistration::OnNoInflightRequests(
+    ServiceWorkerVersion* version) {
+  if (!context_)
+    return;
+  DCHECK_EQ(active_version(), version);
+  if (IsReadyToActivate())
+    ActivateWaitingVersion(false /* delay */);
+}
+
+bool ServiceWorkerRegistration::IsReadyToActivate() const {
+  if (!should_activate_when_ready_)
+    return false;
+
+  DCHECK(waiting_version());
+  const ServiceWorkerVersion* active = active_version();
+  if (!active)
+    return true;
+  if (!active->HasInflightRequests() &&
+      (!active->HasControllee() || waiting_version()->skip_waiting())) {
+    return true;
+  }
+  return false;
 }
 
 void ServiceWorkerRegistration::ActivateWaitingVersion(bool delay) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(context_);
-  DCHECK(waiting_version());
-  DCHECK(should_activate_when_ready_);
+  DCHECK(IsReadyToActivate());
   should_activate_when_ready_ = false;
   scoped_refptr<ServiceWorkerVersion> activating_version = waiting_version();
   scoped_refptr<ServiceWorkerVersion> exiting_version = active_version();
 
   if (activating_version->is_redundant())
     return;  // Activation is no longer relevant.
 
   // "5. If exitingWorker is not null,
   if (exiting_version.get()) {
-    // TODO(michaeln): should wait for events to be complete
+    // TODO(falken): Update the quoted spec comments once
+    // https://github.com/slightlyoff/ServiceWorker/issues/916 is codified in
+    // the spec.
     // "1. Wait for exitingWorker to finish handling any in-progress requests."
+    // This is already handled by IsReadyToActivate().
     // "2. Terminate exitingWorker."
     exiting_version->StopWorker(
         base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
     // "3. Run the [[UpdateState]] algorithm passing exitingWorker and
     // "redundant" as the arguments."
     exiting_version->SetStatus(ServiceWorkerVersion::REDUNDANT);
   }
 
   // "6. Set serviceWorkerRegistration.activeWorker to activatingWorker."
   // "7. Set serviceWorkerRegistration.waitingWorker to null."
@@ skipping 188 matching lines @@
   if (!context_) {
     callback.Run(SERVICE_WORKER_ERROR_ABORT);
     return;
   }
   context_->storage()->NotifyDoneInstallingRegistration(
       this, version.get(), status);
   callback.Run(status);
 }
 
 }  // namespace content