Make a new integer field in sql::MetaTable (a per-profile db) containing a counter for the next download id, so that this id is unique across sessions. This id will allow us to merge download id with db_handle and merge most/all of the maps in DownloadManager in future CLs.

Make DownloadManager read this field to initialize its next_id_ counter in Init().
Put a fine-grained mutex in DownloadManager::GetNextId() so that it can be called directly from any thread.
Define a thunk wrapping DM::GNI() to be passed around between threads to guard against other threads calling any other DM methods.
This thunk owns a scoped_refptr<DM> to manage life-time issues. This pattern is implemented for DM elsewhere.
Store this thunk in ResourceContext to be called by ResourceDispatchHost/DownloadThrottlingResourceHandler on the IO thread. Pass the returned DownloadId into DownloadResourceHandler.
The alternative way to obtain ids on the IO thread is to jump over to the UI thread and back. This way would add significant latency to a critical path. GetNextId() should be fast and easily accessible from any thread.
Now that ids are per-profile, define a class DownloadId containing a per-profile id and an indication of which profile, currently the DownloadManager*. DownloadIds are hashable, comparable, globally unique, not persistent, and are used by DownloadFileManager.
When the download is added to the history, MetaTable.next_download_id will be set to the new download's id +1 if that number is greater than MT.next_download_id. Increasing this counter at the same time as the download is added to the db prevents the counter from desyncing from the db, which was the primary concern re storing the counter in the BrowserPrefs.
See also http://codereview.chromium.org/7192016
LMK what to write a test for if anything.

Review URL: http://codereview.chromium.org/7237034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@98656 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 255 insertions, 102 deletions

diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc
index 7847c5f..01b5e5d 100644
--- a/content/browser/download/download_file_manager.cc
+++ b/content/browser/download/download_file_manager.cc
@@ -65,9 +65,9 @@ void DownloadFileManager::CreateDownloadFile(DownloadCreateInfo* info,
     return;
   }
 
-  int32 id = info->download_id;
-  DCHECK(GetDownloadFile(id) == NULL);
-  downloads_[id] = download_file.release();
+  DownloadId global_id(download_manager, info->download_id);
+  DCHECK(GetDownloadFile(global_id) == NULL);
+  downloads_[global_id] = download_file.release();
 
   // The file is now ready, we can un-pause the request and start saving data.
   info->request_handle.ResumeRequest();
@@ -77,11 +77,11 @@ void DownloadFileManager::CreateDownloadFile(DownloadCreateInfo* info,
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(download_manager,
-                        &DownloadManager::StartDownload, id));
+                        &DownloadManager::StartDownload, info->download_id));
 }
 
-DownloadFile* DownloadFileManager::GetDownloadFile(int id) {
-  DownloadFileMap::iterator it = downloads_.find(id);
+DownloadFile* DownloadFileManager::GetDownloadFile(DownloadId global_id) {
+  DownloadFileMap::iterator it = downloads_.find(global_id);
   return it == downloads_.end() ? NULL : it->second;
 }
 
@@ -102,21 +102,17 @@ void DownloadFileManager::UpdateInProgressDownloads() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   for (DownloadFileMap::iterator i = downloads_.begin();
        i != downloads_.end(); ++i) {
-    int id = i->first;
+    DownloadId global_id = i->first;
     DownloadFile* download_file = i->second;
     DownloadManager* manager = download_file->GetDownloadManager();
     if (manager) {
       BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
           NewRunnableMethod(manager, &DownloadManager::UpdateDownload,
-                            id, download_file->bytes_so_far()));
+                            global_id.local(), download_file->bytes_so_far()));
     }
   }
 }
 
