Pause DownloadService with external downloads

- If there are external downloads active, pause the DownloadService
downloads to not disrupt typical user usage.
- This also means updating DownloadDriver to expose all active downloads
so that on startup the DownloadService can properly set it's state.

Bug: 
Change-Id: Icf3204a1cedafeb9565fc577dc2c4cda3ad283b4
Reviewed-on: https://chromium-review.googlesource.com/537966
Commit-Queue: David Trainor <dtrainor@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480290}

components/download/content/internal/download_driver_impl.cc

@@ -4,6 +4,9 @@
 
 #include "components/download/content/internal/download_driver_impl.h"
 
+#include <set>
+#include <vector>
+
 #include "components/download/internal/driver_entry.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_url_parameters.h"
@@ -152,6 +155,23 @@ base::Optional<DriverEntry> DownloadDriverImpl::Find(const std::string& guid) {
   return base::nullopt;
 }
 
+std::set<std::string> DownloadDriverImpl::GetActiveDownloads() {
+  std::set<std::string> guids;
+  if (!download_manager_)
+    return guids;
+
+  std::vector<content::DownloadItem*> items;
+  download_manager_->GetAllDownloads(&items);
+
+  for (auto* item : items) {
+    DriverEntry::State state = ToDriverEntryState(item->GetState());
+    if (state == DriverEntry::State::IN_PROGRESS)
+      guids.insert(item->GetGuid());
+  }
+
+  return guids;
+}
+
 void DownloadDriverImpl::OnDownloadUpdated(content::DownloadItem* item) {
   DCHECK(client_);
 
@@ -168,6 +188,8 @@ void DownloadDriverImpl::OnDownloadUpdated(content::DownloadItem* item) {
   } else if (reason !=
              content::DownloadInterruptReason::DOWNLOAD_INTERRUPT_REASON_NONE) {
     client_->OnDownloadFailed(entry, static_cast<int>(reason));
+    // TODO(dtrainor, xingliu): This actually might not be correct.  What if we
+    // restart the download?
     item->RemoveObserver(this);
   }
 }

components/download/content/internal/download_driver_impl.h

@@ -43,6 +43,7 @@ class DownloadDriverImpl : public DownloadDriver,
   void Pause(const std::string& guid) override;
   void Resume(const std::string& guid) override;
   base::Optional<DriverEntry> Find(const std::string& guid) override;
+  std::set<std::string> GetActiveDownloads() override;
 
  private:
   // content::DownloadItem::Observer implementation.

components/download/content/internal/download_driver_impl_unittest.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <string>
 
+#include "base/guid.h"
 #include "base/memory/ptr_util.h"
 #include "content/public/test/fake_download_item.h"
 #include "content/public/test/mock_download_manager.h"
@@ -15,6 +16,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
+using testing::Invoke;
 using testing::NiceMock;
 using testing::Return;
 
@@ -22,6 +24,10 @@ namespace download {
 
 namespace {
 
+ACTION_P(PopulateVector, items) {
+  arg0->insert(arg0->begin(), items.begin(), items.end());
+}
+
 const char kFakeGuid[] = "fake_guid";
 
 // Matcher to compare driver entries. Not all the memeber fields are compared.
@@ -126,4 +132,39 @@ TEST_F(DownloadDriverImplTest, DownloadItemUpdateEvents) {
       ->OnDownloadUpdated(&fake_item);
 }
 
+TEST_F(DownloadDriverImplTest, TestGetActiveDownloadsCall) {
+  using DownloadState = content::DownloadItem::DownloadState;
+  content::FakeDownloadItem item1;
+  item1.SetState(DownloadState::IN_PROGRESS);
+  item1.SetGuid(base::GenerateGUID());
+
+  content::FakeDownloadItem item2;
+  item2.SetState(DownloadState::CANCELLED);
+  item2.SetGuid(base::GenerateGUID());
+
+  content::FakeDownloadItem item3;
+  item3.SetState(DownloadState::COMPLETE);
+  item3.SetGuid(base::GenerateGUID());
+
+  content::FakeDownloadItem item4;
+  item4.SetState(DownloadState::INTERRUPTED);
+  item4.SetGuid(base::GenerateGUID());
+
+  std::vector<content::DownloadItem*> items{&item1, &item2, &item3, &item4};
+
+  ON_CALL(mock_manager_, GetAllDownloads(_))
+      .WillByDefault(PopulateVector(items));
+
+  EXPECT_CALL(mock_manager_, IsManagerInitialized())
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_client_, OnDriverReady(true)).Times(1);
+  driver_->Initialize(&mock_client_);
+
+  auto guids = driver_->GetActiveDownloads();
+
+  EXPECT_EQ(1U, guids.size());
+  EXPECT_NE(guids.end(), guids.find(item1.GetGuid()));
+}
+
 }  // namespace download

