[Background Fetch] Delete all in-memory fetch data

Inform upstream systems when a fetch is completely finished (event
dispatch is complete) to know that it's safe to remove in-memory
fetch data.

Bug: 904818
Change-Id: Ieef3ff87ce66e97f177ef8422994adb22568e155
Reviewed-on: https://chromium-review.googlesource.com/c/1333815Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608033}

chrome/browser/background_fetch/background_fetch_browsertest.cc

@@ -256,12 +256,11 @@ class BackgroundFetchBrowserTest : public InProcessBrowserTest {
 
     SetUpBrowser(browser());
 
-    BackgroundFetchDelegateImpl* delegate =
-        static_cast<BackgroundFetchDelegateImpl*>(
-            active_browser_->profile()->GetBackgroundFetchDelegate());
-    DCHECK(delegate);
+    delegate_ = static_cast<BackgroundFetchDelegateImpl*>(
+        active_browser_->profile()->GetBackgroundFetchDelegate());
+    DCHECK(delegate_);
 
-    offline_content_provider_observer_->set_delegate(delegate);
+    offline_content_provider_observer_->set_delegate(delegate_);
   }
 
   void SetUpBrowser(Browser* browser) {
@@ -323,11 +322,8 @@ class BackgroundFetchBrowserTest : public InProcessBrowserTest {
       const ContentId& offline_item_id,
       std::unique_ptr<OfflineItemVisuals>* out_visuals) {
     base::RunLoop run_loop;
-    BackgroundFetchDelegateImpl* delegate =
-        static_cast<BackgroundFetchDelegateImpl*>(
-            active_browser_->profile()->GetBackgroundFetchDelegate());
-    DCHECK(delegate);
-    delegate->GetVisualsForItem(
+
+    delegate_->GetVisualsForItem(
         offline_item_id, base::Bind(&BackgroundFetchBrowserTest::DidGetVisuals,
                                     base::Unretained(this),
                                     run_loop.QuitClosure(), out_visuals));
@@ -427,7 +423,8 @@ class BackgroundFetchBrowserTest : public InProcessBrowserTest {
   }
 
  protected:
-  download::DownloadService* download_service_{nullptr};
+  BackgroundFetchDelegateImpl* delegate_ = nullptr;
+  download::DownloadService* download_service_ = nullptr;
 
   std::unique_ptr<WaitableDownloadLoggerObserver> download_observer_;
   std::unique_ptr<OfflineContentProviderObserver>
@@ -685,6 +682,9 @@ IN_PROC_BROWSER_TEST_F(BackgroundFetchBrowserTest,
   EXPECT_TRUE(
       base::StartsWith(offline_content_provider_observer_->latest_item().title,
                        "New Fetched Title!", base::CompareCase::SENSITIVE));
+
+  // Make sure the delegate cleans up after the fetch is complete.
+  EXPECT_TRUE(delegate_->job_details_map_.empty());
 }
 
 IN_PROC_BROWSER_TEST_F(BackgroundFetchBrowserTest,

chrome/browser/background_fetch/background_fetch_delegate_impl.cc

@@ -314,7 +314,11 @@ void BackgroundFetchDelegateImpl::Abort(const std::string& job_unique_id) {
     download_job_unique_id_map_.erase(download_guid);
   }
   UpdateOfflineItemAndUpdateObservers(&job_details);
-  job_details_map_.erase(job_details_iter);
+}
+
+void BackgroundFetchDelegateImpl::MarkJobComplete(
+    const std::string& job_unique_id) {
+  job_details_map_.erase(job_unique_id);
 }
 
 void BackgroundFetchDelegateImpl::UpdateUI(

chrome/browser/background_fetch/background_fetch_delegate_impl.h

@@ -65,6 +65,7 @@ class BackgroundFetchDelegateImpl
                    const net::NetworkTrafficAnnotationTag& traffic_annotation,
                    const net::HttpRequestHeaders& headers) override;
   void Abort(const std::string& job_unique_id) override;
+  void MarkJobComplete(const std::string& job_unique_id) override;
   void UpdateUI(const std::string& job_unique_id,
                 const base::Optional<std::string>& title,
                 const base::Optional<SkBitmap>& icon) override;
@@ -122,6 +123,9 @@ class BackgroundFetchDelegateImpl
   }
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(BackgroundFetchBrowserTest,
+                           FetchesRunToCompletionAndUpdateTitle_Fetched);
+
   struct JobDetails {
     // If a job is part of the |job_details_map_|, it will have one of these
     // states.

content/browser/background_fetch/background_fetch_delegate_proxy.cc

@@ -168,6 +168,13 @@ class BackgroundFetchDelegateProxy::Core
       delegate_->Abort(job_unique_id);
   }
 
+  void MarkJobComplete(const std::string& job_unique_id) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+    if (delegate_)
+      delegate_->MarkJobComplete(job_unique_id);
+  }
+
   void UpdateUI(const std::string& job_unique_id,
                 const base::Optional<std::string>& title,
                 const base::Optional<SkBitmap>& icon) {
@@ -381,7 +388,13 @@ void BackgroundFetchDelegateProxy::Abort(const std::string& job_unique_id) {
   base::PostTaskWithTraits(
       FROM_HERE, {BrowserThread::UI},
       base::BindOnce(&Core::Abort, ui_core_ptr_, job_unique_id));
+}
 
+void BackgroundFetchDelegateProxy::MarkJobComplete(
+    const std::string& job_unique_id) {
+  base::PostTaskWithTraits(
+      FROM_HERE, {BrowserThread::UI},
+      base::BindOnce(&Core::MarkJobComplete, ui_core_ptr_, job_unique_id));
   job_details_map_.erase(job_unique_id);
 }
 

content/browser/background_fetch/background_fetch_delegate_proxy.h

@@ -111,6 +111,9 @@ class CONTENT_EXPORT BackgroundFetchDelegateProxy {
   // requests already called OnDownloadComplete.
   void Abort(const std::string& job_unique_id);
 
+  // Called when the fetch associated |job_unique_id| is completed.
+  void MarkJobComplete(const std::string& job_unique_id);
+
  private:
   class Core;
 

content/browser/background_fetch/background_fetch_delegate_proxy_unittest.cc

@@ -68,10 +68,13 @@ class FakeBackgroundFetchDelegate : public BackgroundFetchDelegate {
                          base::Unretained(this), job_unique_id, guid));
     }
   }