-int DownloadFileManager::GetNextId() {
-  return next_id_.GetNext();
-}
-
 void DownloadFileManager::StartDownload(DownloadCreateInfo* info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(info);
@@ -144,7 +140,8 @@ void DownloadFileManager::StartDownload(DownloadCreateInfo* info) {
 // download (in the UI thread), we may receive a few more updates before the IO
 // thread gets the cancel message: we just delete the data since the
 // DownloadFile has been deleted.
-void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
+void DownloadFileManager::UpdateDownload(
+    DownloadId global_id, DownloadBuffer* buffer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   std::vector<DownloadBuffer::Contents> contents;
   {
@@ -152,7 +149,7 @@ void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
     contents.swap(buffer->contents);
   }
 
-  DownloadFile* download_file = GetDownloadFile(id);
+  DownloadFile* download_file = GetDownloadFile(global_id);
   for (size_t i = 0; i < contents.size(); ++i) {
     net::IOBuffer* data = contents[i].first;
     const int data_len = contents[i].second;
@@ -163,16 +160,16 @@ void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
 }
 
 void DownloadFileManager::OnResponseCompleted(
-    int id,
+    DownloadId global_id,
     DownloadBuffer* buffer,
     int os_error,
     const std::string& security_info) {
-  VLOG(20) << __FUNCTION__ << "()" << " id = " << id
+  VLOG(20) << __FUNCTION__ << "()" << " id = " << global_id
            << " os_error = " << os_error
            << " security_info = \"" << security_info << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   delete buffer;
-  DownloadFile* download_file = GetDownloadFile(id);
+  DownloadFile* download_file = GetDownloadFile(global_id);
   if (!download_file)
     return;
 
@@ -180,7 +177,7 @@ void DownloadFileManager::OnResponseCompleted(
 
   DownloadManager* download_manager = download_file->GetDownloadManager();
   if (!download_manager) {
-    CancelDownload(id);
+    CancelDownload(global_id);
     return;
   }
 
@@ -192,7 +189,7 @@ void DownloadFileManager::OnResponseCompleted(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(
         download_manager, &DownloadManager::OnResponseCompleted,
-        id, download_file->bytes_so_far(), os_error, hash));
+        global_id.local(), download_file->bytes_so_far(), os_error, hash));
   // We need to keep the download around until the UI thread has finalized
   // the name.
 }
@@ -200,10 +197,10 @@ void DownloadFileManager::OnResponseCompleted(
 // This method will be sent via a user action, or shutdown on the UI thread, and
 // run on the download thread. Since this message has been sent from the UI
 // thread, the download may have already completed and won't exist in our map.
-void DownloadFileManager::CancelDownload(int id) {
-  VLOG(20) << __FUNCTION__ << "()" << " id = " << id;
+void DownloadFileManager::CancelDownload(DownloadId global_id) {
+  VLOG(20) << __FUNCTION__ << "()" << " id = " << global_id;
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DownloadFileMap::iterator it = downloads_.find(id);
+  DownloadFileMap::iterator it = downloads_.find(global_id);
   if (it == downloads_.end())
     return;
 
@@ -212,24 +209,24 @@ void DownloadFileManager::CancelDownload(int id) {
            << " download_file = " << download_file->DebugString();
   download_file->Cancel();
 
-  EraseDownload(id);
+  EraseDownload(global_id);
 }
 
-void DownloadFileManager::CompleteDownload(int id) {
+void DownloadFileManager::CompleteDownload(DownloadId global_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  if (!ContainsKey(downloads_, id))
+  if (!ContainsKey(downloads_, global_id))
     return;
 
-  DownloadFile* download_file = downloads_[id];
+  DownloadFile* download_file = downloads_[global_id];
 
   VLOG(20) << " " << __FUNCTION__ << "()"
-           << " id = " << id
+           << " id = " << global_id
            << " download_file = " << download_file->DebugString();
 
   download_file->Detach();
 
-  EraseDownload(id);
+  EraseDownload(global_id);
 }
 
 void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
@@ -249,7 +246,7 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
 
   for (std::set<DownloadFile*>::iterator i = to_remove.begin();
        i != to_remove.end(); ++i) {
-    downloads_.erase((*i)->id());
+    downloads_.erase(DownloadId((*i)->GetDownloadManager(), (*i)->id()));
     delete *i;
   }
 }
@@ -263,12 +260,12 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
 // 1. tmp -> foo.crdownload (not final, safe)
 // 2. tmp-> Unconfirmed.xxx.crdownload (not final, dangerous)
 void DownloadFileManager::RenameInProgressDownloadFile(
-    int id, const FilePath& full_path) {
-  VLOG(20) << __FUNCTION__ << "()" << " id = " << id
+    DownloadId global_id, const FilePath& full_path) {
+  VLOG(20) << __FUNCTION__ << "()" << " id = " << global_id
            << " full_path = \"" << full_path.value() << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  DownloadFile* download_file = GetDownloadFile(id);
+  DownloadFile* download_file = GetDownloadFile(global_id);
   if (!download_file)
     return;
 
@@ -278,7 +275,7 @@ void DownloadFileManager::RenameInProgressDownloadFile(
   if (!download_file->Rename(full_path)) {
     // Error. Between the time the UI thread generated 'full_path' to the time
     // this code runs, something happened that prevents us from renaming.
-    CancelDownloadOnRename(id);
+    CancelDownloadOnRename(global_id);
   }
 }
 
@@ -290,13 +287,15 @@ void DownloadFileManager::RenameInProgressDownloadFile(
 // 1. foo.crdownload -> foo (final, safe)
 // 2. Unconfirmed.xxx.crdownload -> xxx (final, validated)
 void DownloadFileManager::RenameCompletingDownloadFile(
-    int id, const FilePath& full_path, bool overwrite_existing_file) {
-  VLOG(20) << __FUNCTION__ << "()" << " id = " << id
+    DownloadId global_id,
+    const FilePath& full_path,
+    bool overwrite_existing_file) {
+  VLOG(20) << __FUNCTION__ << "()" << " id = " << global_id
            << " overwrite_existing_file = " << overwrite_existing_file
            << " full_path = \"" << full_path.value() << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  DownloadFile* download_file = GetDownloadFile(id);
+  DownloadFile* download_file = GetDownloadFile(global_id);
   if (!download_file)
     return;
 
@@ -326,7 +325,7 @@ void DownloadFileManager::RenameCompletingDownloadFile(
   if (!download_file->Rename(new_path)) {
     // Error. Between the time the UI thread generated 'full_path' to the time
     // this code runs, something happened that prevents us from renaming.
-    CancelDownloadOnRename(id);
+    CancelDownloadOnRename(global_id);
     return;
   }
 
@@ -336,19 +335,17 @@ void DownloadFileManager::RenameCompletingDownloadFile(
   download_file->AnnotateWithSourceInformation();
 #endif
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(
-          download_manager, &DownloadManager::OnDownloadRenamedToFinalName, id,
-          new_path, uniquifier));
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, NewRunnableMethod(
+      download_manager, &DownloadManager::OnDownloadRenamedToFinalName,
+      global_id.local(), new_path, uniquifier));
 }
 
 // Called only from RenameInProgressDownloadFile and
 // RenameCompletingDownloadFile on the FILE thread.
-void DownloadFileManager::CancelDownloadOnRename(int id) {
+void DownloadFileManager::CancelDownloadOnRename(DownloadId global_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  DownloadFile* download_file = GetDownloadFile(id);
+  DownloadFile* download_file = GetDownloadFile(global_id);
   if (!download_file)
     return;
 
@@ -364,22 +361,22 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(download_manager,
-                        &DownloadManager::CancelDownload, id));
+                        &DownloadManager::CancelDownload, global_id.local()));
 }
 
-void DownloadFileManager::EraseDownload(int id) {
+void DownloadFileManager::EraseDownload(DownloadId global_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  if (!ContainsKey(downloads_, id))
+  if (!ContainsKey(downloads_, global_id))
     return;
 
-  DownloadFile* download_file = downloads_[id];
+  DownloadFile* download_file = downloads_[global_id];
 
   VLOG(20) << " " << __FUNCTION__ << "()"
-           << " id = " << id
+           << " id = " << global_id
            << " download_file = " << download_file->DebugString();
 
-  downloads_.erase(id);
+  downloads_.erase(global_id);
 
   delete download_file;
 
diff --git a/content/browser/download/download_file_manager.h b/content/browser/download/download_file_manager.h
index 0c14dca..d7e62f0 100644
--- a/content/browser/download/download_file_manager.h
+++ b/content/browser/download/download_file_manager.h
@@ -49,6 +49,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/timer.h"
 #include "content/browser/download/download_request_handle.h"
+#include "content/browser/download/download_id.h"
 #include "ui/gfx/native_widget_types.h"
 
 struct DownloadBuffer;
@@ -73,21 +74,18 @@ class DownloadFileManager
   // Called on shutdown on the UI thread.
   void Shutdown();
 
-  // Called on the IO or UI threads.
-  int GetNextId();
-
   // Called on UI thread to make DownloadFileManager start the download.
   void StartDownload(DownloadCreateInfo* info);
 
   // Handlers for notifications sent from the IO thread and run on the
   // FILE thread.
-  void UpdateDownload(int id, DownloadBuffer* buffer);
+  void UpdateDownload(DownloadId id, DownloadBuffer* buffer);
   // |os_error| is 0 for normal completions, and non-0 for errors.
   // |security_info| contains SSL information (cert_id, cert_status,
   // security_bits, ssl_connection_status), which can be used to
   // fine-tune the error message.  It is empty if the transaction
   // was not performed securely.
-  void OnResponseCompleted(int id,
+  void OnResponseCompleted(DownloadId id,
                            DownloadBuffer* buffer,
                            int os_error,
                            const std::string& security_info);
@@ -97,22 +95,22 @@ class DownloadFileManager
   // download file, as far as the DownloadFileManager is concerned -- if
   // anything happens to the download file after they are called, it will
   // be ignored.
-  void CancelDownload(int id);
-  void CompleteDownload(int id);
+  void CancelDownload(DownloadId id);
+  void CompleteDownload(DownloadId id);
 
   // Called on FILE thread by DownloadManager at the beginning of its shutdown.
   void OnDownloadManagerShutdown(DownloadManager* manager);
 
   // The DownloadManager in the UI thread has provided an intermediate
   // .crdownload name for the download specified by |id|.
-  void RenameInProgressDownloadFile(int id, const FilePath& full_path);
+  void RenameInProgressDownloadFile(DownloadId id, const FilePath& full_path);
 
   // The DownloadManager in the UI thread has provided a final name for the
   // download specified by |id|.
   // |overwrite_existing_file| prevents uniquification, and is used for SAFE
   // downloads, as the user may have decided to overwrite the file.
   // Sent from the UI thread and run on the FILE thread.
-  void RenameCompletingDownloadFile(int id,
+  void RenameCompletingDownloadFile(DownloadId id,
                                     const FilePath& full_path,
                                     bool overwrite_existing_file);
 
@@ -144,20 +142,17 @@ class DownloadFileManager
                           bool hash_needed);
 
   // Called only on the download thread.
-  DownloadFile* GetDownloadFile(int id);
+  DownloadFile* GetDownloadFile(DownloadId id);
 
   // Called only from RenameInProgressDownloadFile and
   // RenameCompletingDownloadFile on the FILE thread.
-  void CancelDownloadOnRename(int id);
+  void CancelDownloadOnRename(DownloadId id);
 
   // Erases the download file with the given the download |id| and removes
   // it from the maps.
-  void EraseDownload(int id);
-
-  // Unique ID for each DownloadItem.
-  base::AtomicSequenceNumber next_id_;
+  void EraseDownload(DownloadId id);
 
-  typedef base::hash_map<int, DownloadFile*> DownloadFileMap;
+  typedef base::hash_map<DownloadId, DownloadFile*> DownloadFileMap;
 
   // A map of all in progress downloads.  It owns the download files.
   DownloadFileMap downloads_;
diff --git a/content/browser/download/download_id.cc b/content/browser/download/download_id.cc
new file mode 100644
index 0000000..bef4964
--- /dev/null
+++ b/content/browser/download/download_id.cc
@@ -0,0 +1,9 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/download/download_id.h"
+
+std::ostream& operator<<(std::ostream& out, const DownloadId& global_id) {
+  return out << global_id.manager_ << ":" << global_id.local();
+}
diff --git a/content/browser/download/download_id.h b/content/browser/download/download_id.h
new file mode 100644
index 0000000..eb9c556
--- /dev/null
+++ b/content/browser/download/download_id.h
@@ -0,0 +1,87 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ID_H_
+#define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ID_H_
+#pragma once
+
+#include <iostream>
+
+#include "base/hash_tables.h"
+
+class DownloadManager;
+
+// DownloadId combines per-profile Download ids with an indication of which
+// profile in order to be globally unique. DownloadIds are not persistent across
+// sessions, but their local() field is.
+class DownloadId {
+ public:
+  DownloadId(const DownloadManager* manager, int32 local_id)
+    : manager_(manager),
+      local_id_(local_id) {
+  }
+
+  // Return the per-profile and persistent part of this DownloadId.
+  int32 local() const { return local_id_; }
+
+  // Returns true if this DownloadId has been allocated and could possibly refer
+  // to a DownloadItem that exists.
+  bool IsValid() const { return ((manager_ != NULL) && (local_id_ >= 0)); }
+
+  // The following methods (operator==, hash(), copy, and assign) provide
+  // support for STL containers such as hash_map.
+
+  bool operator==(const DownloadId& that) const {
+    return ((that.local_id_ == local_id_) &&
+            (that.manager_ == manager_));
+  }
+  bool operator<(const DownloadId& that) const {
+    // Even though DownloadManager* < DownloadManager* is not well defined and
+    // GCC does not require it for hash_map, MSVC requires operator< for
+    // hash_map. We don't ifdef it out here because we will probably make a
+    // set<DownloadId> at some point, when GCC will require it.
+    return ((that.local_id_ < local_id_) &&
+            (that.manager_ < manager_));
+  }
+
+  size_t hash() const {
+    // The top half of manager is unlikely to be distinct, and the user is
+    // unlikely to have >64K downloads. If these assumptions are incorrect, then
+    // DownloadFileManager's hash_map might have a few collisions, but it will
+    // use operator== to safely disambiguate.
+    return reinterpret_cast<size_t>(manager_) +
+           (static_cast<size_t>(local_id_) << (4 * sizeof(size_t)));
+  }
+
+ private:
+  // DownloadId is used mostly off the UI thread, so manager's methods can't be
+  // called, but the pointer can be compared.
+  const DownloadManager* manager_;
+
+  int32 local_id_;
+
+  friend std::ostream& operator<<(std::ostream& out,
+                                  const DownloadId& global_id);
+
+  // Allow copy and assign.
+};
+
+// Allow logging DownloadIds. Looks like "0x01234567:42".
+std::ostream& operator<<(std::ostream& out, const DownloadId& global_id);
+
+// Allow using DownloadIds as keys in hash_maps.
+namespace BASE_HASH_NAMESPACE {
+#if defined(COMPILER_GCC)
+template<> struct hash<DownloadId> {
+  std::size_t operator()(const DownloadId& id) const {
+    return id.hash();
+  }
+};
+#elif defined(COMPILER_MSVC)
+inline size_t hash_value(const DownloadId& id) {
+  return id.hash();
+}
+#endif  // COMPILER
+}
+#endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ID_H_
diff --git a/content/browser/download/download_item.cc b/content/browser/download/download_item.cc
index e881f0f..1e2b98b 100644
--- a/content/browser/download/download_item.cc
+++ b/content/browser/download/download_item.cc
@@ -19,6 +19,7 @@
 #include "content/browser/download/download_file.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file_manager.h"
+#include "content/browser/download/download_id.h"
 #include "content/browser/download/download_manager.h"
 #include "content/browser/download/download_manager_delegate.h"
 #include "content/browser/download/download_persistent_store_info.h"
@@ -190,8 +191,8 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
                            const FilePath& path,
                            const GURL& url,
                            bool is_otr,
-                           int download_id)
-    : download_id_(download_id),
+                           DownloadId download_id)
+    : download_id_(download_id.local()),
       full_path_(path),
       url_chain_(1, url),
       referrer_url_(GURL()),
@@ -224,6 +225,10 @@ DownloadItem::~DownloadItem() {
   download_manager_->AssertQueueStateConsistent(this);
 }
 
+DownloadId DownloadItem::global_id() const {
+  return DownloadId(download_manager_, id());
+}
+
 void DownloadItem::AddObserver(Observer* observer) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -575,16 +580,15 @@ void DownloadItem::OnDownloadCompleting(DownloadFileManager* file_manager) {
   if (NeedsRename()) {
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
         NewRunnableMethod(file_manager,
-            &DownloadFileManager::RenameCompletingDownloadFile, id(),
+            &DownloadFileManager::RenameCompletingDownloadFile, global_id(),
             GetTargetFilePath(), safety_state() == SAFE));
     return;
   }
 
   Completed();
 
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(file_manager, &DownloadFileManager::CompleteDownload,
-                        id()));
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, NewRunnableMethod(
+        file_manager, &DownloadFileManager::CompleteDownload, global_id()));
 }
 
 void DownloadItem::OnDownloadRenamedToFinalName(const FilePath& full_path) {
@@ -710,7 +714,7 @@ void DownloadItem::OffThreadCancel(DownloadFileManager* file_manager) {
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
-          file_manager, &DownloadFileManager::CancelDownload, download_id_));
+          file_manager, &DownloadFileManager::CancelDownload, global_id()));
 }
 
 void DownloadItem::Init(bool active) {
diff --git a/content/browser/download/download_item.h b/content/browser/download/download_item.h
index 5476aa1..536c466 100644
--- a/content/browser/download/download_item.h
+++ b/content/browser/download/download_item.h
@@ -19,6 +19,7 @@
 #pragma once
 
 #include <string>
+#include <iostream>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
@@ -30,6 +31,7 @@
 #include "googleurl/src/gurl.h"
 
 class DownloadFileManager;
+class DownloadId;
 class DownloadManager;
 struct DownloadCreateInfo;
 struct DownloadPersistentStoreInfo;
@@ -122,7 +124,7 @@ class DownloadItem {
                const FilePath& path,
                const GURL& url,
                bool is_otr,
-               int download_id);
+               DownloadId download_id);
 
   virtual ~DownloadItem();
 
