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..f90eecd89b4a058ea4c4b52b5265776f5cf029c7 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 (IsListeningForNoControllees())
+    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_);

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

there's some subtlety to this expression and about when it really is and isn't
valid, maybe an explicit flag for whether the listerer has actually been
attached and rename this method to ShouldBeListenengingForNoControllees?

i think this could simpliflfy/clarify aspects of the
SetShouldActivateWhenReady() and SetUninstalling() methods too

+}
+
+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())

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

very small nit: testing bools for equality always looks odd to me, maybe get rid
of one of them?

bool is_listening = IsListening();
should_activate_ = shld;
bool should_listen = IsListening();

if (is_listening && !should_listen)
 rmvlistener;
else if (should_listen && !is_listening)
 addlisteneer;

+    return;
+  if (IsListeningForNoControllees())
     active_version()->AddListener(this);
-    should_activate_when_ready_ = true;
+  else
+    active_version()->RemoveListener(this);
+}
+
+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,26 @@ void ServiceWorkerRegistration::ActivateWaitingVersion() {
                  this, activating_version));
 }
 
+void ServiceWorkerRegistration::ClearWhenReady() {
+  DCHECK(context_);
+  if (is_uninstalling())
+    return;
+  context_->storage()->NotifyUninstallingRegistration(this);
+
+  if (active_version() && active_version()->HasControllee())
+    SetUninstalling(true);
+  else
+    Clear();
+}
+
+void ServiceWorkerRegistration::AbortPendingClear() {
+  DCHECK(context_);
+  if (!is_uninstalling())
+    return;
+  SetUninstalling(false);
+  context_->storage()->NotifyAbortedUninstallingRegistration(this);
+}
+
 void ServiceWorkerRegistration::OnActivateEventFinished(
     ServiceWorkerVersion* activating_version,
     ServiceWorkerStatusCode status) {
@@ -228,17 +282,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()->NotifyUninstalledRegistration(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