Make the BackgroundFetchJobController a per-job object

content/browser/background_fetch/background_fetch_context.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/background_fetch/background_fetch_context.h"
 
 #include "content/browser/background_fetch/background_fetch_job_info.h"
 #include "content/browser/background_fetch/background_fetch_request_info.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/storage_partition.h"
 
 namespace content {
 
 BackgroundFetchContext::BackgroundFetchContext(
     BrowserContext* browser_context,
     StoragePartition* storage_partition,
     const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context)
-    : service_worker_context_(service_worker_context),
-      background_fetch_job_controller_(browser_context, storage_partition),
+    : browser_context_(browser_context),
+      storage_partition_(storage_partition),
+      service_worker_context_(service_worker_context),
       background_fetch_data_manager_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // TODO(harkness): BackgroundFetchContext should have
   // ServiceWorkerContextObserver as a parent class and should register as an
   // observer here.
 }
 
 BackgroundFetchContext::~BackgroundFetchContext() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);

[Peter Beverloo, 2017/03/08 14:27:04]

There is nothing that ensures that this gets deleted on the UI thread now -
which will lead to crashes. See platform_notification_context.h for how to make
sure deletion happens on the right thread.

[harkness, 2017/03/09 13:33:25]

Updated it to have DeleteOnUI.

 }
 
 void BackgroundFetchContext::Init() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // TODO(harkness): Create the Download observer.
 }
 
 void BackgroundFetchContext::Shutdown() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&BackgroundFetchContext::ShutdownOnIO, this));
+}
+
+void BackgroundFetchContext::ShutdownOnIO() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Call Shutdown on all pending job controllers to give them a chance to flush
+  // any status to the DataManager.

[Peter Beverloo, 2017/03/08 14:27:04]

That does mean that such flushing must be synchronous (or they must keep
themselves alive), since the destructor will be called at some point after
running this method.

[harkness, 2017/03/09 13:33:25]

Yes, it will either mean that or that we do the flushing as best effort or that
we not flush at all and just re-construct on startup. The later might be the
best thing to do, since we're going to need code for that anyway.

[Peter Beverloo, 2017/03/09 15:18:57]

Not flushing means potential data loss, that's not OK.

[harkness, 2017/03/09 18:35:31]

As discussed in person, we should be able to avoid data loss by reconstructing
the state from the DownloadManager, as long as we make sure that we have flushed
before we tell the DownloadManager to remove the download metadata from the
download database.

+  for (auto& job : job_map_)
+    job.second->Shutdown();
 }
 
 void BackgroundFetchContext::CreateRequest(
     const BackgroundFetchJobInfo& job_info,
     std::vector<BackgroundFetchRequestInfo>& request_infos) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_GE(1U, request_infos.size());
+
   // Inform the data manager about the new download.
   BackgroundFetchJobData* job_data =
       background_fetch_data_manager_.CreateRequest(job_info, request_infos);
+
   // If job_data is null, the DataManager will have logged an error.
-  if (job_data)
-    background_fetch_job_controller_.ProcessJob(job_info.guid(), job_data);
+  if (job_data) {
+    // Create a controller which drives the processing of the job. It will use
+    // the JobData to get information about individual requests for the job.
+    job_map_[job_info.guid()] = base::MakeUnique<BackgroundFetchJobController>(
+        job_info.guid(), browser_context_, storage_partition_, job_data);
+  }
 }
 
 }  // namespace content