Assert about ServiceWorkerRegisterJob internal state in terms of phases

content/browser/service_worker/service_worker_register_job.cc

Index: content/browser/service_worker/service_worker_register_job.cc
diff --git a/content/browser/service_worker/service_worker_register_job.cc b/content/browser/service_worker/service_worker_register_job.cc
index 9ed601b4964ca019aa8a64a96b2555c15346e725..2535a620072c91f5d2575ef9411961db8ba25af2 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -22,6 +22,7 @@ ServiceWorkerRegisterJob::ServiceWorkerRegisterJob(
     : context_(context),
       pattern_(pattern),
       script_url_(script_url),
+      phase_(INITIAL),
       weak_factory_(this) {}
 
 ServiceWorkerRegisterJob::~ServiceWorkerRegisterJob() {}
@@ -32,14 +33,15 @@ void ServiceWorkerRegisterJob::AddCallback(const RegistrationCallback& callback,
   // otherwise queue it up.
   callbacks_.push_back(callback);
   DCHECK_NE(-1, process_id);
-  if (pending_version_) {
-    pending_version_->AddProcessToWorker(process_id);
+  if (phase_ >= UPDATE && pending_version()) {
+    pending_version()->AddProcessToWorker(process_id);
   } else {
     pending_process_ids_.push_back(process_id);
   }
 }
 
 void ServiceWorkerRegisterJob::Start() {
+  phase_ = START;

[dominicc (has gone to gerrit), 2014/04/09 00:09:58]

It would be nice to have SetPhase which DCHECKs the previous state makes sense.
This switch statement should end up reading like a transition table reading

case x:
  DCHECK(phase == y);
  break;
...
phase_ = phase;

indicating transitions from x->y are possible.

[falken, 2014/04/09 01:33:20]

Done.

   context_->storage()->FindRegistrationForPattern(
       pattern_,
       base::Bind(
@@ -57,16 +59,41 @@ bool ServiceWorkerRegisterJob::Equals(ServiceWorkerRegisterJobBase* job) {
 }
 
 RegistrationJobType ServiceWorkerRegisterJob::GetType() {
-  return REGISTER;
+  return REGISTRATION;
+}
+
+void ServiceWorkerRegisterJob::set_registration(
+    ServiceWorkerRegistration* registration) {
+  DCHECK(phase_ == START || phase_ == REGISTER) << phase_;
+  DCHECK(!internal_.registration_);
+  internal_.registration_ = registration;
+}
+
+ServiceWorkerRegistration* ServiceWorkerRegisterJob::registration() {
+  DCHECK(phase_ >= REGISTER) << phase_;
+  DCHECK(internal_.registration_);
+  return internal_.registration_;
+}
+
+void ServiceWorkerRegisterJob::set_pending_version(
+    ServiceWorkerVersion* version) {
+  DCHECK(phase_ == UPDATE || phase_ == ACTIVATE) << phase_;
+  DCHECK(!internal_.pending_version_ || !version);
+  internal_.pending_version_ = version;
+}
+
+ServiceWorkerVersion* ServiceWorkerRegisterJob::pending_version() {
+  DCHECK(phase_ >= UPDATE) << phase_;
+  return internal_.pending_version_;
 }
 
 // This function corresponds to the steps in Register following
 // "Let serviceWorkerRegistration be _GetRegistration(scope)"
-// |registration| corresponds to serviceWorkerRegistration.
+// |existing_registration| corresponds to serviceWorkerRegistration.
 // Throughout this file, comments in quotes are excerpts from the spec.
 void ServiceWorkerRegisterJob::HandleExistingRegistrationAndContinue(
     ServiceWorkerStatusCode status,
-    const scoped_refptr<ServiceWorkerRegistration>& registration) {
+    const scoped_refptr<ServiceWorkerRegistration>& existing_registration) {
   // On unexpected error, abort this registration job.
   if (status != SERVICE_WORKER_ERROR_NOT_FOUND && status != SERVICE_WORKER_OK) {
     Complete(status);
@@ -76,22 +103,23 @@ void ServiceWorkerRegisterJob::HandleExistingRegistrationAndContinue(
   // "If serviceWorkerRegistration is not null and script is equal to
   // serviceWorkerRegistration.scriptUrl..." resolve with the existing
   // registration and abort.
-  if (registration.get() && registration->script_url() == script_url_) {
-    registration_ = registration;
+  if (existing_registration.get() &&
+      existing_registration->script_url() == script_url_) {
+    set_registration(existing_registration);
     // If there's no active version, go ahead to Update (this isn't in the spec
     // but seems reasonable, and without SoftUpdate implemented we can never
     // Update otherwise).
-    if (!registration_->active_version()) {
+    if (!existing_registration->active_version()) {
       UpdateAndContinue(status);
       return;
     }
-    RunCallbacks(status, registration_->active_version());
+    RunCallbacks(status, existing_registration->active_version());
     Complete(SERVICE_WORKER_OK);
     return;
   }
 
   // "If serviceWorkerRegistration is null..." create a new registration.
-  if (!registration.get()) {
+  if (!existing_registration.get()) {
     RegisterAndContinue(SERVICE_WORKER_OK);
     return;
   }
@@ -101,7 +129,7 @@ void ServiceWorkerRegisterJob::HandleExistingRegistrationAndContinue(
   // registering a new one.
   // TODO(falken): Match the spec. We now throw away the active_version_ and
   // pending_version_ of the existing registration, which isn't in the spec.
-  registration->Shutdown();
+  existing_registration->Shutdown();
   context_->storage()->DeleteRegistration(
       pattern_,
       base::Bind(&ServiceWorkerRegisterJob::RegisterAndContinue,
@@ -111,18 +139,18 @@ void ServiceWorkerRegisterJob::HandleExistingRegistrationAndContinue(
 // Registers a new ServiceWorkerRegistration.
 void ServiceWorkerRegisterJob::RegisterAndContinue(
     ServiceWorkerStatusCode status) {
-  DCHECK(!registration_);
+  phase_ = REGISTER;
   if (status != SERVICE_WORKER_OK) {
     // Abort this registration job.
     Complete(status);
     return;
   }
 
-  registration_ = new ServiceWorkerRegistration(
+  set_registration(new ServiceWorkerRegistration(
       pattern_, script_url_, context_->storage()->NewRegistrationId(),
-      context_);
+      context_));
   context_->storage()->StoreRegistration(
-      registration_.get(),
+      registration(),
       base::Bind(&ServiceWorkerRegisterJob::UpdateAndContinue,
                  weak_factory_.GetWeakPtr()));
 }
@@ -130,7 +158,7 @@ void ServiceWorkerRegisterJob::RegisterAndContinue(
 // This function corresponds to the spec's _Update algorithm.
 void ServiceWorkerRegisterJob::UpdateAndContinue(
     ServiceWorkerStatusCode status) {
-  DCHECK(registration_);
+  phase_ = UPDATE;
   if (status != SERVICE_WORKER_OK) {
     // Abort this registration job.
     Complete(status);
@@ -141,20 +169,20 @@ void ServiceWorkerRegisterJob::UpdateAndContinue(
   // terminate the pending worker. It doesn't make sense to implement yet since
   // we always activate the worker if install completed, so there can be no
   // pending worker at this point.
-  DCHECK(!registration_->pending_version());
+  DCHECK(!registration()->pending_version());
 
   // TODO: Script fetching and comparing the old and new script belongs here.
 
   // "Let serviceWorker be a newly-created ServiceWorker object..." and start
   // the worker.
-  pending_version_ = new ServiceWorkerVersion(
-      registration_, context_->storage()->NewVersionId(), context_);
+  set_pending_version(new ServiceWorkerVersion(
+      registration(), context_->storage()->NewVersionId(), context_));
   for (std::vector<int>::const_iterator it = pending_process_ids_.begin();
        it != pending_process_ids_.end();
        ++it)
-    pending_version_->AddProcessToWorker(*it);
+    pending_version()->AddProcessToWorker(*it);
 
-  pending_version_->StartWorker(
+  pending_version()->StartWorker(
       base::Bind(&ServiceWorkerRegisterJob::OnStartWorkerFinished,
                  weak_factory_.GetWeakPtr()));
 }
@@ -172,18 +200,19 @@ void ServiceWorkerRegisterJob::OnStartWorkerFinished(
   // Although the spec doesn't set pendingWorker until after resolving the
   // promise, our system's resolving works by passing ServiceWorkerRegistration
   // to the callbacks, so pendingWorker must be set first.
-  DCHECK(!registration_->pending_version());
-  registration_->set_pending_version(pending_version_);
-  RunCallbacks(status, pending_version_.get());
+  DCHECK(!registration()->pending_version());
+  registration()->set_pending_version(pending_version());
+  RunCallbacks(status, pending_version());
 
   InstallAndContinue();
 }
 
 // This function corresponds to the spec's _Install algorithm.
 void ServiceWorkerRegisterJob::InstallAndContinue() {
+  phase_ = INSTALL;
   // "Set serviceWorkerRegistration.pendingWorker._state to installing."
   // "Fire install event on the associated ServiceWorkerGlobalScope object."
-  pending_version_->DispatchInstallEvent(
+  pending_version()->DispatchInstallEvent(
       -1,
       base::Bind(&ServiceWorkerRegisterJob::OnInstallFinished,
                  weak_factory_.GetWeakPtr()));
@@ -194,7 +223,7 @@ void ServiceWorkerRegisterJob::OnInstallFinished(
   // "If any handler called waitUntil()..." and the resulting promise
   // is rejected, abort.
   if (status != SERVICE_WORKER_OK) {
-    registration_->set_pending_version(NULL);
+    registration()->set_pending_version(NULL);
     Complete(status);
     return;
   }
@@ -205,34 +234,37 @@ void ServiceWorkerRegisterJob::OnInstallFinished(
 
 // This function corresponds to the spec's _Activate algorithm.
 void ServiceWorkerRegisterJob::ActivateAndContinue() {
+  phase_ = ACTIVATE;
   // "Set serviceWorkerRegistration.pendingWorker to null."
-  registration_->set_pending_version(NULL);
+  registration()->set_pending_version(NULL);
 
   // TODO: Dispatch the activate event.
   // TODO(michaeln): Persist the newly ACTIVE version.
-  pending_version_->SetStatus(ServiceWorkerVersion::ACTIVE);
-  DCHECK(!registration_->active_version());
-  registration_->set_active_version(pending_version_);
-  pending_version_ = NULL;
+  pending_version()->SetStatus(ServiceWorkerVersion::ACTIVE);
+  DCHECK(!registration()->active_version());
+  registration()->set_active_version(pending_version());
+  set_pending_version(NULL);
   Complete(SERVICE_WORKER_OK);
 }
 
 void ServiceWorkerRegisterJob::Complete(ServiceWorkerStatusCode status) {
+  phase_ = COMPLETE;
   // In success case the callbacks must have been dispatched already
   // (so this is no-op), otherwise we must have come here for abort case,
   // so dispatch callbacks with NULL.
   DCHECK(callbacks_.empty() || status != SERVICE_WORKER_OK);
   RunCallbacks(status, NULL);
 
-  // If |pending_version_| exists, it was not activated, so we are the sole
+  // If |pending_version| exists, it was not activated, so we are the sole
   // owner of it, so it will be destroyed when this job ends, so Shutdown here.
   // We should be able to remove this code later, when something else holds a
   // reference to |pending_version_|.
   // TODO(kinuko): Fix these ownership and shutdown semantics.
-  if (pending_version_) {
-    DCHECK(!registration_->pending_version());
-    DCHECK(!registration_->active_version());
-    pending_version_->Shutdown();
+  if (pending_version()) {
+    DCHECK(status != SERVICE_WORKER_OK);
+    DCHECK(!registration()->pending_version());
+    DCHECK(!registration()->active_version());
+    pending_version()->Shutdown();
   }
 
   context_->job_coordinator()->FinishJob(pattern_, this);