[Background Fetch] Cleanup/fix thread safety

content/browser/background_fetch/background_fetch_job_controller.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_job_controller.h"
 
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 30 matching lines @@
 class BackgroundFetchJobController::Core : public DownloadItem::Observer {
  public:
   Core(const base::WeakPtr<BackgroundFetchJobController>& io_parent,
        const BackgroundFetchRegistrationId& registration_id,
        BrowserContext* browser_context,
        scoped_refptr<net::URLRequestContextGetter> request_context)
       : io_parent_(io_parent),
         registration_id_(registration_id),
         browser_context_(browser_context),
         request_context_(std::move(request_context)),
-        weak_ptr_factory_(this) {}
+        weak_ptr_factory_(this) {
+    // Although the Core lives only on the UI thread, it is constructed on IO.
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  }
 
   ~Core() final {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
     for (const auto& pair : downloads_)
       pair.first->RemoveObserver(this);
   }
 
-  // Returns a weak pointer that can be used to talk to |this|.
-  base::WeakPtr<Core> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); }
+  // Returns a weak pointer that can be used to talk to |this|. Must only be
+  // called on the UI thread, unless the Core is guaranteed not to be destroyed
+  // in parallel with this call.
+  base::WeakPtr<Core> GetWeakPtrOnUI() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
 
   // Starts fetching the |request| with the download manager.
   void StartRequest(
       scoped_refptr<BackgroundFetchRequestInfo> request,
       const net::NetworkTrafficAnnotationTag& traffic_annotation) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     DCHECK(request_context_);
     DCHECK(request);
 
     DownloadManager* download_manager =
@@ skipping 47 matching lines @@
   void OnDownloadUpdated(DownloadItem* download_item) override {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
     auto iter = downloads_.find(download_item);
     DCHECK(iter != downloads_.end());
 
     scoped_refptr<BackgroundFetchRequestInfo> request = iter->second;
 
     switch (download_item->GetState()) {
       case DownloadItem::DownloadState::COMPLETE:
-        request->PopulateResponseFromDownloadItem(download_item);
+        request->PopulateResponseFromDownloadItemOnUI(download_item);
+        BrowserThread::PostTask(
+            BrowserThread::IO, FROM_HERE,
+            base::Bind(&BackgroundFetchRequestInfo::SetResponseDataPopulated,
+                       request));
         download_item->RemoveObserver(this);
 
         // Inform the host about |host| having completed.
         BrowserThread::PostTask(
             BrowserThread::IO, FROM_HERE,
             base::Bind(&BackgroundFetchJobController::DidCompleteRequest,
                        io_parent_, std::move(request)));
 
         // Clear the local state for the |request|, it no longer is our concern.
         downloads_.erase(iter);
@@ skipping 26 matching lines @@
   // Called when the download manager has started the given |request|. The
   // |download_item| continues to be owned by the download system. The
   // |interrupt_reason| will indicate when a request could not be started.
   void DidStartRequest(scoped_refptr<BackgroundFetchRequestInfo> request,
                        DownloadItem* download_item,
                        DownloadInterruptReason interrupt_reason) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     DCHECK_EQ(interrupt_reason, DOWNLOAD_INTERRUPT_REASON_NONE);
     DCHECK(download_item);
 
-    request->PopulateDownloadState(download_item, interrupt_reason);
+    request->PopulateDownloadStateOnUI(download_item, interrupt_reason);
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&BackgroundFetchRequestInfo::SetDownloadStatePopulated,
+                   request));
 
     // TODO(peter): The above two DCHECKs are assumptions our implementation
     // currently makes, but are not fit for production. We need to handle such
     // failures gracefully.
 
     // Register for updates on the download's progress.
     download_item->AddObserver(this);
 
     // Inform the host about the |request| having started.
     BrowserThread::PostTask(
@@ skipping 41 matching lines @@
       completed_callback_(std::move(completed_callback)),
       traffic_annotation_(traffic_annotation),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // Create the core, containing the internal functionality that will have to
   // be run on the UI thread. It will respond to this class with a weak pointer.
   ui_core_.reset(new Core(weak_ptr_factory_.GetWeakPtr(), registration_id,
                           browser_context, std::move(request_context)));
 
-  // Get a WeakPtr over which we can talk to the |ui_core_|.
-  ui_core_ptr_ = ui_core_->GetWeakPtr();
+  // Get a WeakPtr over which we can talk to the |ui_core_|. Normally it would
+  // be unsafe to obtain a weak pointer on the IO thread from a factory that
+  // lives on the UI thread, but it's ok in this constructor since the Core
+  // can't be destroyed before this constructor finishes.
+  ui_core_ptr_ = ui_core_->GetWeakPtrOnUI();
 }
 
-BackgroundFetchJobController::~BackgroundFetchJobController() = default;
+BackgroundFetchJobController::~BackgroundFetchJobController() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+};
 
 void BackgroundFetchJobController::Start(
     std::vector<scoped_refptr<BackgroundFetchRequestInfo>> initial_requests) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_LE(initial_requests.size(), kMaximumBackgroundFetchParallelRequests);
   DCHECK_EQ(state_, State::INITIALIZED);
 
   state_ = State::FETCHING;
 
   for (const auto& request : initial_requests)
@@ skipping 26 matching lines @@
   pending_completed_file_acknowledgements_++;
 
   data_manager_->MarkRequestAsCompleteAndGetNextRequest(
       registration_id_, request.get(),
       base::BindOnce(&BackgroundFetchJobController::DidGetNextRequest,
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BackgroundFetchJobController::DidGetNextRequest(
     scoped_refptr<BackgroundFetchRequestInfo> request) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_LE(pending_completed_file_acknowledgements_, 1);
   pending_completed_file_acknowledgements_--;
 
   // If a |request| has been given, start downloading the file and bail.
   if (request) {
     StartRequest(std::move(request));
     return;
   }
 
   // If there are outstanding completed file acknowlegements, bail as well.
@@ skipping 17 matching lines @@
 
   // TODO(harkness): Abort all in-progress downloads.
 
   state_ = State::ABORTED;
 
   // Inform the owner of the controller about the job having completed.
   std::move(completed_callback_).Run(this);
 }
 
 }  // namespace content