Service Worker: in Unregister, wait until after the active worker no longer controls a document

content/browser/service_worker/service_worker_registration.cc

Index: content/browser/service_worker/service_worker_registration.cc
diff --git a/content/browser/service_worker/service_worker_registration.cc b/content/browser/service_worker/service_worker_registration.cc
index 9cd539d42567ca84cdd523fe209973bd1cd293e3..f086d40680dab3f671e3517f299dc6ab08af9cee 100644
--- a/content/browser/service_worker/service_worker_registration.cc
+++ b/content/browser/service_worker/service_worker_registration.cc
@@ -31,6 +31,7 @@ ServiceWorkerRegistration::ServiceWorkerRegistration(
       script_url_(script_url),
       registration_id_(registration_id),
       is_deleted_(false),
+      is_uninstalling_(false),
       should_activate_when_ready_(false),
       context_(context) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
@@ -42,7 +43,8 @@ ServiceWorkerRegistration::~ServiceWorkerRegistration() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (context_)
     context_->RemoveLiveRegistration(registration_id_);
-  ResetShouldActivateWhenReady();
+  if (should_activate_when_ready_ || is_uninstalling())
+    active_version()->RemoveListener(this);
 }
 
 void ServiceWorkerRegistration::AddListener(Listener* listener) {
@@ -66,14 +68,14 @@ ServiceWorkerRegistrationInfo ServiceWorkerRegistration::GetInfo() {
 
 void ServiceWorkerRegistration::SetActiveVersion(
     ServiceWorkerVersion* version) {
-  ResetShouldActivateWhenReady();
+  SetShouldActivateWhenReady(false);
   SetVersionInternal(version, &active_version_,
                      ChangedVersionAttributesMask::ACTIVE_VERSION);
 }
 
 void ServiceWorkerRegistration::SetWaitingVersion(
     ServiceWorkerVersion* version) {
-  ResetShouldActivateWhenReady();
+  SetShouldActivateWhenReady(false);
   SetVersionInternal(version, &waiting_version_,
                      ChangedVersionAttributesMask::WAITING_VERSION);
 }
@@ -133,20 +135,52 @@ void ServiceWorkerRegistration::ActivateWaitingVersionWhenReady() {
   DCHECK(waiting_version());
   if (should_activate_when_ready_)
     return;
-  if (active_version() && active_version()->HasControllee()) {
+  if (active_version() && active_version()->HasControllee())
+    SetShouldActivateWhenReady(true);
+  else
+    ActivateWaitingVersion();
+}
+
+bool ServiceWorkerRegistration::IsListeningForNoControllees() {
+  return active_version() && (should_activate_when_ready_ || is_uninstalling_);
+}
+
+void ServiceWorkerRegistration::SetShouldActivateWhenReady(
+    bool should_activate) {
+  if (should_activate_when_ready_ == should_activate)
+    return;
+  bool listening = IsListeningForNoControllees();
+  should_activate_when_ready_ = should_activate;
+  if (listening == IsListeningForNoControllees())
+    return;
+  if (IsListeningForNoControllees())
     active_version()->AddListener(this);
-    should_activate_when_ready_ = true;
+  else
+    active_version()->RemoveListener(this);
+}

[falken, 2014/07/29 14:04:05]

It may be better for Registration to always listen to its active_version. Then
in NoControllees it always Clears() if is_uninstall is set, or activates the
waiting version if it exists. Then we don't need this bookkeeping about when to
listen.

[michaeln, 2014/07/31 00:08:07]

sgtm

+
+void ServiceWorkerRegistration::SetUninstalling(bool is_uninstalling) {
+  if (is_uninstalling_ == is_uninstalling)
     return;
-  }
-  ActivateWaitingVersion();
+  bool listening = IsListeningForNoControllees();
+  is_uninstalling_ = is_uninstalling;
+  if (listening == IsListeningForNoControllees())
+    return;
+  if (IsListeningForNoControllees())
+    active_version()->AddListener(this);
+  else
+    active_version()->RemoveListener(this);
 }
 
 void ServiceWorkerRegistration::OnNoControllees(ServiceWorkerVersion* version) {
   DCHECK_EQ(active_version(), version);
-  DCHECK(should_activate_when_ready_);
-  active_version_->RemoveListener(this);
+  active_version()->RemoveListener(this);
+  if (is_uninstalling())
+    Clear();
+  else if (should_activate_when_ready_)
+    ActivateWaitingVersion();
+  is_uninstalling_ = false;
   should_activate_when_ready_ = false;
-  ActivateWaitingVersion();
 }
 
 void ServiceWorkerRegistration::ActivateWaitingVersion() {
@@ -193,6 +227,46 @@ void ServiceWorkerRegistration::ActivateWaitingVersion() {
                  this, activating_version));
 }
 
+void ServiceWorkerRegistration::ClearWhenReady() {
+  DCHECK(context_);
+  if (is_uninstalling())
+    return;
+  context_->storage()->NotifyUninstallingRegistration(this);
+
+  // Delete from DB in case browser dies before we're ready.
+  context_->storage()->DeleteRegistration(
+      id(),
+      script_url().GetOrigin(),
+      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+
+  if (active_version() && active_version()->HasControllee())
+    SetUninstalling(true);
+  else
+    Clear();
+}
+
+void ServiceWorkerRegistration::AbortPendingClear() {
+  DCHECK(context_);
+  if (!is_uninstalling())
+    return;
+  SetUninstalling(false);
+  context_->storage()->NotifyDoneUninstallingRegistration(this);
+
+  if (waiting_version()) {
+    context_->storage()->StoreRegistration(
+        this,
+        waiting_version(),
+        base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+    return;
+  }
+  if (active_version()) {
+    context_->storage()->StoreRegistration(
+        this,
+        active_version(),
+        base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  }
+}
+
 void ServiceWorkerRegistration::OnActivateEventFinished(
     ServiceWorkerVersion* activating_version,
     ServiceWorkerStatusCode status) {
@@ -228,17 +302,29 @@ void ServiceWorkerRegistration::OnActivateEventFinished(
   }
 }
 
-void ServiceWorkerRegistration::ResetShouldActivateWhenReady() {
-  if (should_activate_when_ready_) {
-    active_version()->RemoveListener(this);
-    should_activate_when_ready_ = false;
-  }
-}
-
 void ServiceWorkerRegistration::OnDeleteFinished(
     ServiceWorkerStatusCode status) {
   // Intentionally empty completion callback, used to prevent
   // |this| from being deleted until the storage method completes.
 }
 
+void ServiceWorkerRegistration::Clear() {
+  context_->storage()->NotifyDoneUninstallingRegistration(this);
+
+  if (installing_version()) {
+    installing_version()->Doom();
+    UnsetVersion(installing_version());
+  }
+
+  if (waiting_version()) {
+    waiting_version()->Doom();
+    UnsetVersion(waiting_version());
+  }
+
+  if (active_version()) {
+    active_version()->Doom();
+    UnsetVersion(active_version());
+  }
+}
+
 }  // namespace content