Fix an issue that incognito download may use the same id as regular download

As we moved the download id generation logic to DownloadManagerImpl, but
incognito download is still using regular profile's DownloadManagerDelegate
to generate the id. So it is possible that incognito download may have the
same id as a regular download.
In theory this shouldn't be an issue as incognito and regular download belongs
to different DownloadManagerImpl. However, Chrome's download home and extensions
use id to uniquely identify a download. Thus causing the issues when accessing
these downloads from the download home and extensions.

BUG=902336

Change-Id: I3591153e4f453c75d4877f5ce1088c096d3edf1e
Reviewed-on: https://chromium-review.googlesource.com/c/1330214Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607059}

chrome/browser/download/chrome_download_manager_delegate.cc

@@ -378,8 +378,8 @@ void ChromeDownloadManagerDelegate::GetNextId(
     const content::DownloadIdCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (profile_->IsOffTheRecord()) {
-    content::BrowserContext::GetDownloadManager(
-        profile_->GetOriginalProfile())->GetDelegate()->GetNextId(callback);
+    content::BrowserContext::GetDownloadManager(profile_->GetOriginalProfile())
+        ->GetNextId(callback);
     return;
   }
   if (!next_id_retrieved_) {

chrome/browser/download/download_browsertest.cc

@@ -144,6 +144,10 @@ namespace {
 
 const char kDownloadTest1Path[] = "download-test1.lib";
 
+void VerifyNewDownloadId(uint32_t expected_download_id, uint32_t download_id) {
+  ASSERT_EQ(expected_download_id, download_id);
+}
+
 class DownloadTestContentBrowserClient : public content::ContentBrowserClient {
  public:
   explicit DownloadTestContentBrowserClient(bool must_download)
@@ -1625,6 +1629,11 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_IncognitoRegular) {
   ASSERT_TRUE(base::PathExists(download_items[0]->GetTargetFilePath()));
   EXPECT_TRUE(VerifyFile(download_items[0]->GetTargetFilePath(),
                          original_contents, origin_file_size));
+  uint32_t download_id = download_items[0]->GetId();
+  // Verify that manager will increment the download ID when a new download is
+  // requested.
+  DownloadManagerForBrowser(browser())->GetNextId(
+      base::BindOnce(&VerifyNewDownloadId, download_id + 1));
 
   // Setup an incognito window.
   Browser* incognito = CreateIncognitoBrowser();
@@ -1648,6 +1657,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_IncognitoRegular) {
   ASSERT_TRUE(base::PathExists(download_items[0]->GetTargetFilePath()));
   EXPECT_TRUE(VerifyFile(download_items[0]->GetTargetFilePath(),
                          original_contents, origin_file_size));
+  // The incognito download should increment the download ID again.
+  ASSERT_EQ(download_id + 2, download_items[0]->GetId());
 }
 
 // Navigate to a new background page, but don't download.

content/browser/download/download_manager_impl.h

@@ -133,6 +133,7 @@ class CONTENT_EXPORT DownloadManagerImpl
       base::OnceClosure load_history_downloads_cb) override;
   download::DownloadItem* GetDownload(uint32_t id) override;
   download::DownloadItem* GetDownloadByGuid(const std::string& guid) override;
+  void GetNextId(GetNextIdCallback callback) override;
 
   // UrlDownloadHandler::Delegate implementation.
   void OnUrlDownloadStarted(
@@ -215,11 +216,6 @@ class CONTENT_EXPORT DownloadManagerImpl
       download::InProgressDownloadManager::StartDownloadItemCallback callback,
       uint32_t id);
 
-  using GetNextIdCallback = base::OnceCallback<void(uint32_t)>;
-  // Called to get an ID for a new download. |callback| may be called
-  // synchronously.
-  void GetNextId(GetNextIdCallback callback);
-
   // Sets the |next_download_id_| if the |next_id| is larger. Runs all the
   // |id_callbacks_| if both the ID from both history db and in-progress db
   // are retrieved.

content/public/browser/download_manager.h

@@ -239,6 +239,11 @@ class CONTENT_EXPORT DownloadManager : public base::SupportsUserData::Data {
   // Get the download item for |guid|.
   virtual download::DownloadItem* GetDownloadByGuid(
       const std::string& guid) = 0;
+
+  using GetNextIdCallback = base::OnceCallback<void(uint32_t)>;
+  // Called to get an ID for a new download. |callback| may be called
+  // synchronously.
+  virtual void GetNextId(GetNextIdCallback callback) = 0;
 };
 
 }  // namespace content

content/public/test/mock_download_manager.h

@@ -168,6 +168,7 @@ class MockDownloadManager : public DownloadManager {
   MOCK_METHOD0(CheckForHistoryFilesRemoval, void());
   MOCK_METHOD1(GetDownload, download::DownloadItem*(uint32_t id));
   MOCK_METHOD1(GetDownloadByGuid, download::DownloadItem*(const std::string&));
+  MOCK_METHOD1(GetNextId, void(base::OnceCallback<void(uint32_t)>));
 
   void OnHistoryQueryComplete(
       base::OnceClosure load_history_downloads_cb) override;