+
   void Abort(const std::string& job_unique_id) override {
     aborted_jobs_.insert(job_unique_id);
   }
 
+  void MarkJobComplete(const std::string& job_unique_id) override {}
+
   void UpdateUI(const std::string& job_unique_id,
                 const base::Optional<std::string>& title,
                 const base::Optional<SkBitmap>& icon) override {
@@ -257,7 +260,6 @@ TEST_F(BackgroundFetchDelegateProxyTest, Abort) {
   delegate_proxy_.Abort(kExampleUniqueId);
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_FALSE(controller.request_started_) << "Aborted job started";
   EXPECT_FALSE(controller.request_completed_) << "Aborted job completed";
   EXPECT_TRUE(controller2.request_started_) << "Normal job did not start";
   EXPECT_TRUE(controller2.request_completed_) << "Normal job did not complete";

content/browser/background_fetch/background_fetch_scheduler.cc

@@ -164,6 +164,9 @@ void BackgroundFetchScheduler::CleanupRegistration(
   // downloaded data around so long as there are references to it, and delete
   // it once there is none. We don't need to do that accounting.
   data_manager_->DeleteRegistration(registration_id, base::DoNothing());
+
+  // Notify other systems that this registration is complete.
+  delegate_proxy_->MarkJobComplete(registration_id.unique_id());
 }
 
 void BackgroundFetchScheduler::DispatchClickEvent(

content/browser/background_fetch/background_fetch_service_unittest.cc

@@ -641,6 +641,10 @@ TEST_F(BackgroundFetchServiceTest, FetchSuccessEventDispatch) {
     ASSERT_FALSE(fetches[i].response->blob->uuid.empty());
     ASSERT_GT(fetches[i].response->blob->size, 0u);
   }
+
+  auto* delegate = static_cast<MockBackgroundFetchDelegate*>(
+      browser_context()->GetBackgroundFetchDelegate());
+  EXPECT_TRUE(delegate->completed_jobs().count(registration.unique_id));
 }
 
 TEST_F(BackgroundFetchServiceTest, FetchFailEventDispatch) {
@@ -732,6 +736,10 @@ TEST_F(BackgroundFetchServiceTest, FetchFailEventDispatch) {
               blink::mojom::ServiceWorkerResponseError::kUnknown);
     EXPECT_TRUE(fetches[i].response->cors_exposed_header_names.empty());
   }
+
+  auto* delegate = static_cast<MockBackgroundFetchDelegate*>(
+      browser_context()->GetBackgroundFetchDelegate());
+  EXPECT_TRUE(delegate->completed_jobs().count(registration.unique_id));
 }
 
 TEST_F(BackgroundFetchServiceTest, UpdateUI) {
@@ -815,6 +823,10 @@ TEST_F(BackgroundFetchServiceTest, Abort) {
   GetRegistration(service_worker_registration_id, kExampleDeveloperId,
                   &second_error, &second_registration);
   ASSERT_EQ(second_error, blink::mojom::BackgroundFetchError::INVALID_ID);
+
+  auto* delegate = static_cast<MockBackgroundFetchDelegate*>(
+      browser_context()->GetBackgroundFetchDelegate());
+  EXPECT_TRUE(delegate->completed_jobs().count(registration_id.unique_id()));
 }
 
 TEST_F(BackgroundFetchServiceTest, AbortInvalidDeveloperIdArgument) {

content/browser/background_fetch/mock_background_fetch_delegate.cc

@@ -171,6 +171,11 @@ void MockBackgroundFetchDelegate::Abort(const std::string& job_unique_id) {
   aborted_jobs_.insert(job_unique_id);
 }
 
+void MockBackgroundFetchDelegate::MarkJobComplete(
+    const std::string& job_unique_id) {
+  completed_jobs_.insert(job_unique_id);
+}
+
 void MockBackgroundFetchDelegate::UpdateUI(
     const std::string& job_unique_id,
     const base::Optional<std::string>& title,

content/browser/background_fetch/mock_background_fetch_delegate.h

@@ -83,6 +83,7 @@ class MockBackgroundFetchDelegate : public BackgroundFetchDelegate {
                    const net::NetworkTrafficAnnotationTag& traffic_annotation,
                    const net::HttpRequestHeaders& headers) override;
   void Abort(const std::string& job_unique_id) override;
+  void MarkJobComplete(const std::string& job_unique_id) override;
   void UpdateUI(const std::string& job_unique_id,
                 const base::Optional<std::string>& title,
                 const base::Optional<SkBitmap>& icon) override;
@@ -90,6 +91,10 @@ class MockBackgroundFetchDelegate : public BackgroundFetchDelegate {
   void RegisterResponse(const GURL& url,
                         std::unique_ptr<TestResponse> response);
 
+  const std::set<std::string>& completed_jobs() const {
+    return completed_jobs_;
+  }
+
  private:
   // Posts (to the default task runner) a callback that is only run if the job
   // indicated by |job_unique_id| has not been aborted.
@@ -113,6 +118,9 @@ class MockBackgroundFetchDelegate : public BackgroundFetchDelegate {
   // Set of unique job ids that have been aborted.
   std::set<std::string> aborted_jobs_;
 
+  // Set of unique job ids that have been completed.
+  std::set<std::string> completed_jobs_;
+
   // Map from download GUIDs to unique job ids.
   std::map<std::string, std::string> download_guid_to_job_id_map_;
 

content/public/browser/background_fetch_delegate.h

@@ -137,6 +137,9 @@ class CONTENT_EXPORT BackgroundFetchDelegate {
   // Aborts any downloads associated with |job_unique_id|.
   virtual void Abort(const std::string& job_unique_id) = 0;
 
+  // Called after the fetch has completed so that the delegate can clean up.
+  virtual void MarkJobComplete(const std::string& job_unique_id) = 0;
+
   // Updates the UI shown for the fetch job associated with |job_unique_id| to
   // display a new |title| or |icon|.
   virtual void UpdateUI(const std::string& job_unique_id,

content/shell/browser/layout_test/layout_test_background_fetch_delegate.cc

@@ -252,6 +252,9 @@ void LayoutTestBackgroundFetchDelegate::Abort(
   // TODO(peter): Implement the ability to abort the |job_unique_id|.
 }
 
+void LayoutTestBackgroundFetchDelegate::MarkJobComplete(
+    const std::string& job_unique_id) {}
+
 void LayoutTestBackgroundFetchDelegate::UpdateUI(
     const std::string& job_unique_id,
     const base::Optional<std::string>& title,

content/shell/browser/layout_test/layout_test_background_fetch_delegate.h

@@ -40,6 +40,7 @@ class LayoutTestBackgroundFetchDelegate : public BackgroundFetchDelegate {
                    const net::NetworkTrafficAnnotationTag& traffic_annotation,
                    const net::HttpRequestHeaders& headers) override;
   void Abort(const std::string& job_unique_id) override;
+  void MarkJobComplete(const std::string& job_unique_id) override;
   void UpdateUI(const std::string& job_unique_id,
                 const base::Optional<std::string>& title,
                 const base::Optional<SkBitmap>& icon) override;