Background download: Retry when no response header is persisted.

We persist download response headers and url chain when
OnDownloadCreated is called. This CL does the following:

1. If the persist failed during ACTIVE state, delete the driver and the
file to let the download service to retry.

2. Change DownloadDriver::Remove to be able to remove file for completed
download.

3. Add did_receive_response in entry proto, since some protocol or
a socket error may cause the response headers to be empty. We should
validate the entry state with this flag instead of response headers.

TBR=carlosk@chromium.org


Bug: 883359, 881314
Change-Id: I560cc478b5d1f12c33b56b94ee97cbb1d2b76c3d
Reviewed-on: https://chromium-review.googlesource.com/1227290
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592306}

components/download/content/internal/download_driver_impl.cc

@@ -7,6 +7,7 @@
 #include <set>
 #include <vector>
 
+#include "base/bind_helpers.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/memory_usage_estimator.h"
@@ -176,22 +177,27 @@ void DownloadDriverImpl::Start(
                                  nullptr /* blob_url_loader_factory */);
 }
 
-void DownloadDriverImpl::Remove(const std::string& guid) {
+void DownloadDriverImpl::Remove(const std::string& guid, bool remove_file) {
   guid_to_remove_.emplace(guid);
 
   // DownloadItem::Remove will cause the item object removed from memory, post
   // the remove task to avoid the object being accessed in the same call stack.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&DownloadDriverImpl::DoRemoveDownload,
-                                weak_ptr_factory_.GetWeakPtr(), guid));
+      FROM_HERE,
+      base::BindOnce(&DownloadDriverImpl::DoRemoveDownload,
+                     weak_ptr_factory_.GetWeakPtr(), guid, remove_file));
 }
 
-void DownloadDriverImpl::DoRemoveDownload(const std::string& guid) {
+void DownloadDriverImpl::DoRemoveDownload(const std::string& guid,
+                                          bool remove_file) {
   if (!download_manager_)
     return;
   DownloadItem* item = download_manager_->GetDownloadByGuid(guid);
   // Cancels the download and removes the persisted records in content layer.
   if (item) {
+    // Remove the download file for completed download.
+    if (remove_file)
+      item->DeleteFile(base::DoNothing());
     item->Remove();
   }
 }
@@ -278,9 +284,9 @@ void DownloadDriverImpl::OnDownloadCreated(content::DownloadManager* manager,
     // Client has removed the download before content persistence layer created
     // the record, remove the download immediately.
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::BindOnce(&DownloadDriverImpl::DoRemoveDownload,
-                       weak_ptr_factory_.GetWeakPtr(), item->GetGuid()));
+        FROM_HERE, base::BindOnce(&DownloadDriverImpl::DoRemoveDownload,
+                                  weak_ptr_factory_.GetWeakPtr(),
+                                  item->GetGuid(), false /* remove_file */));
     return;
   }
 

components/download/content/internal/download_driver_impl.h

@@ -44,7 +44,7 @@ class DownloadDriverImpl : public DownloadDriver,
       const base::FilePath& file_path,
       scoped_refptr<network::ResourceRequestBody> post_body,
       const net::NetworkTrafficAnnotationTag& traffic_annotation) override;
-  void Remove(const std::string& guid) override;
+  void Remove(const std::string& guid, bool remove_file) override;
   void Pause(const std::string& guid) override;
   void Resume(const std::string& guid) override;
   base::Optional<DriverEntry> Find(const std::string& guid) override;
@@ -65,7 +65,7 @@ class DownloadDriverImpl : public DownloadDriver,
   void OnHardRecoverComplete(bool success);
 
   // Remove the download, used to be posted to the task queue.
-  void DoRemoveDownload(const std::string& guid);
+  void DoRemoveDownload(const std::string& guid, bool remove_file);
 
   // Low level download handle.
   content::DownloadManager* download_manager_;

components/download/content/internal/download_driver_impl_unittest.cc