@@ -273,6 +275,7 @@ class DownloadItem {
   }
   int64 received_bytes() const { return received_bytes_; }
   int32 id() const { return download_id_; }
+  DownloadId global_id() const;
   base::Time start_time() const { return start_time_; }
   void set_db_handle(int64 handle) { db_handle_ = handle; }
   int64 db_handle() const { return db_handle_; }
diff --git a/content/browser/download/download_manager.cc b/content/browser/download/download_manager.cc
index bc12c56..d4226f7 100644
--- a/content/browser/download/download_manager.cc
+++ b/content/browser/download/download_manager.cc
@@ -6,11 +6,15 @@
 
 #include <iterator>
 
+#include "base/bind.h"
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/stringprintf.h"
+#include "base/synchronization/lock.h"
+#include "base/sys_string_conversions.h"
 #include "base/task.h"
 #include "build/build_config.h"
 #include "content/browser/browser_context.h"
@@ -50,6 +54,7 @@ DownloadManager::DownloadManager(DownloadManagerDelegate* delegate,
                                  DownloadStatusUpdater* status_updater)
     : shutdown_needed_(false),
       browser_context_(NULL),
+      next_id_(0),
       file_manager_(NULL),
       status_updater_(status_updater->AsWeakPtr()),
       delegate_(delegate),
