Ensure ServiceWorker{,Un}RegisterJob's callbacks are executed

This CL replaces ClearForShutdown() call in the destructor of
ServiceWorkerContextCore with AbortAll() to run callbacks.

Some callbacks contain mojo response callbacks which need to be
executed before being destroyed if their corresponding bindings
are still connected. It's a bit hard to disconnect these bindings
so run callbacks in destructors.

Bug: 1161799
Change-Id: I39ff795f7246d944f26f2814cb60adfd68b17abe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2635403Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844676}

content/browser/service_worker/service_worker_context_core.cc

@@ -325,7 +325,7 @@ ServiceWorkerContextCore::~ServiceWorkerContextCore() {
   for (const auto& it : live_versions_)
     it.second->RemoveObserver(this);
 
-  job_coordinator_->ClearForShutdown();
+  job_coordinator_->AbortAll();
 }
 
 std::unique_ptr<ServiceWorkerContextCore::ContainerHostIterator>

content/browser/service_worker/service_worker_job_coordinator.cc

@@ -58,12 +58,6 @@ void ServiceWorkerJobCoordinator::JobQueue::AbortAll() {
   jobs_.clear();
 }
 
-void ServiceWorkerJobCoordinator::JobQueue::ClearForShutdown() {
-  for (const auto& job : jobs_)
-    job->WillShutDown();
-  jobs_.clear();
-}
-
 ServiceWorkerJobCoordinator::ServiceWorkerJobCoordinator(
     ServiceWorkerContextCore* context)
     : context_(context) {
@@ -148,12 +142,6 @@ void ServiceWorkerJobCoordinator::AbortAll() {
   job_queues_.clear();
 }
 
-void ServiceWorkerJobCoordinator::ClearForShutdown() {
-  for (auto& job_pair : job_queues_)
-    job_pair.second.ClearForShutdown();
-  job_queues_.clear();
-}
-
 void ServiceWorkerJobCoordinator::FinishJob(const GURL& scope,
                                             ServiceWorkerRegisterJobBase* job) {
   auto pending_jobs = job_queues_.find(scope);

content/browser/service_worker/service_worker_job_coordinator.h

@@ -52,10 +52,6 @@ class CONTENT_EXPORT ServiceWorkerJobCoordinator {
   void Abort(const GURL& scope);
   void AbortAll();
 
-  // Marks that the ServiceWorkerContextCore is shutting down, so jobs may be
-  // destroyed before finishing.
-  void ClearForShutdown();
-
   // Removes the job. A job that was not aborted must call FinishJob when it is
   // done.
   void FinishJob(const GURL& scope, ServiceWorkerRegisterJobBase* job);
@@ -84,10 +80,6 @@ class CONTENT_EXPORT ServiceWorkerJobCoordinator {
     // Aborts all jobs in the queue and removes them.
     void AbortAll();
 
-    // Marks that the browser is shutting down, so jobs may be destroyed before
-    // finishing.
-    void ClearForShutdown();
-
    private:
     base::circular_deque<std::unique_ptr<ServiceWorkerRegisterJobBase>> jobs_;
 

content/browser/service_worker/service_worker_register_job.cc

@@ -53,7 +53,6 @@ ServiceWorkerRegisterJob::ServiceWorkerRegisterJob(
       outside_fetch_client_settings_object_(
           std::move(outside_fetch_client_settings_object)),
       phase_(INITIAL),
-      is_shutting_down_(false),
       is_promise_resolved_(false),
       should_uninstall_on_failure_(false),
       force_bypass_cache_(false),
@@ -77,7 +76,6 @@ ServiceWorkerRegisterJob::ServiceWorkerRegisterJob(
       outside_fetch_client_settings_object_(
           std::move(outside_fetch_client_settings_object)),
       phase_(INITIAL),
-      is_shutting_down_(false),
       is_promise_resolved_(false),
       should_uninstall_on_failure_(false),
       force_bypass_cache_(force_bypass_cache),
@@ -89,8 +87,7 @@ ServiceWorkerRegisterJob::ServiceWorkerRegisterJob(
 }
 
 ServiceWorkerRegisterJob::~ServiceWorkerRegisterJob() {
-  DCHECK(is_shutting_down_ || phase_ == INITIAL || phase_ == COMPLETE ||
-         phase_ == ABORT)
+  DCHECK(phase_ == INITIAL || phase_ == COMPLETE || phase_ == ABORT)
       << "Jobs should only be interrupted during shutdown.";
 }
 
@@ -152,10 +149,6 @@ void ServiceWorkerRegisterJob::Abort() {
   // the jobs from the queue.
 }
 
-void ServiceWorkerRegisterJob::WillShutDown() {
-  is_shutting_down_ = true;
-}
-
 bool ServiceWorkerRegisterJob::Equals(ServiceWorkerRegisterJobBase* job) const {
   if (job->GetType() != job_type_)
     return false;

content/browser/service_worker/service_worker_register_job.h

@@ -69,7 +69,6 @@ class ServiceWorkerRegisterJob : public ServiceWorkerRegisterJobBase {
   // ServiceWorkerRegisterJobBase implementation:
   void Start() override;
   void Abort() override;
-  void WillShutDown() override;
   bool Equals(ServiceWorkerRegisterJobBase* job) const override;
   RegistrationJobType GetType() const override;
 
@@ -184,7 +183,6 @@ class ServiceWorkerRegisterJob : public ServiceWorkerRegisterJobBase {
       outside_fetch_client_settings_object_;
   std::vector<RegistrationCallback> callbacks_;
   Phase phase_;
-  bool is_shutting_down_;
   Internal internal_;
   bool is_promise_resolved_;
   bool should_uninstall_on_failure_;

content/browser/service_worker/service_worker_register_job_base.h

@@ -24,9 +24,6 @@ class ServiceWorkerRegisterJobBase {
   // It can be called regardless of whether Start() was called.
   virtual void Abort() = 0;
 
-  // Notifies the job that the context is shutting down.
-  virtual void WillShutDown() = 0;
-
   // Returns true if this job is identical to |job| for the purpose of
   // collapsing them together in a ServiceWorkerJobCoordinator queue.
   // Registration jobs are equal if they are for the same scope and script

content/browser/service_worker/service_worker_unregister_job.cc

@@ -24,14 +24,11 @@ ServiceWorkerUnregisterJob::ServiceWorkerUnregisterJob(
     ServiceWorkerContextCore* context,
     const GURL& scope,
     bool is_immediate)
-    : context_(context),
-      scope_(scope),
-      is_immediate_(is_immediate),
-      is_promise_resolved_(false) {
+    : context_(context), scope_(scope), is_immediate_(is_immediate) {
   DCHECK(context_);
 }
 
-ServiceWorkerUnregisterJob::~ServiceWorkerUnregisterJob() {}
+ServiceWorkerUnregisterJob::~ServiceWorkerUnregisterJob() = default;
 
 void ServiceWorkerUnregisterJob::AddCallback(UnregistrationCallback callback) {
   callbacks_.emplace_back(std::move(callback));
@@ -48,8 +45,6 @@ void ServiceWorkerUnregisterJob::Abort() {
                    blink::ServiceWorkerStatusCode::kErrorAbort);
 }
 
-void ServiceWorkerUnregisterJob::WillShutDown() {}
-
 bool ServiceWorkerUnregisterJob::Equals(
     ServiceWorkerRegisterJobBase* job) const {
   if (job->GetType() != GetType())
@@ -109,6 +104,7 @@ void ServiceWorkerUnregisterJob::ResolvePromise(
   is_promise_resolved_ = true;
   for (UnregistrationCallback& callback : callbacks_)
     std::move(callback).Run(registration_id, status);
+  callbacks_.clear();
 }
 
 }  // namespace content

content/browser/service_worker/service_worker_unregister_job.h

@@ -46,7 +46,6 @@ class ServiceWorkerUnregisterJob : public ServiceWorkerRegisterJobBase {
   // ServiceWorkerRegisterJobBase implementation:
   void Start() override;
   void Abort() override;
-  void WillShutDown() override;
   bool Equals(ServiceWorkerRegisterJobBase* job) const override;
   RegistrationJobType GetType() const override;
 
@@ -65,7 +64,7 @@ class ServiceWorkerUnregisterJob : public ServiceWorkerRegisterJobBase {
   const GURL scope_;
   const bool is_immediate_;
   std::vector<UnregistrationCallback> callbacks_;
-  bool is_promise_resolved_;
+  bool is_promise_resolved_ = false;
   base::WeakPtrFactory<ServiceWorkerUnregisterJob> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerUnregisterJob);