[Background Fetch] Fix UpdateUI race condition.

The jobs were sometimes being erased before the icon was updated. The
delegate now notifies the clients after the new UI information has been
passed along.

This adds another internal Job State to the delegate to know when the
job was complete.

Bug: 917384

Change-Id: I00c599e298e342fbfafaaa9a39d5d7bcdc5cf7ba
Reviewed-on: https://chromium-review.googlesource.com/c/1388646
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Mugdha Lakhani <nator@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619916}

chrome/browser/background_fetch/background_fetch_browsertest.cc

@@ -707,6 +707,9 @@ IN_PROC_BROWSER_TEST_F(BackgroundFetchBrowserTest,
                        FetchesRunToCompletionAndUpdateTitle_Fetched) {
   ASSERT_NO_FATAL_FAILURE(RunScriptAndCheckResultingMessage(
       "RunFetchTillCompletion()", "backgroundfetchsuccess"));
+  EXPECT_EQ(offline_content_provider_observer_->latest_item().state,
+            offline_items_collection::OfflineItemState::COMPLETE);
+
   base::RunLoop().RunUntilIdle();  // Give `updateUI` a chance to propagate.
   EXPECT_TRUE(
       base::StartsWith(offline_content_provider_observer_->latest_item().title,
@@ -720,6 +723,9 @@ IN_PROC_BROWSER_TEST_F(BackgroundFetchBrowserTest,
                        FetchesRunToCompletionAndUpdateTitle_Failed) {
   ASSERT_NO_FATAL_FAILURE(RunScriptAndCheckResultingMessage(
       "RunFetchTillCompletionWithMissingResource()", "backgroundfetchfail"));
+  EXPECT_EQ(offline_content_provider_observer_->latest_item().state,
+            offline_items_collection::OfflineItemState::COMPLETE);
+
   base::RunLoop().RunUntilIdle();  // Give `updateUI` a chance to propagate.
   EXPECT_TRUE(
       base::StartsWith(offline_content_provider_observer_->latest_item().title,

chrome/browser/background_fetch/background_fetch_delegate_impl.cc

@@ -108,6 +108,13 @@ void BackgroundFetchDelegateImpl::JobDetails::MarkJobAsStarted() {
     job_state = State::kStartedButPaused;
 }
 
+void BackgroundFetchDelegateImpl::JobDetails::UpdateJobOnDownloadComplete() {
+  fetch_description->completed_parts++;
+  in_progress_parts_size = 0u;
+  if (fetch_description->completed_parts == fetch_description->total_parts)
+    job_state = State::kDownloadsComplete;
+}
+
 void BackgroundFetchDelegateImpl::JobDetails::UpdateOfflineItem() {
   DCHECK_GT(fetch_description->total_parts, 0);
 
@@ -116,8 +123,7 @@ void BackgroundFetchDelegateImpl::JobDetails::UpdateOfflineItem() {
     // If we have completed all downloads, update progress max to
     // completed_parts_size in case total_parts_size was set too high. This
     // avoid unnecessary jumping in the progress bar.
-    offline_item.progress.max =
-        (fetch_description->completed_parts == fetch_description->total_parts)
+    offline_item.progress.max = job_state == State::kDownloadsComplete
                                     ? fetch_description->completed_parts_size
                                     : fetch_description->total_parts_size;
   } else {
@@ -134,18 +140,21 @@ void BackgroundFetchDelegateImpl::JobDetails::UpdateOfflineItem() {
   offline_item.is_resumable = true;
 
   using OfflineItemState = offline_items_collection::OfflineItemState;
-  if (job_state == State::kCancelled) {
+  switch (job_state) {
+    case State::kCancelled:
       offline_item.state = OfflineItemState::CANCELLED;
-  } else if (fetch_description->completed_parts ==
-             fetch_description->total_parts) {
+      break;
+    case State::kDownloadsComplete:
       // This includes cases when the download failed, or completed but the
       // response was an HTTP error, e.g. 404.
       offline_item.state = OfflineItemState::COMPLETE;
       offline_item.is_openable = true;
-  } else if (job_state == State::kPendingWillStartPaused ||
-             job_state == State::kStartedButPaused) {
+      break;
+    case State::kPendingWillStartPaused:
+    case State::kStartedButPaused:
       offline_item.state = OfflineItemState::PAUSED;
-  } else {
+      break;
+    default:
       offline_item.state = OfflineItemState::IN_PROGRESS;
   }
 }
@@ -361,6 +370,17 @@ void BackgroundFetchDelegateImpl::UpdateUI(
     job_details.offline_item.refresh_visuals = true;
   }
 
+  bool should_update_visuals = job_details.offline_item.refresh_visuals;
+#if !defined(OS_ANDROID)
+  should_update_visuals = false;
+#endif
+
+  if (!should_update_visuals) {
+    // Notify the client that the UI updates have been handed over.
+    if (job_details.client)
+      job_details.client->OnUIUpdated(job_unique_id);
+  }
+
   UpdateOfflineItemAndUpdateObservers(&job_details);
 }
 
@@ -438,9 +458,8 @@ void BackgroundFetchDelegateImpl::OnDownloadFailed(
 
   const std::string& job_unique_id = download_job_unique_id_iter->second;
   JobDetails& job_details = job_details_map_.find(job_unique_id)->second;
-  ++job_details.fetch_description->completed_parts;
-  job_details.in_progress_parts_size = 0u;
 
+  job_details.UpdateJobOnDownloadComplete();
   UpdateOfflineItemAndUpdateObservers(&job_details);
 
   // The client cancelled or aborted the download so no need to notify it.
@@ -472,8 +491,7 @@ void BackgroundFetchDelegateImpl::OnDownloadSucceeded(
 
   const std::string& job_unique_id = download_job_unique_id_iter->second;
   JobDetails& job_details = job_details_map_.find(job_unique_id)->second;
-  ++job_details.fetch_description->completed_parts;
-  job_details.in_progress_parts_size = 0u;
+  job_details.UpdateJobOnDownloadComplete();
 
   job_details.fetch_description->completed_parts_size +=
       profile_->IsOffTheRecord() ? result->blob_handle->size()
@@ -633,9 +651,14 @@ void BackgroundFetchDelegateImpl::GetVisualsForItem(
       std::make_unique<offline_items_collection::OfflineItemVisuals>();
   auto it = job_details_map_.find(id.id);
   if (it != job_details_map_.end()) {
+    auto& job_details = it->second;
     visuals->icon =
-        gfx::Image::CreateFrom1xBitmap(it->second.fetch_description->icon);
-    it->second.offline_item.refresh_visuals = false;
+        gfx::Image::CreateFrom1xBitmap(job_details.fetch_description->icon);
+    job_details.offline_item.refresh_visuals = false;
+    if (job_details.client &&
+        job_details.job_state == JobDetails::State::kDownloadsComplete) {
+      job_details.client->OnUIUpdated(id.id);
+    }
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(

chrome/browser/background_fetch/background_fetch_delegate_impl.h

@@ -142,7 +142,10 @@ class BackgroundFetchDelegateImpl
       kPendingWillStartDownloading,
       kStartedButPaused,
       kStartedAndDownloading,
+      // The job was aborted.
       kCancelled,
+      // All requests were processed (either succeeded or failed).
+      kDownloadsComplete,
     };
 
     JobDetails(JobDetails&&);
@@ -155,6 +158,7 @@ class BackgroundFetchDelegateImpl
 
     void UpdateOfflineItem();
     void MarkJobAsStarted();
+    void UpdateJobOnDownloadComplete();
 
     // Returns how many bytes have been processed by the Download Service so
     // far.

content/browser/background_fetch/background_fetch_context.cc

@@ -229,8 +229,8 @@ void BackgroundFetchContext::UpdateUI(
     blink::mojom::BackgroundFetchService::UpdateUICallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  delegate_proxy_.UpdateUI(registration_id.unique_id(), title, icon);
-  std::move(callback).Run(blink::mojom::BackgroundFetchError::NONE);
+  delegate_proxy_.UpdateUI(registration_id.unique_id(), title, icon,
+                           std::move(callback));
 }
 
 void BackgroundFetchContext::Abort(

content/browser/background_fetch/background_fetch_delegate_proxy.cc

@@ -207,6 +207,7 @@ class BackgroundFetchDelegateProxy::Core
       const std::string& guid,
       std::unique_ptr<content::BackgroundFetchResponse> response) override;
   void OnUIActivated(const std::string& unique_id) override;
+  void OnUIUpdated(const std::string& unique_id) override;
   void GetUploadData(
       const std::string& job_unique_id,
       const std::string& download_guid,
@@ -279,6 +280,16 @@ void BackgroundFetchDelegateProxy::Core::OnUIActivated(
                      job_unique_id));
 }
 
+void BackgroundFetchDelegateProxy::Core::OnUIUpdated(
+    const std::string& job_unique_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  base::PostTaskWithTraits(
+      FROM_HERE, {BrowserThread::IO},
+      base::BindOnce(&BackgroundFetchDelegateProxy::DidUpdateUI, io_parent_,
+                     job_unique_id));
+}
+
 void BackgroundFetchDelegateProxy::Core::GetUploadData(
     const std::string& job_unique_id,
     const std::string& download_guid,
@@ -405,9 +416,20 @@ void BackgroundFetchDelegateProxy::StartRequest(
 void BackgroundFetchDelegateProxy::UpdateUI(
     const std::string& job_unique_id,
     const base::Optional<std::string>& title,
-    const base::Optional<SkBitmap>& icon) {
+    const base::Optional<SkBitmap>& icon,
+    blink::mojom::BackgroundFetchService::UpdateUICallback update_ui_callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  auto job_details_iter = job_details_map_.find(job_unique_id);
+  if (job_details_iter == job_details_map_.end()) {
+    std::move(update_ui_callback)
+        .Run(blink::mojom::BackgroundFetchError::INVALID_ID);
+    return;
+  }
+
+  JobDetails& job_details = job_details_iter->second;
+  job_details.update_ui_callback = std::move(update_ui_callback);
+
   base::PostTaskWithTraits(FROM_HERE, {BrowserThread::UI},
                            base::BindOnce(&Core::UpdateUI, ui_core_ptr_,
                                           job_unique_id, title, icon));
@@ -481,6 +503,19 @@ void BackgroundFetchDelegateProxy::DidActivateUI(
   click_event_dispatcher_callback_.Run(job_unique_id);
 }
 
+void BackgroundFetchDelegateProxy::DidUpdateUI(
+    const std::string& job_unique_id) {
+  auto job_details_iter = job_details_map_.find(job_unique_id);
+  if (job_details_iter == job_details_map_.end())
+    return;
+
+  JobDetails& job_details = job_details_iter->second;
+
+  DCHECK(job_details.update_ui_callback);
+  std::move(job_details.update_ui_callback)
+      .Run(blink::mojom::BackgroundFetchError::NONE);
+}
+
 void BackgroundFetchDelegateProxy::OnDownloadUpdated(
     const std::string& job_unique_id,
     const std::string& guid,

content/browser/background_fetch/background_fetch_delegate_proxy.h

@@ -33,7 +33,7 @@ class CONTENT_EXPORT BackgroundFetchDelegateProxy {
   // Subclasses must only be destroyed on the IO thread, since these methods
   // will be called on the IO thread.
   using DispatchClickEventCallback =
-      base::RepeatingCallback<void(const std::string& /* unique_id */)>;
+      base::RepeatingCallback<void(const std::string& unique_id)>;
   class Controller {
    public:
     // Called when the given |request| has started fetching.
@@ -110,11 +110,14 @@ class CONTENT_EXPORT BackgroundFetchDelegateProxy {
                     scoped_refptr<BackgroundFetchRequestInfo> request);
 
   // Updates the representation of this registration in the user interface to
-  // match the given |title| or |icon|.
+  // match the given |title| or |icon|. |update_ui_callback| should be called
+  // after all the relevant UI information has been processed.
   // Called from the Controller (on the IO thread).
   void UpdateUI(const std::string& job_unique_id,
                 const base::Optional<std::string>& title,
-                const base::Optional<SkBitmap>& icon);
+                const base::Optional<SkBitmap>& icon,
+                blink::mojom::BackgroundFetchService::UpdateUICallback
+                    update_ui_callback);
 
   // Aborts in progress downloads for the given registration. Called from the
   // Controller (on the IO thread) after it is aborted. May occur even if all
@@ -155,6 +158,9 @@ class CONTENT_EXPORT BackgroundFetchDelegateProxy {
   // Should only be called from the BackgroundFetchDelegate (on the IO thread).
   void DidActivateUI(const std::string& job_unique_id);
 
+  // Should only be called from the BackgroundFetchDelegate (on the IO thread).
+  void DidUpdateUI(const std::string& job_unique_id);
+
   // Should only be called from the BackgroundFetchDelegate (on the IO thread).
   void GetUploadData(const std::string& job_unique_id,
                      const std::string& download_guid,
@@ -176,6 +182,8 @@ class CONTENT_EXPORT BackgroundFetchDelegateProxy {
     base::flat_map<std::string, scoped_refptr<BackgroundFetchRequestInfo>>
         current_request_map;
 
+    blink::mojom::BackgroundFetchService::UpdateUICallback update_ui_callback;
+
    private:
     DISALLOW_COPY_AND_ASSIGN(JobDetails);
   };

content/browser/background_fetch/background_fetch_delegate_proxy_unittest.cc

@@ -324,7 +324,8 @@ TEST_F(BackgroundFetchDelegateProxyTest, UpdateUI) {
   EXPECT_TRUE(controller.request_started_);
   EXPECT_TRUE(controller.request_completed_);
 
-  delegate_proxy_.UpdateUI(kExampleUniqueId, "Job 1 Complete!", base::nullopt);
+  delegate_proxy_.UpdateUI(kExampleUniqueId, "Job 1 Complete!", base::nullopt,
+                           base::DoNothing());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(delegate_->ui_update_count_, 1);
 }

content/browser/background_fetch/mock_background_fetch_delegate.cc

@@ -199,7 +199,9 @@ void MockBackgroundFetchDelegate::MarkJobComplete(
 void MockBackgroundFetchDelegate::UpdateUI(
     const std::string& job_unique_id,
     const base::Optional<std::string>& title,
-    const base::Optional<SkBitmap>& icon) {}
+    const base::Optional<SkBitmap>& icon) {
+  job_id_to_client_map_[job_unique_id]->OnUIUpdated(job_unique_id);
+}
 
 void MockBackgroundFetchDelegate::RegisterResponse(
     const GURL& url,

content/public/browser/background_fetch_delegate.h

@@ -99,6 +99,9 @@ class CONTENT_EXPORT BackgroundFetchDelegate {
     // Called when the UI of a background fetch job is activated.
     virtual void OnUIActivated(const std::string& job_unique_id) = 0;
 
+    // Called after the UI has been updated.
+    virtual void OnUIUpdated(const std::string& job_unique_id) = 0;
+
     // Called by the Download Client when it needs the upload data for
     // the given |download_guid|.
     virtual void GetUploadData(const std::string& job_unique_id,

content/shell/browser/web_test/web_test_background_fetch_delegate.cc

@@ -182,6 +182,11 @@ class WebTestBackgroundFetchDelegate::WebTestBackgroundFetchDownloadClient
     std::move(callback).Run(std::move(request_body));
   }
 
+  const base::WeakPtr<content::BackgroundFetchDelegate::Client>& client()
+      const {
+    return client_;
+  }
+
  private:
   base::WeakPtr<content::BackgroundFetchDelegate::Client> client_;
   base::flat_map<std::string, std::string> guid_to_unique_job_id_mapping_;
@@ -286,6 +291,8 @@ void WebTestBackgroundFetchDelegate::MarkJobComplete(
 void WebTestBackgroundFetchDelegate::UpdateUI(
     const std::string& job_unique_id,
     const base::Optional<std::string>& title,
-    const base::Optional<SkBitmap>& icon) {}
+    const base::Optional<SkBitmap>& icon) {
+  background_fetch_client_->client()->OnUIUpdated(job_unique_id);
+}
 
 }  // namespace content