Decouple script_url from ServiceWorkerRegistration

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 a0453313bc0d33e636ac973cd616178b878e28a5..971044aff8dd12a4572cae1d119414f7944cfb08 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -44,11 +44,11 @@ ServiceWorkerRegisterJob::ServiceWorkerRegisterJob(
     : context_(context),
       job_type_(UPDATE_JOB),
       pattern_(registration->pattern()),
-      script_url_(registration->script_url()),

[michaeln, 2014/08/25 22:00:58]

Can this data member can remain const and be reliably set at all times?

  script_url_(registration()->GetNewestVersion()->script_url());

Othewise the ServiceWorkerRegisterJob::Equals comparison function does not work.
I think we can assert that GetNewestVersion() is not null since if there is no
newest version, there can't be anything to update.

[falken, 2014/08/26 01:02:27]

Great catch. But we must handle this scenario:

1. Queue a Register job for same-scope, new script URL
2. Queue an Update job, it sets script_url_ to current script
3. Register job makes a waiting version with new script
4. Update job will make an installing version with the old script

I think Update can just abort when it starts if its script_url_ is not the
newest version's.

       phase_(INITIAL),
       is_promise_resolved_(false),
       promise_resolved_status_(SERVICE_WORKER_OK),
       weak_factory_(this) {
+  DCHECK(registration);
   internal_.registration = registration;
 }
 
@@ -190,7 +190,7 @@ void ServiceWorkerRegisterJob::ContinueWithRegistration(
   existing_registration->AbortPendingClear();
 
   // "If scriptURL is equal to registration.[[ScriptURL]], then:"
-  if (existing_registration->script_url() == script_url_) {
+  if (existing_registration->GetNewestVersion()->script_url() == script_url_) {
     // Spec says to resolve with registration.[[GetNewestWorker]]. We come close
     // by resolving with the active version.
     set_registration(existing_registration);
@@ -213,7 +213,7 @@ void ServiceWorkerRegisterJob::ContinueWithRegistration(
   // controllees.
   context_->storage()->DeleteRegistration(
       existing_registration->id(),
-      existing_registration->script_url().GetOrigin(),
+      existing_registration->pattern().GetOrigin(),
       base::Bind(&ServiceWorkerRegisterJob::RegisterAndContinue,
                  weak_factory_.GetWeakPtr()));
 }
@@ -232,6 +232,14 @@ void ServiceWorkerRegisterJob::ContinueWithUpdate(
     return;
   }
 
+  // A previous job may have unregistered this registration.
+  if (registration()->is_uninstalling()) {
+    Complete(SERVICE_WORKER_ERROR_NOT_FOUND);
+    return;
+  }
+
+  script_url_ = registration()->GetNewestVersion()->script_url();
+
   // TODO(michaeln): If the last update check was less than 24 hours
   // ago, depending on the freshness of the cached worker script we
   // may be able to complete the update job right here.
@@ -250,8 +258,7 @@ void ServiceWorkerRegisterJob::RegisterAndContinue(
   }
 
   set_registration(new ServiceWorkerRegistration(
-      pattern_, script_url_, context_->storage()->NewRegistrationId(),
-      context_));
+      pattern_, context_->storage()->NewRegistrationId(), context_));
   AssociateProviderHostsToRegistration(registration());
   UpdateAndContinue();
 }
@@ -268,8 +275,10 @@ void ServiceWorkerRegisterJob::UpdateAndContinue() {
 
   // "Let serviceWorker be a newly-created ServiceWorker object..." and start
   // the worker.
-  set_new_version(new ServiceWorkerVersion(
-      registration(), context_->storage()->NewVersionId(), context_));
+  set_new_version(new ServiceWorkerVersion(registration(),
+                                           script_url_,
+                                           context_->storage()->NewVersionId(),
+                                           context_));
 
   bool pause_after_download = job_type_ == UPDATE_JOB;
   if (pause_after_download)
@@ -386,7 +395,7 @@ void ServiceWorkerRegisterJob::CompleteInternal(
         registration()->NotifyRegistrationFailed();
         context_->storage()->DeleteRegistration(
             registration()->id(),
-            registration()->script_url().GetOrigin(),
+            registration()->pattern().GetOrigin(),
             base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
       }
     }