@@ -177,6 +182,30 @@ void DownloadManager::SearchDownloads(const string16& query,
   }
 }
 
+void DownloadManager::OnPersistentStoreGetNextId(int next_id) {
+  DVLOG(1) << __FUNCTION__ << " " << next_id;
+  base::AutoLock lock(next_id_lock_);
+  // TODO(benjhayden) Delay Profile initialization until here, and set next_id_
+  // = next_id. The '+=' works for now because these ids are not yet persisted
+  // to the database. GetNextId() can allocate zero or more ids starting from 0,
+  // then this callback can increment next_id_, and the items with lower ids
+  // won't clash with any other items even though there may be items loaded from
+  // the history because items from the history don't have valid ids.
+  next_id_ += next_id;
+}
+
+DownloadId DownloadManager::GetNextId() {
+  // May be called on any thread via the GetNextIdThunk.
+  // TODO(benjhayden) If otr, forward to parent DM.
+  base::AutoLock lock(next_id_lock_);
+  return DownloadId(this, next_id_++);
+}
+
+DownloadManager::GetNextIdThunkType DownloadManager::GetNextIdThunk() {
+  // TODO(benjhayden) If otr, forward to parent DM.
+  return base::Bind(&DownloadManager::GetNextId, this);
+}
+
 // Query the history service for information about all persisted downloads.
 bool DownloadManager::Init(content::BrowserContext* browser_context) {
   DCHECK(browser_context);
@@ -328,7 +357,7 @@ void DownloadManager::ContinueDownloadWithPath(DownloadItem* download,
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
           file_manager_, &DownloadFileManager::RenameInProgressDownloadFile,
-          download->id(), download_path));
+          download->global_id(), download_path));
 
   download->Rename(download_path);
 
