Make downloads work when history database fails to initialize.
Previously all download calls will be stuck when history database fails
to initialize.
This CL assigns 1 as the first id when history database failed to load,
all downloads in this browser session will not be persisted to history
database.
Bug: 736511
Change-Id: If4db04ca518633a7dd999f45629e60e97fc78b1a
Reviewed-on: https://chromium-review.googlesource.com/587327
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490540}diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index e6343383..57e7f29 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -88,6 +88,11 @@
namespace {
+// The first id assigned to a download when download database failed to
+// initialize.
+const uint32_t kFirstDownloadIdNoPersist =
+ content::DownloadItem::kInvalidId + 1;
+
#if defined(FULL_SAFE_BROWSING)
// String pointer used for identifying safebrowing data associated with
@@ -251,8 +256,18 @@
void ChromeDownloadManagerDelegate::SetNextId(uint32_t next_id) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(!profile_->IsOffTheRecord());
- DCHECK_NE(content::DownloadItem::kInvalidId, next_id);
- next_download_id_ = next_id;
+
+ // |content::DownloadItem::kInvalidId| will be returned only when download
+ // database failed to initialize.
+ bool download_db_available = (next_id != content::DownloadItem::kInvalidId);
+ RecordDatabaseAvailability(download_db_available);
+ if (download_db_available) {
+ next_download_id_ = next_id;
+ } else {
+ // Still download files without download database, all download history in
+ // this browser session will not be persisted.
+ next_download_id_ = kFirstDownloadIdNoPersist;
+ }
IdCallbackVector callbacks;
id_callbacks_.swap(callbacks);
diff --git a/chrome/browser/download/chrome_download_manager_delegate.h b/chrome/browser/download/chrome_download_manager_delegate.h
index b19c7fa..731bff9 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.h
+++ b/chrome/browser/download/chrome_download_manager_delegate.h
@@ -152,8 +152,11 @@
uint32_t download_id,
const base::Closure& user_complete_callback);
+ // Sets the next download id based on download database records, and runs all
+ // cached id callbacks.
void SetNextId(uint32_t id);
+ // Runs the |callback| with next id. Results in the download being started.
void ReturnNextId(const content::DownloadIdCallback& callback);
void OnDownloadTargetDetermined(
@@ -169,7 +172,14 @@
bool ShouldBlockFile(content::DownloadDangerType danger_type) const;
Profile* profile_;
+
+ // Incremented by one for each download, the first available download id is
+ // assigned from history database or 1 when history database fails to
+ // intialize.
uint32_t next_download_id_;
+
+ // The |GetNextId| callbacks that may be cached before loading the download
+ // database.
IdCallbackVector id_callbacks_;
std::unique_ptr<DownloadPrefs> download_prefs_;
diff --git a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
index 9661b63..bb93527 100644
--- a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
@@ -243,12 +243,16 @@
DownloadPrefs* download_prefs();
PrefService* pref_service();
+ const std::vector<uint32_t>& download_ids() const { return download_ids_; }
+ void GetNextId(uint32_t next_id) { download_ids_.emplace_back(next_id); }
+
private:
sync_preferences::TestingPrefServiceSyncable* pref_service_;
base::ScopedTempDir test_download_dir_;
std::unique_ptr<content::MockDownloadManager> download_manager_;
std::unique_ptr<TestChromeDownloadManagerDelegate> delegate_;
MockWebContentsDelegate web_contents_delegate_;
+ std::vector<uint32_t> download_ids_;
};
ChromeDownloadManagerDelegateTest::ChromeDownloadManagerDelegateTest()
@@ -615,6 +619,30 @@
VerifyAndClearExpectations();
}
+TEST_F(ChromeDownloadManagerDelegateTest, WithoutHistoryDbNextId) {
+ content::DownloadIdCallback id_callback = base::Bind(
+ &ChromeDownloadManagerDelegateTest::GetNextId, base::Unretained(this));
+ delegate()->GetNextId(id_callback);
+ delegate()->GetNextId(id_callback);
+ // When download database fails to initialize, id will be set to
+ // |content::DownloadItem::kInvalidId|.
+ delegate()->GetDownloadIdReceiverCallback().Run(
+ content::DownloadItem::kInvalidId);
+ std::vector<uint32_t> expected_ids = std::vector<uint32_t>{1u, 2u};
+ EXPECT_EQ(expected_ids, download_ids());
+}
+
+TEST_F(ChromeDownloadManagerDelegateTest, WithHistoryDbNextId) {
+ content::DownloadIdCallback id_callback = base::Bind(
+ &ChromeDownloadManagerDelegateTest::GetNextId, base::Unretained(this));
+ delegate()->GetNextId(id_callback);
+ delegate()->GetNextId(id_callback);
+ // Simulates a valid download database with no records.
+ delegate()->GetDownloadIdReceiverCallback().Run(1u);
+ std::vector<uint32_t> expected_ids = std::vector<uint32_t>{1u, 2u};
+ EXPECT_EQ(expected_ids, download_ids());
+}
+
#if defined(FULL_SAFE_BROWSING)
namespace {
diff --git a/chrome/browser/download/download_stats.cc b/chrome/browser/download/download_stats.cc
index 6477de7d..bed151aa 100644
--- a/chrome/browser/download/download_stats.cc
+++ b/chrome/browser/download/download_stats.cc
@@ -50,3 +50,7 @@
open_method,
DOWNLOAD_OPEN_METHOD_LAST_ENTRY);
}
+
+void RecordDatabaseAvailability(bool is_available) {
+ UMA_HISTOGRAM_BOOLEAN("Download.Database.IsAvailable", is_available);
+}
diff --git a/chrome/browser/download/download_stats.h b/chrome/browser/download/download_stats.h
index cec8776..495b783 100644
--- a/chrome/browser/download/download_stats.h
+++ b/chrome/browser/download/download_stats.h
@@ -90,4 +90,8 @@
// Record how a download was opened.
void RecordDownloadOpenMethod(ChromeDownloadOpenMethod open_method);
+// Record if the database is available to provide the next download id before
+// starting all downloads.
+void RecordDatabaseAvailability(bool is_available);
+
#endif // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_STATS_H_
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 713cef47..145bb90 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -14917,6 +14917,16 @@
<summary>Time taken to create a single download in the history DB.</summary>
</histogram>
+<histogram name="Download.Database.IsAvailable" enum="BooleanAvailable">
+ <owner>xingliu@chromium.org</owner>
+ <summary>
+ Records whether the download database is available when database startup
+ completes, before starting any pending downloads. If the database is
+ available, it will provide the next download id. Or no download history will
+ be persisted.
+ </summary>
+</histogram>
+
<histogram name="Download.Database.QueryDownloadDuration" units="ms">
<owner>asanka@chromium.org</owner>
<summary>Time taken to query all downloads from history DB.</summary>