components/download/internal/controller_impl.cc

@@ -73,6 +73,7 @@ ControllerImpl::ControllerImpl(
       device_status_listener_(std::move(device_status_listener)),
       scheduler_(std::move(scheduler)),
       task_scheduler_(std::move(task_scheduler)),
+      initializing_internals_(false),
       weak_ptr_factory_(this) {}
 
 ControllerImpl::~ControllerImpl() = default;
@@ -80,6 +81,7 @@ ControllerImpl::~ControllerImpl() = default;
 void ControllerImpl::Initialize() {
   DCHECK(!startup_status_.Complete());
 
+  initializing_internals_ = true;
   driver_->Initialize(this);
   model_->Initialize(this);
 }
@@ -260,10 +262,13 @@ void ControllerImpl::OnDriverReady(bool success) {
 }
 
 void ControllerImpl::OnDownloadCreated(const DriverEntry& download) {
+  if (initializing_internals_)
+    return;
+
   Entry* entry = model_->Get(download.guid);
 
   if (!entry) {
-    // TODO(xingliu): Log non download service initiated downloads.
+    HandleExternalDownload(download.guid, true);
     return;
   }
 
@@ -278,35 +283,54 @@ void ControllerImpl::OnDownloadCreated(const DriverEntry& download) {
 }
 
 void ControllerImpl::OnDownloadFailed(const DriverEntry& download, int reason) {
+  if (initializing_internals_)
+    return;
+
   Entry* entry = model_->Get(download.guid);
-  if (!entry)
+  if (!entry) {
+    HandleExternalDownload(download.guid, false);
     return;
+  }
 
   // TODO(dtrainor): Add retry logic here.  Connect to restart code for tracking
   // number of retries.
 
+  // TODO(dtrainor, xingliu): We probably have to prevent cancel calls from
+  // coming through here as we remove downloads (especially through
+  // initialization).
   HandleCompleteDownload(CompletionType::FAIL, download.guid);
 }
 
 void ControllerImpl::OnDownloadSucceeded(const DriverEntry& download,
                                          const base::FilePath& path) {
+  if (initializing_internals_)
+    return;
+
   Entry* entry = model_->Get(download.guid);
-  if (!entry)
+  if (!entry) {
+    HandleExternalDownload(download.guid, false);
     return;
+  }
 
   HandleCompleteDownload(CompletionType::SUCCEED, download.guid);
 }
 
 void ControllerImpl::OnDownloadUpdated(const DriverEntry& download) {
+  if (initializing_internals_)
+    return;
+
   Entry* entry = model_->Get(download.guid);
-  if (!entry)
+  if (!entry) {
+    HandleExternalDownload(download.guid, !download.paused);
     return;
+  }
 
   DCHECK_EQ(download.state, DriverEntry::State::IN_PROGRESS);
 
-  download::Client* client = clients_->GetClient(entry->client);
-  if (client)
-    client->OnDownloadUpdated(download.guid, download.bytes_downloaded);
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(&ControllerImpl::SendOnDownloadUpdated,
+                            weak_ptr_factory_.GetWeakPtr(), entry->client,
+                            download.guid, download.bytes_downloaded));
 }
 
 void ControllerImpl::OnModelReady(bool success) {
@@ -379,16 +403,29 @@ void ControllerImpl::AttemptToFinalizeSetup() {
   }
 
   device_status_listener_->Start(this);
+  PollActiveDriverDownloads();
   CancelOrphanedRequests();
   ResolveInitialRequestStates();
-  UpdateDriverStates();
   NotifyClientsOfStartup();
+
+  initializing_internals_ = false;
+
+  UpdateDriverStates();
   ProcessScheduledTasks();
 
   // Pull the initial straw if active downloads haven't reach maximum.
   ActivateMoreDownloads();
 }
 
+void ControllerImpl::PollActiveDriverDownloads() {
+  std::set<std::string> guids = driver_->GetActiveDownloads();
+
+  for (auto guid : guids) {
+    if (!model_->Get(guid))
+      externally_active_downloads_.insert(guid);
+  }
+}
+
 void ControllerImpl::CancelOrphanedRequests() {
   auto entries = model_->PeekEntries();
 
@@ -399,9 +436,9 @@ void ControllerImpl::CancelOrphanedRequests() {
   }
 
   for (const auto& guid : guids_to_remove) {
-    model_->Remove(guid);
     // TODO(xingliu): Use file monitor to delete the files.
     driver_->Remove(guid);
+    model_->Remove(guid);
   }
 }
 
@@ -527,6 +564,13 @@ void ControllerImpl::UpdateDriverStates() {
 }
 
 void ControllerImpl::UpdateDriverState(const Entry& entry) {
+  DCHECK(!initializing_internals_);
+
+  if (entry.state != Entry::State::ACTIVE &&
+      entry.state != Entry::State::PAUSED) {
+    return;
+  }
+
   // This method will need to figure out what to do with a failed download and
   // either a) restart it or b) fail the download.
 
@@ -535,35 +579,23 @@ void ControllerImpl::UpdateDriverState(const Entry& entry) {
   bool meets_device_criteria = device_status_listener_->CurrentDeviceStatus()
                                    .MeetsCondition(entry.scheduling_params)
                                    .MeetsRequirements();
-  switch (entry.state) {
-    case Entry::State::ACTIVE:
-      if (!meets_device_criteria) {
-        driver_->Pause(entry.guid);
-        break;
-      }
-      // Start or resume the download if it should be running.
-      if (!driver_entry.has_value()) {
-        driver_->Start(entry.request_params, entry.guid,
-                       NO_TRAFFIC_ANNOTATION_YET);
-        break;
-      }
-      if (driver_entry->state != DriverEntry::State::IN_PROGRESS) {
-        driver_->Resume(entry.guid);
-      }
-      break;
-    case Entry::State::PAUSED:
-      // Pause the in progress downloads that should not be running.
-      if (driver_entry.has_value() &&
-          driver_entry->state == DriverEntry::State::IN_PROGRESS) {
-        driver_->Pause(entry.guid);
-      }
-      break;
-    case Entry::State::AVAILABLE:  // Intentional fallthrough.
-    case Entry::State::NEW:        // Intentional fallthrough.
-    case Entry::State::COMPLETE:   // Intentional fallthrough.
-      break;
-    default:
-      NOTREACHED();
+  bool force_pause =
+      !externally_active_downloads_.empty() &&
+      entry.scheduling_params.priority != SchedulingParams::Priority::UI;
+  bool entry_paused = entry.state == Entry::State::PAUSED;
+
+  bool pause_driver = entry_paused || force_pause || !meets_device_criteria;
+
+  if (pause_driver) {
+    if (driver_entry.has_value())
+      driver_->Pause(entry.guid);
+  } else {
+    if (driver_entry.has_value()) {
+      driver_->Resume(entry.guid);
+    } else {
+      driver_->Start(entry.request_params, entry.guid,
+                     NO_TRAFFIC_ANNOTATION_YET);
+    }
   }
 }
 
@@ -640,6 +672,9 @@ void ControllerImpl::HandleCompleteDownload(CompletionType type,
 }
 
 void ControllerImpl::ActivateMoreDownloads() {
+  if (initializing_internals_)
+    return;
+
   // TODO(xingliu): Check the configuration to throttle downloads.
   Entry* next = scheduler_->Next(
       model_->PeekEntries(), device_status_listener_->CurrentDeviceStatus());
@@ -654,6 +689,28 @@ void ControllerImpl::ActivateMoreDownloads() {
   scheduler_->Reschedule(model_->PeekEntries());
 }
 
+void ControllerImpl::HandleExternalDownload(const std::string& guid,
+                                            bool active) {
+  if (active) {
+    externally_active_downloads_.insert(guid);
+  } else {
+    externally_active_downloads_.erase(guid);
+  }
+
+  UpdateDriverStates();
+}
+
+void ControllerImpl::SendOnDownloadUpdated(DownloadClient client_id,
+                                           const std::string& guid,
+                                           uint64_t bytes_downloaded) {
+  if (!model_->Get(guid))
+    return;
+
+  auto* client = clients_->GetClient(client_id);
+  DCHECK(client);
+  client->OnDownloadUpdated(guid, bytes_downloaded);
+}
+
 void ControllerImpl::SendOnDownloadSucceeded(DownloadClient client_id,
                                              const std::string& guid,
                                              const base::FilePath& path,

components/download/internal/controller_impl.h

@@ -7,6 +7,7 @@
 
 #include <map>
 #include <memory>
+#include <set>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -91,6 +92,10 @@ class ControllerImpl : public Controller,
   // internal state initialization.
   void AttemptToFinalizeSetup();
 
+  // Checks for all the currently active driver downloads.  This lets us know
+  // which ones are active that we haven't tracked.
+  void PollActiveDriverDownloads();
+
   // Cancels and cleans upany requests that are no longer associated with a
   // Client in |clients_|.
   void CancelOrphanedRequests();
@@ -135,8 +140,13 @@ class ControllerImpl : public Controller,
   // reached maximum.
   void ActivateMoreDownloads();
 
+  void HandleExternalDownload(const std::string& guid, bool active);
+
   // Postable methods meant to just be pass throughs to Client APIs.  This is
   // meant to help prevent reentrancy.
+  void SendOnDownloadUpdated(DownloadClient client_id,
+                             const std::string& guid,
+                             uint64_t bytes_downloaded);
   void SendOnDownloadSucceeded(DownloadClient client_id,
                                const std::string& guid,
                                const base::FilePath& path,
@@ -156,7 +166,13 @@ class ControllerImpl : public Controller,
   std::unique_ptr<TaskScheduler> task_scheduler_;
 
   // Internal state.
+  // Is set to true if this class is currently in the process of initializing
+  // it's internal state.  This will be false until |startup_status_| signals it
+  // is complete *and* all internal structures are set up.  This is to prevent
+  // outside signals from triggering state updates before we are ready.
+  bool initializing_internals_;
   StartupStatus startup_status_;
+  std::set<std::string> externally_active_downloads_;
   std::map<std::string, DownloadParams::StartCallback> start_callbacks_;
   std::map<DownloadTaskType, TaskFinishedCallback> task_finished_callbacks_;
 

components/download/internal/controller_impl_unittest.cc

@@ -620,6 +620,8 @@ TEST_F(DownloadServiceControllerImplTest, OnDownloadUpdated) {
               OnDownloadUpdated(entry.guid, driver_entry.bytes_downloaded));
   driver_->NotifyDownloadUpdate(driver_entry);
   EXPECT_EQ(Entry::State::ACTIVE, model_->Get(entry.guid)->state);
+
+  task_runner_->RunUntilIdle();
 }
 
 TEST_F(DownloadServiceControllerImplTest, DownloadCompletionTest) {
@@ -829,4 +831,130 @@ TEST_F(DownloadServiceControllerImplTest, StartupRecovery) {
   EXPECT_EQ(base::nullopt, driver_->Find(entries[24].guid));
 }
 
+TEST_F(DownloadServiceControllerImplTest, ExistingExternalDownload) {
+  Entry entry1 = test::BuildBasicEntry(Entry::State::ACTIVE);
+  Entry entry2 = test::BuildBasicEntry(Entry::State::ACTIVE);
+  Entry entry3 = test::BuildBasicEntry(Entry::State::ACTIVE);
+  entry3.scheduling_params.priority = SchedulingParams::Priority::UI;
+
+  // Simulate an existing download the service knows about and one it does not.
+  DriverEntry dentry1 =
+      BuildDriverEntry(entry2, DriverEntry::State::IN_PROGRESS);
+  DriverEntry dentry2;
+  dentry2.guid = base::GenerateGUID();
+  dentry2.state = DriverEntry::State::IN_PROGRESS;
+
+  std::vector<Entry> entries = {entry1, entry2, entry3};
+  std::vector<DriverEntry> dentries = {dentry1, dentry2};
+
+  EXPECT_CALL(*client_, OnServiceInitialized(_)).Times(1);
+
+  // Set up the Controller.
+  device_status_listener_->SetDeviceStatus(
+      DeviceStatus(BatteryStatus::CHARGING, NetworkStatus::UNMETERED));
+
+  driver_->AddTestData(dentries);
+  controller_->Initialize();
+  store_->TriggerInit(true, base::MakeUnique<std::vector<Entry>>(entries));
+  driver_->MakeReady();
+  task_runner_->RunUntilIdle();
+
+  EXPECT_EQ(Entry::State::ACTIVE, model_->Get(entry1.guid)->state);
+  EXPECT_EQ(Entry::State::ACTIVE, model_->Get(entry2.guid)->state);
+  EXPECT_EQ(Entry::State::ACTIVE, model_->Get(entry3.guid)->state);
+
+  EXPECT_FALSE(driver_->Find(entry1.guid).has_value());
+
+  EXPECT_TRUE(driver_->Find(entry2.guid).has_value());
+  EXPECT_TRUE(driver_->Find(entry2.guid).value().paused);
+
+  EXPECT_TRUE(driver_->Find(entry3.guid).has_value());
+  EXPECT_FALSE(driver_->Find(entry3.guid).value().paused);
+
+  // Simulate a successful external download.
+  driver_->NotifyDownloadSucceeded(dentry2, base::FilePath());
+
+  EXPECT_TRUE(driver_->Find(entry1.guid).has_value());
+  EXPECT_FALSE(driver_->Find(entry1.guid).value().paused);
+  EXPECT_FALSE(driver_->Find(entry2.guid).value().paused);
+  EXPECT_FALSE(driver_->Find(entry3.guid).value().paused);
+}
+
+TEST_F(DownloadServiceControllerImplTest, NewExternalDownload) {
+  Entry entry1 = test::BuildBasicEntry(Entry::State::ACTIVE);
+  Entry entry2 = test::BuildBasicEntry(Entry::State::ACTIVE);
+  entry2.scheduling_params.priority = SchedulingParams::Priority::UI;
+
+  DriverEntry dentry1 =
+      BuildDriverEntry(entry2, DriverEntry::State::IN_PROGRESS);
+
+  std::vector<Entry> entries = {entry1, entry2};
+  std::vector<DriverEntry> dentries = {dentry1};
+
+  EXPECT_CALL(*client_, OnServiceInitialized(_)).Times(1);
+
+  // Set up the Controller.
+  device_status_listener_->SetDeviceStatus(
+      DeviceStatus(BatteryStatus::CHARGING, NetworkStatus::UNMETERED));
+
+  driver_->AddTestData(dentries);
+  controller_->Initialize();
+  store_->TriggerInit(true, base::MakeUnique<std::vector<Entry>>(entries));
+  driver_->MakeReady();
+  task_runner_->RunUntilIdle();
+
+  EXPECT_EQ(Entry::State::ACTIVE, model_->Get(entry1.guid)->state);
+  EXPECT_EQ(Entry::State::ACTIVE, model_->Get(entry2.guid)->state);
+
+  EXPECT_TRUE(driver_->Find(entry1.guid).has_value());
+  EXPECT_FALSE(driver_->Find(entry1.guid).value().paused);
+  EXPECT_TRUE(driver_->Find(entry2.guid).has_value());
+  EXPECT_FALSE(driver_->Find(entry2.guid).value().paused);
+
+  DriverEntry dentry2;
+  dentry2.guid = base::GenerateGUID();
+  dentry2.state = DriverEntry::State::IN_PROGRESS;
+
+  // Simulate a newly created external download.
+  driver_->Start(RequestParams(), dentry2.guid, NO_TRAFFIC_ANNOTATION_YET);
+
+  EXPECT_TRUE(driver_->Find(entry1.guid).value().paused);
+  EXPECT_FALSE(driver_->Find(entry2.guid).value().paused);
+
+  // Simulate a paused external download.
+  dentry2.paused = true;
+  driver_->NotifyDownloadUpdate(dentry2);
+
+  EXPECT_FALSE(driver_->Find(entry1.guid).value().paused);
+  EXPECT_FALSE(driver_->Find(entry2.guid).value().paused);
+
+  // Simulate a resumed external download.
+  dentry2.paused = false;
+  driver_->NotifyDownloadUpdate(dentry2);
+
+  EXPECT_TRUE(driver_->Find(entry1.guid).value().paused);
+  EXPECT_FALSE(driver_->Find(entry2.guid).value().paused);
+
+  // Simulate a failed external download.
+  dentry2.state = DriverEntry::State::INTERRUPTED;
+  driver_->NotifyDownloadFailed(dentry2, 1);
+
+  EXPECT_FALSE(driver_->Find(entry1.guid).value().paused);
+  EXPECT_FALSE(driver_->Find(entry2.guid).value().paused);
+
+  // Rebuild the download so we can simulate more.
+  dentry2.state = DriverEntry::State::IN_PROGRESS;
+  driver_->Start(RequestParams(), dentry2.guid, NO_TRAFFIC_ANNOTATION_YET);
+
+  EXPECT_TRUE(driver_->Find(entry1.guid).value().paused);
+  EXPECT_FALSE(driver_->Find(entry2.guid).value().paused);
+
+  // Simulate a successful external download.
+  dentry2.state = DriverEntry::State::COMPLETE;
+  driver_->NotifyDownloadSucceeded(dentry2, base::FilePath());
+
+  EXPECT_FALSE(driver_->Find(entry1.guid).value().paused);
+  EXPECT_FALSE(driver_->Find(entry2.guid).value().paused);
+}
+
 }  // namespace download

components/download/internal/download_driver.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_DOWNLOAD_INTERNAL_DOWNLOAD_DRIVER_H_
 #define COMPONENTS_DOWNLOAD_INTERNAL_DOWNLOAD_DRIVER_H_
 
+#include <set>
 #include <string>
 
 #include "base/optional.h"
@@ -75,6 +76,10 @@ class DownloadDriver {
 
   // Finds a download record from low level download library.
   virtual base::Optional<DriverEntry> Find(const std::string& guid) = 0;
+
+  // Called to query the current set of active downloads.  This doesn't
+  // necessarily mean downloads started by the service.
+  virtual std::set<std::string> GetActiveDownloads() = 0;
 };
 
 }  // namespace download

components/download/internal/test/test_download_driver.cc

@@ -104,5 +104,16 @@ base::Optional<DriverEntry> TestDownloadDriver::Find(const std::string& guid) {
   return it->second;
 }
 
+std::set<std::string> TestDownloadDriver::GetActiveDownloads() {
+  std::set<std::string> guids;
+
+  for (auto& entry : entries_) {
+    if (entry.second.state == DriverEntry::State::IN_PROGRESS)
+      guids.insert(entry.second.guid);
+  }
+
+  return guids;
+}
+
 }  // namespace test
 }  // namespace download

components/download/internal/test/test_download_driver.h

@@ -45,6 +45,7 @@ class TestDownloadDriver : public DownloadDriver {
   void Pause(const std::string& guid) override;
   void Resume(const std::string& guid) override;
   base::Optional<DriverEntry> Find(const std::string& guid) override;
+  std::set<std::string> GetActiveDownloads() override;
 
  private:
   bool is_ready_;