@@ -116,7 +116,7 @@ TEST_F(DownloadDriverImplTest, RemoveBeforeCreated) {
   // Download is not created yet in content layer.
   ON_CALL(mock_manager_, GetDownloadByGuid(kTestGuid))
       .WillByDefault(Return(nullptr));
-  driver_->Remove(kTestGuid);
+  driver_->Remove(kTestGuid, false);
   task_runner_->RunUntilIdle();
 
   // Download is created in content layer.

components/download/internal/background_service/controller_impl.cc

@@ -429,6 +429,7 @@ void ControllerImpl::OnDownloadCreated(const DriverEntry& download) {
 
   entry->url_chain = download.url_chain;
   entry->response_headers = download.response_headers;
+  entry->did_received_response = true;
   model_->Update(*entry);
 
   download::Client* client = clients_->GetClient(entry->client);
@@ -565,7 +566,7 @@ void ControllerImpl::OnItemUpdated(bool success,
   // DriverEntry.  If we restart we'll see a COMPLETE state and handle it
   // accordingly.
   if (entry->state == Entry::State::COMPLETE)
-    driver_->Remove(guid);
+    driver_->Remove(guid, false);
 
   // TODO(dtrainor): If failed, clean up any download state accordingly.
 
@@ -731,7 +732,7 @@ void ControllerImpl::CancelOrphanedRequests() {
   }
 
   for (const auto& guid : guids_to_remove) {
-    driver_->Remove(guid);
+    driver_->Remove(guid, false);
     model_->Remove(guid);
   }
 
@@ -794,6 +795,15 @@ void ControllerImpl::ResolveInitialRequestStates() {
           break;
         }
 
+        // We didn't persist the response headers in time, so just restart the
+        // the fetch. The driver entry and the temporary file will also be
+        // deleted.
+        if (state == Entry::State::ACTIVE && entry->require_response_headers &&
+            !entry->did_received_response) {
+          driver_->Remove(entry->guid, true /* remove_file */);
+          break;
+        }
+
         // If we have a real DriverEntry::State, we need to determine which of
         // those states makes sense for our Entry.  Our entry can either be in
         // two states now: It's effective 'active' state (ACTIVE or PAUSED) or
@@ -841,7 +851,7 @@ void ControllerImpl::ResolveInitialRequestStates() {
       case Entry::State::AVAILABLE:  // Intentional fallthrough.
         // We should not have a DriverEntry here.
         if (driver_entry.has_value())
-          driver_->Remove(entry->guid);
+          driver_->Remove(entry->guid, false);
         break;
       case Entry::State::ACTIVE:  // Intentional fallthrough.
       case Entry::State::PAUSED:
@@ -866,7 +876,7 @@ void ControllerImpl::ResolveInitialRequestStates() {
         } else {
           // We're staying in COMPLETE.  Make sure there is no DriverEntry here.
           if (driver_entry.has_value())
-            driver_->Remove(entry->guid);
+            driver_->Remove(entry->guid, false);
         }
         break;
       case Entry::State::COUNT:
@@ -1128,7 +1138,8 @@ void ControllerImpl::HandleCompleteDownload(CompletionType type,
     entry->completion_time = driver_entry->completion_time;
     entry->bytes_downloaded = driver_entry->bytes_downloaded;
     CompletionInfo completion_info(driver_entry->current_file_path,
-                                   driver_entry->bytes_downloaded);
+                                   driver_entry->bytes_downloaded,
+                                   entry->url_chain, entry->response_headers);
     completion_info.blob_handle = driver_entry->blob_handle;
 
     entry->last_cleanup_check_time = driver_entry->completion_time;
@@ -1147,7 +1158,7 @@ void ControllerImpl::HandleCompleteDownload(CompletionType type,
 
     // TODO(dtrainor): Handle the case where we crash before the model write
     // happens and we have no driver entry.
-    driver_->Remove(entry->guid);
+    driver_->Remove(entry->guid, false);
     model_->Remove(guid);
   }
 

components/download/internal/background_service/controller_impl_unittest.cc

@@ -1007,7 +1007,9 @@ TEST_F(DownloadServiceControllerImplTest, OnDownloadSucceeded) {
   DriverEntry dentry = BuildDriverEntry(entry, DriverEntry::State::IN_PROGRESS);
   driver_->AddTestData(std::vector<DriverEntry>{dentry});
 
-  CompletionInfo completion_info(base::FilePath::FromUTF8Unsafe("123"), 1024u);
+  CompletionInfo completion_info(dentry.current_file_path,
+                                 dentry.bytes_downloaded, entry.url_chain,
+                                 entry.response_headers);
   EXPECT_CALL(*client_, OnServiceInitialized(false, _)).Times(1);
   EXPECT_CALL(*client_, OnDownloadSucceeded(entry.guid, completion_info))
       .Times(1);
@@ -1560,6 +1562,44 @@ TEST_F(DownloadServiceControllerImplTest, StartupRecoveryForUploadEntries) {
   verify_entry(entries[14].guid, Entry::State::COMPLETE, base::nullopt);
 }
 
+// Download driver will remove the download if failed to persist the response
+// headers.
+TEST_F(DownloadServiceControllerImplTest, StartupRecoveryNoResponseHeaders) {
+  std::vector<Entry> entries;
+  std::vector<DriverEntry> driver_entries;
+  entries.push_back(test::BuildBasicEntry(Entry::State::ACTIVE));
+  entries.back().response_headers = nullptr;
+  entries.back().did_received_response = false;
+
+  driver_entries.push_back(
+      BuildDriverEntry(entries.back(), DriverEntry::State::IN_PROGRESS));
+
+  device_status_listener_->SetDeviceStatus(
+      DeviceStatus(BatteryStatus::CHARGING, NetworkStatus::UNMETERED));
+
+  InitializeController();
+  driver_->AddTestData(driver_entries);
+  driver_->MakeReady();
+  store_->AutomaticallyTriggerAllFutureCallbacks(true);
+  store_->TriggerInit(true, std::make_unique<std::vector<Entry>>(entries));
+  file_monitor_->TriggerInit(true);
+
+  // Verify that the driver entry will be removed without response headers.
+  EXPECT_FALSE(driver_->Find(entries[0].guid).has_value());
+
+  task_runner_->RunUntilIdle();
+
+  // The download is retried.
+  EXPECT_EQ(Entry::State::ACTIVE, model_->Get(entries[0].guid)->state);
+  EXPECT_TRUE(model_->Get(entries[0].guid)->did_received_response);
+  auto expected_driver_entry = driver_->Find(entries[0].guid);
+  EXPECT_TRUE(expected_driver_entry.has_value());
+  EXPECT_EQ(model_->Get(entries[0].guid)->url_chain,
+            expected_driver_entry->url_chain);
+  EXPECT_EQ(model_->Get(entries[0].guid)->response_headers->raw_headers(),
+            expected_driver_entry->response_headers->raw_headers());
+}
+
 TEST_F(DownloadServiceControllerImplTest, ExistingExternalDownload) {
   Entry entry1 = test::BuildBasicEntry(Entry::State::ACTIVE);
   Entry entry2 = test::BuildBasicEntry(Entry::State::ACTIVE);

components/download/internal/background_service/download_driver.h

@@ -92,8 +92,10 @@ class DownloadDriver : public MemoryTracker {
       const net::NetworkTrafficAnnotationTag& traffic_annotation) = 0;
 
   // Cancels an existing download, all data associated with this download should
-  // be removed.
-  virtual void Remove(const std::string& guid) = 0;
+  // be removed. If download is not completed, the temporary file will be
+  // deleted. If download is completed, the download file will be deleted when
+  // |remove_file| is true.
+  virtual void Remove(const std::string& guid, bool remove_file) = 0;
 
   // Pauses the download.
   virtual void Pause(const std::string& guid) = 0;

components/download/internal/background_service/entry.cc

@@ -24,7 +24,9 @@ Entry::Entry()
       attempt_count(0),
       resumption_count(0),
       cleanup_attempt_count(0),
-      has_upload_data(false) {}
+      has_upload_data(false),
+      did_received_response(false),
+      require_response_headers(true) {}
 Entry::Entry(const Entry& other) = default;
 
 Entry::Entry(const DownloadParams& params)
@@ -38,7 +40,9 @@ Entry::Entry(const DownloadParams& params)
       resumption_count(0),
       cleanup_attempt_count(0),
       has_upload_data(false),
-      traffic_annotation(params.traffic_annotation) {}
+      traffic_annotation(params.traffic_annotation),
+      did_received_response(false),
+      require_response_headers(true) {}
 
 Entry::~Entry() = default;
 
@@ -65,7 +69,10 @@ bool Entry::operator==(const Entry& other) const {
          has_upload_data == other.has_upload_data &&
          traffic_annotation == other.traffic_annotation &&
          url_chain == other.url_chain &&
-         AreHeadersEqual(response_headers.get(), other.response_headers.get());
+         AreHeadersEqual(response_headers.get(),
+                         other.response_headers.get()) &&
+         did_received_response == other.did_received_response &&
+         require_response_headers == other.require_response_headers;
 }
 
 size_t Entry::EstimateMemoryUsage() const {

components/download/internal/background_service/entry.h

@@ -114,6 +114,14 @@ struct Entry {
 
   // The response headers for the download request.
   scoped_refptr<const net::HttpResponseHeaders> response_headers;
+
+  // If the response is received. |response_headers| may be null in the response
+  // for certain protocol, or without network connection.
+  bool did_received_response;
+
+  // If the download requires response headers to be persisted. False for older
+  // proto version.
+  bool require_response_headers;
 };
 
 }  // namespace download

components/download/internal/background_service/entry_utils.cc

@@ -94,9 +94,8 @@ DownloadMetaData BuildDownloadMetaData(Entry* entry, DownloadDriver* driver) {
   meta_data.guid = entry->guid;
   if (entry->state == Entry::State::COMPLETE) {
     meta_data.completion_info =
-        CompletionInfo(entry->target_file_path, entry->bytes_downloaded);
-    meta_data.completion_info->response_headers = entry->response_headers;
-    meta_data.completion_info->url_chain = entry->url_chain;
+        CompletionInfo(entry->target_file_path, entry->bytes_downloaded,
+                       entry->url_chain, entry->response_headers);
     // If the download is completed, the |current_size| needs to pull from entry
     // since the history db record has been deleted.
     meta_data.current_size = entry->bytes_downloaded;

components/download/internal/background_service/in_memory_download_driver.cc

@@ -110,7 +110,7 @@ void InMemoryDownloadDriver::Start(
   download_ptr->Start();
 }
 
-void InMemoryDownloadDriver::Remove(const std::string& guid) {
+void InMemoryDownloadDriver::Remove(const std::string& guid, bool remove_file) {
   downloads_.erase(guid);
 }
 

components/download/internal/background_service/in_memory_download_driver.h

@@ -64,7 +64,7 @@ class InMemoryDownloadDriver : public DownloadDriver,
       const base::FilePath& file_path,
       scoped_refptr<network::ResourceRequestBody> post_body,
       const net::NetworkTrafficAnnotationTag& traffic_annotation) override;
-  void Remove(const std::string& guid) override;
+  void Remove(const std::string& guid, bool remove_file) override;
   void Pause(const std::string& guid) override;
   void Resume(const std::string& guid) override;
   base::Optional<DriverEntry> Find(const std::string& guid) override;

components/download/internal/background_service/in_memory_download_driver_unittest.cc

@@ -163,7 +163,7 @@ TEST_F(InMemoryDownloadDriverTest, DownloadSuccessAndRemove) {
   EXPECT_EQ(guid, entry->guid);
   EXPECT_EQ(DriverEntry::State::COMPLETE, entry->state);
 
-  driver()->Remove(guid);
+  driver()->Remove(guid, false);
   entry = driver()->Find(guid);
   EXPECT_FALSE(entry.has_value());
 }

components/download/internal/background_service/proto/entry.proto

@@ -71,4 +71,6 @@ message Entry {
 
   repeated string url_chain = 16;
   optional string response_headers = 17;
+  optional bool did_received_response = 18;
+  optional bool require_response_headers = 19;
 }

components/download/internal/background_service/proto_conversions.cc

@@ -296,6 +296,8 @@ Entry ProtoConversions::EntryFromProto(const protodb::Entry& proto) {
         base::MakeRefCounted<const net::HttpResponseHeaders>(
             proto.response_headers());
   }
+  entry.did_received_response = proto.did_received_response();
+  entry.require_response_headers = proto.require_response_headers();
 
   return entry;
 }
@@ -324,6 +326,9 @@ protodb::Entry ProtoConversions::EntryToProto(const Entry& entry) {
     proto.add_url_chain(url.spec());
   if (entry.response_headers)
     proto.set_response_headers(entry.response_headers->raw_headers());
+  proto.set_did_received_response(entry.did_received_response);
+  proto.set_require_response_headers(entry.require_response_headers);
+
   return proto;
 }
 

components/download/internal/background_service/test/entry_utils.cc

@@ -54,6 +54,7 @@ Entry BuildBasicEntry(Entry::State state) {
     entry.response_headers =
         base::MakeRefCounted<const net::HttpResponseHeaders>(
             "HTTP/1.1 200 OK\nContent-type: text/html\n\n");
+    entry.did_received_response = true;
   }
   return entry;
 }
@@ -103,6 +104,7 @@ Entry BuildEntry(DownloadClient client,
   entry.url_chain = {url, url};
   entry.response_headers = base::MakeRefCounted<const net::HttpResponseHeaders>(
       "HTTP/1.1 200 OK\nContent-type: text/html\n\n");
+  entry.did_received_response = true;
   return entry;
 }
 

components/download/internal/background_service/test/test_download_driver.cc

@@ -81,6 +81,7 @@ void TestDownloadDriver::Start(
   entry.paused = false;
   entry.bytes_downloaded = 0;
   entry.expected_total_size = 0;
+  entry.url_chain = {params.url};
   entry.response_headers =
       base::MakeRefCounted<net::HttpResponseHeaders>("HTTP/1.1 200");
   entries_[guid] = entry;
@@ -89,7 +90,7 @@ void TestDownloadDriver::Start(
     client_->OnDownloadCreated(entry);
 }
 
-void TestDownloadDriver::Remove(const std::string& guid) {
+void TestDownloadDriver::Remove(const std::string& guid, bool remove_file) {
   entries_.erase(guid);
 }
 

components/download/internal/background_service/test/test_download_driver.h

@@ -44,7 +44,7 @@ class TestDownloadDriver : public DownloadDriver {
       const base::FilePath& file_path,
       scoped_refptr<network::ResourceRequestBody> post_body,
       const net::NetworkTrafficAnnotationTag& traffic_annotation) override;
-  void Remove(const std::string& guid) override;
+  void Remove(const std::string& guid, bool remove_file) override;
   void Pause(const std::string& guid) override;
   void Resume(const std::string& guid) override;
   base::Optional<DriverEntry> Find(const std::string& guid) override;

components/download/public/background_service/download_metadata.cc

@@ -4,11 +4,28 @@
 
 #include "components/download/public/background_service/download_metadata.h"
 
+namespace {
+
+bool AreResponseHeadersEqual(const net::HttpResponseHeaders* h1,
+                             const net::HttpResponseHeaders* h2) {
+  if (h1 && h2)
+    return h1->raw_headers() == h2->raw_headers();
+  return !h1 && !h2;
+}
+
+}  // namespace
+
 namespace download {
 
-CompletionInfo::CompletionInfo(const base::FilePath& path,
-                               uint64_t bytes_downloaded)
-    : path(path), bytes_downloaded(bytes_downloaded) {}
+CompletionInfo::CompletionInfo(
+    const base::FilePath& path,
+    uint64_t bytes_downloaded,
+    const std::vector<GURL>& url_chain,
+    scoped_refptr<const net::HttpResponseHeaders> response_headers)
+    : path(path),
+      bytes_downloaded(bytes_downloaded),
+      url_chain(url_chain),
+      response_headers(std::move(response_headers)) {}
 
 CompletionInfo::CompletionInfo(const CompletionInfo& other) = default;
 
@@ -16,7 +33,10 @@ CompletionInfo::~CompletionInfo() = default;
 
 bool CompletionInfo::operator==(const CompletionInfo& other) const {
   // The blob data handle is not compared here.
-  return path == other.path && bytes_downloaded == other.bytes_downloaded;
+  return path == other.path && bytes_downloaded == other.bytes_downloaded &&
+         url_chain == other.url_chain &&
+         AreResponseHeadersEqual(response_headers.get(),
+                                 other.response_headers.get());
 }
 
 DownloadMetaData::DownloadMetaData() : current_size(0u) {}

components/download/public/background_service/download_metadata.h

@@ -40,7 +40,11 @@ struct CompletionInfo {
   // account changed values during retries/resumptions.
   scoped_refptr<const net::HttpResponseHeaders> response_headers;
 
-  CompletionInfo(const base::FilePath& path, uint64_t bytes_downloaded);
+  CompletionInfo(
+      const base::FilePath& path,
+      uint64_t bytes_downloaded,
+      const std::vector<GURL>& url_chain,
+      scoped_refptr<const net::HttpResponseHeaders> response_headers);
   CompletionInfo(const CompletionInfo& other);
   ~CompletionInfo();
   bool operator==(const CompletionInfo& other) const;

components/download/public/background_service/test/test_download_service.cc

@@ -138,7 +138,8 @@ void TestDownloadService::ProcessDownload() {
   if (!failed_download_id_.empty() && params.guid == failed_download_id_) {
     OnDownloadFailed(params.guid, Client::FailureReason::ABORTED);
   } else {
-    CompletionInfo completion_info(base::FilePath(), file_size_);
+    CompletionInfo completion_info(base::FilePath(), file_size_,
+                                   {params.request_params.url}, nullptr);
     OnDownloadSucceeded(params.guid, completion_info);
   }
 }

components/download/public/common/download_item.h

@@ -347,7 +347,7 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadItem : public base::SupportsUserData {
   // If the file is successfully deleted, then GetFileExternallyRemoved() will
   // become true, GetFullPath() will become empty, and
   // DownloadItem::OnDownloadUpdated() will be called. Does nothing if
-  // GetState() == COMPLETE or GetFileExternallyRemoved() is already true or
+  // GetState() != COMPLETE or GetFileExternallyRemoved() is already true or
   // GetFullPath() is already empty. The callback is always run, and it is
   // always run asynchronously. It will be passed true if the file is
   // successfully deleted or if GetFilePath() was already empty or if

components/offline_pages/core/prefetch/test_download_service.cc

@@ -69,7 +69,8 @@ void TestDownloadService::FinishDownload(const std::string& guid) {
   const int file_size = static_cast<int>(test_file_data_.size());
   CHECK_EQ(file_size, base::WriteFile(path, test_file_data_.data(), file_size));
   client_->OnDownloadSucceeded(
-      guid, download::CompletionInfo(path, test_file_data_.size()));
+      guid, download::CompletionInfo(path, test_file_data_.size(),
+                                     std::vector<GURL>(), nullptr));
 }
 
 void TestDownloadService::SetTestFileData(const std::string& data) {