@@ -499,10 +528,10 @@ void DownloadManager::OnDownloadRenamedToFinalName(int download_id,
     DCHECK_EQ(0, uniquifier) << "We should not uniquify SAFE downloads twice";
   }
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::CompleteDownload, download_id));
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, NewRunnableMethod(
+      file_manager_,
+      &DownloadFileManager::CompleteDownload,
+      item->global_id()));
 
   if (uniquifier)
     item->set_path_uniquifier(uniquifier);
@@ -573,10 +602,10 @@ void DownloadManager::OnDownloadError(int32 download_id,
     delegate_->UpdateItemInPersistentStore(download);
   }
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::CancelDownload, download_id));
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, NewRunnableMethod(
+      file_manager_,
+      &DownloadFileManager::CancelDownload,
+      download->global_id()));
 }
 
 void DownloadManager::UpdateDownloadProgress() {
diff --git a/content/browser/download/download_manager.h b/content/browser/download/download_manager.h
index 7f92423..185781f 100644
--- a/content/browser/download/download_manager.h
+++ b/content/browser/download/download_manager.h
@@ -34,6 +34,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/hash_tables.h"
@@ -41,6 +42,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/synchronization/lock.h"
 #include "base/time.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/download/download_item.h"
@@ -106,6 +108,24 @@ class DownloadManager
   // everything.
   void SearchDownloads(const string16& query, DownloadVector* result);
 
+  // Returns the next download id in a DownloadId and increments the counter.
+  // May be called on any thread. The incremented counter is not persisted, but
+  // the base counter for this accessor is initialized from the largest id
+  // actually saved to the download history database.
+  DownloadId GetNextId();
+
+  // Instead of passing a DownloadManager* between threads and hoping users only
+  // call GetNextId(), you can pass this thunk around instead. Pass the thunk
+  // around by const ref and store it by copy per the base::Callback interface.
+  // The thunk may be copied, including between threads.  If you change
+  // GetNextIdThunkType from base::Callback, then you should think about how
+  // you're changing the ref-count of DownloadManager. Use it like:
+  //   const DownloadManager::GetNextIdThunkType& next_id_thunk =
+  //       download_manager->GetNextIdThunk();
+  //   id = next_id_thunk.Run();
+  typedef base::Callback<DownloadId(void)> GetNextIdThunkType;
+  GetNextIdThunkType GetNextIdThunk();
+
   // Returns true if initialized properly.
   bool Init(content::BrowserContext* browser_context);
 
@@ -186,6 +206,11 @@ class DownloadManager
   // Remove a download observer from ourself.
   void RemoveObserver(Observer* observer);
 
+  // Called by the embedder after creating the download manager to inform it of
+  // the next available download id.
+  // TODO(benjhayden): Separate this functionality out into a separate object.
+  void OnPersistentStoreGetNextId(int next_id);
+
   // Called by the embedder, after creating the download manager, to let it know
   // about downloads from previous runs of the browser.
   void OnPersistentStoreQueryComplete(
@@ -368,6 +393,9 @@ class DownloadManager
   // The current active browser context.
   content::BrowserContext* browser_context_;
 
+  base::Lock next_id_lock_;
+  int next_id_;
+
   // Non-owning pointer for handling file writing on the download_thread_.
   DownloadFileManager* file_manager_;
 
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index cd03db8..681e1bc 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -30,11 +30,12 @@ DownloadResourceHandler::DownloadResourceHandler(
     int render_view_id,
     int request_id,
     const GURL& url,
+    DownloadId dl_id,
     DownloadFileManager* download_file_manager,
     net::URLRequest* request,
     bool save_as,
     const DownloadSaveInfo& save_info)
-    : download_id_(-1),
+    : download_id_(dl_id),
       global_id_(render_process_host_id, request_id),
       render_view_id_(render_view_id),
       content_length_(0),
@@ -45,6 +46,7 @@ DownloadResourceHandler::DownloadResourceHandler(
       buffer_(new DownloadBuffer),
       rdh_(rdh),
       is_paused_(false) {
+  DCHECK(dl_id.IsValid());
   download_stats::RecordDownloadCount(download_stats::UNTHROTTLED_COUNT);
 }
 
@@ -65,6 +67,7 @@ bool DownloadResourceHandler::OnRequestRedirected(int request_id,
 // Send the download creation information to the download thread.
 bool DownloadResourceHandler::OnResponseStarted(int request_id,
                                                 ResourceResponse* response) {
+  DCHECK(download_id_.IsValid());
   VLOG(20) << __FUNCTION__ << "()" << DebugString()
            << " request_id = " << request_id;
   download_start_time_ = base::TimeTicks::Now();
@@ -77,8 +80,6 @@ bool DownloadResourceHandler::OnResponseStarted(int request_id,
   const ResourceDispatcherHostRequestInfo* request_info =
     ResourceDispatcherHost::InfoForRequest(request_);
 
-  download_id_ = download_file_manager_->GetNextId();
-
   // Deleted in DownloadManager.
   DownloadCreateInfo* info = new DownloadCreateInfo;
   info->url_chain = request_->url_chain();
@@ -87,7 +88,7 @@ bool DownloadResourceHandler::OnResponseStarted(int request_id,
   info->received_bytes = 0;
   info->total_bytes = content_length_;
   info->state = DownloadItem::IN_PROGRESS;
-  info->download_id = download_id_;
+  info->download_id = download_id_.local();
   info->has_user_gesture = request_info->has_user_gesture();
   info->request_handle = DownloadRequestHandle(rdh_,
                                                global_id_.child_id,
@@ -257,7 +258,7 @@ std::string DownloadResourceHandler::DebugString() const {
                             " save_info_.file_path = \"%" PRFilePath "\""
                             " }",
                             request_->url().spec().c_str(),
-                            download_id_,
+                            download_id_.local(),
                             global_id_.child_id,
                             global_id_.request_id,
                             render_view_id_,
diff --git a/content/browser/download/download_resource_handler.h b/content/browser/download/download_resource_handler.h
index b4cdbd6..e864c0e 100644
--- a/content/browser/download/download_resource_handler.h
+++ b/content/browser/download/download_resource_handler.h
@@ -11,6 +11,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/timer.h"
 #include "content/browser/download/download_file.h"
+#include "content/browser/download/download_id.h"
 #include "content/browser/renderer_host/global_request_id.h"
 #include "content/browser/renderer_host/resource_handler.h"
 
@@ -25,15 +26,17 @@ class URLRequest;
 // Forwards data to the download thread.
 class DownloadResourceHandler : public ResourceHandler {
  public:
-  DownloadResourceHandler(ResourceDispatcherHost* rdh,
-                          int render_process_host_id,
-                          int render_view_id,
-                          int request_id,
-                          const GURL& url,
-                          DownloadFileManager* download_file_manager,
-                          net::URLRequest* request,
-                          bool save_as,
-                          const DownloadSaveInfo& save_info);
+  DownloadResourceHandler(
+      ResourceDispatcherHost* rdh,
+      int render_process_host_id,
+      int render_view_id,
+      int request_id,
+      const GURL& url,
+      DownloadId dl_id,
+      DownloadFileManager* download_file_manager,
+      net::URLRequest* request,
+      bool save_as,
+      const DownloadSaveInfo& save_info);
 
   virtual bool OnUploadProgress(int request_id, uint64 position, uint64 size);
 
@@ -75,7 +78,7 @@ class DownloadResourceHandler : public ResourceHandler {
 
   void StartPauseTimer();
 
-  int download_id_;
+  DownloadId download_id_;
   GlobalRequestID global_id_;
   int render_view_id_;
   scoped_refptr<net::IOBuffer> read_buffer_;
diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc
index 5cb56df..db6b423 100644
--- a/content/browser/download/save_package.cc
+++ b/content/browser/download/save_package.cc
@@ -263,15 +263,12 @@ bool SavePackage::Init() {
     return false;
   }
 
-  ResourceDispatcherHost* rdh =
-      content::GetContentClient()->browser()->GetResourceDispatcherHost();
-
   // Create the download item, and add ourself as an observer.
   download_ = new DownloadItem(download_manager_,
                                saved_main_file_path_,
                                page_url_,
                                browser_context->IsOffTheRecord(),
-                               rdh->download_file_manager()->GetNextId());
+                               download_manager_->GetNextId());
   download_->AddObserver(this);
 
   // Transfer ownership to the download manager.