Avoid LazyInstance in DownloadPathReservationTracker.

chrome/browser/download/download_path_reservation_tracker.cc

Index: chrome/browser/download/download_path_reservation_tracker.cc
diff --git a/chrome/browser/download/download_path_reservation_tracker.cc b/chrome/browser/download/download_path_reservation_tracker.cc
index 2f0e3d86cfa8cffeaeca0469f69cd4e7210472e6..c6e9dbb27a1fcae561802ab835397379eb38e60e 100644
--- a/chrome/browser/download/download_path_reservation_tracker.cc
+++ b/chrome/browser/download/download_path_reservation_tracker.cc
@@ -4,10 +4,11 @@
 
 #include "chrome/browser/download/download_path_reservation_tracker.h"
 
+#include <map>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/file_util.h"
-#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
@@ -23,13 +24,23 @@ using content::DownloadItem;
 
 namespace {
 
+typedef std::map<content::DownloadId, FilePath> ReservationMap;
+
+// Map of download path reservations. Each reserved path is associated with a
+// DownloadId. This object is destroyed in |Revoke()| when there are no more
+// reservations.
+//
+// It is not an error, although undesirable, to have multiple paths that are
+// mapped to the same DownloadId. This can happen if a reservation is created
+// that is supposed to overwrite an existing file or reservation.

[Randy Smith (Not in Mondays), 2012/08/03 19:17:14]

I don't understand this comment, and I think it doesn't match the code.  As I
understand it, a "std::map" is a 1->1 mapping; you need a multimap to have a
1->many mapping.  Did you mean multiple DownloadIds assigned to the same path?

[asanka, 2012/08/03 19:50:48]

Written backwards, the comment I have. enoD.

+ReservationMap* g_reservation_map = NULL;
+
 // Observes a DownloadItem for changes to its target path and state. Updates or
-// revokes associated download path reservations as necessary.
+// revokes associated download path reservations as necessary. Created, invoked
+// and destroyed on the UI thread.
 class DownloadItemObserver : public DownloadItem::Observer {
  public:
-  DownloadItemObserver(DownloadItem& download_item,
-                       base::Closure revoke,
-                       base::Callback<void(const FilePath&)> update);
+  explicit DownloadItemObserver(DownloadItem& download_item);
 
  private:
   virtual ~DownloadItemObserver();
@@ -43,124 +54,73 @@ class DownloadItemObserver : public DownloadItem::Observer {
   // Last known target path for the download.
   FilePath last_target_path_;
 
-  // Callback to invoke to revoke the path reseration.
-  base::Closure revoke_callback_;
-
-  // Callback to invoke to update the path reservation.
-  base::Callback<void(const FilePath&)> update_callback_;
-
   DISALLOW_COPY_AND_ASSIGN(DownloadItemObserver);
 };
 
-DownloadItemObserver::DownloadItemObserver(
-    DownloadItem& download_item,
-    base::Closure revoke,
-    base::Callback<void(const FilePath&)> update)
-    : download_item_(download_item),
-      last_target_path_(download_item.GetTargetFilePath()),
-      revoke_callback_(revoke),
-      update_callback_(update) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  download_item_.AddObserver(this);
-}
-
-DownloadItemObserver::~DownloadItemObserver() {
-  download_item_.RemoveObserver(this);
-}
-
-void DownloadItemObserver::OnDownloadUpdated(DownloadItem* download) {
-  switch (download->GetState()) {
-    case DownloadItem::IN_PROGRESS: {
-      // Update the reservation.
-      FilePath new_target_path = download->GetTargetFilePath();
-      if (new_target_path != last_target_path_) {
-        BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                                base::Bind(update_callback_, new_target_path));
-        last_target_path_ = new_target_path;
-      }
-      break;
-    }
-
-    case DownloadItem::COMPLETE:
-      // If the download is complete, then it has already been renamed to the
-      // final name. The existence of the file on disk is sufficient to prevent
-      // conflicts from now on.
-
-    case DownloadItem::CANCELLED:
-      // We no longer need the reservation if the download is being removed.
-
-    case DownloadItem::INTERRUPTED:
-      // The download filename will need to be re-generated when the download is
-      // restarted. Holding on to the reservation now would prevent the name
-      // from being used for a subsequent retry attempt.
-
-      BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, revoke_callback_);
-      delete this;
-      break;
-
-    case DownloadItem::MAX_DOWNLOAD_STATE:
-      // Compiler appeasement.
-      NOTREACHED();
+// Returns true if the given path is in use by a path reservation.
+bool IsPathReserved(const FilePath& path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  // No reservation map => no reservations.
+  if (g_reservation_map == NULL)
+    return false;
+  // Unfortunately path normalization doesn't work reliably for non-existant
+  // files. So given a FilePath, we can't derive a normalized key that we can
+  // use for lookups. We only expect a small number of concurrent downloads at
+  // any given time, so going through all of them shouldn't be too slow.
+  for (ReservationMap::const_iterator iter = g_reservation_map->begin();
+       iter != g_reservation_map->end(); ++iter) {
+    if (iter->second ==  path)

[Randy Smith (Not in Mondays), 2012/08/03 19:17:14]

nit: Extra space.

[asanka, 2012/08/03 19:50:48]

Done.

+      return true;
   }
+  return false;
 }
 
-void DownloadItemObserver::OnDownloadDestroyed(DownloadItem* download) {
-  // This shouldn't happen. We should catch either COMPLETE, CANCELLED, or
-  // INTERRUPTED first.
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, revoke_callback_);
-  delete this;
-}
-
-}  // namespace
-
-// static
-void DownloadPathReservationTracker::GetReservedPath(
-    DownloadItem& download_item,
-    const FilePath& target_path,
-    const FilePath& default_path,
-    bool uniquify_path,
-    const ReservedPathCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadPathReservationTracker* tracker =
-      DownloadPathReservationTracker::GetInstance();
-
-  // Attach an observer to the download item so that we know when the target
-  // path changes and/or the download is no longer active.
-  new DownloadItemObserver(
-      download_item,
-      base::Bind(&DownloadPathReservationTracker::Revoke,
-                 base::Unretained(tracker),
-                 download_item.GetGlobalId()),
-      base::Bind(&DownloadPathReservationTracker::Update,
-                 base::Unretained(tracker),
-                 download_item.GetGlobalId()));
-  // DownloadItemObserver deletes itself.
+// Returns true if the given path is in use by any path reservation or the
+// file system. Called on the FILE thread.
+bool IsPathInUse(const FilePath& path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  // If there is a reservation, then the path is in use.
+  if (IsPathReserved(path))
+    return true;
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadPathReservationTracker::ReserveInternal,
-                 base::Unretained(tracker), download_item.GetGlobalId(),
-                 target_path, default_path, uniquify_path, callback));
-}
+  // If the path exists in the file system, then the path is in use.
+  if (file_util::PathExists(path))
+    return true;
 
-DownloadPathReservationTracker::DownloadPathReservationTracker() {
-}
+  // If the .crdownload path exists in the file system, then the path is also in
+  // use. This is to avoid potential collisions for the intermediate path if
+  // there is a .crdownload left around.

[Randy Smith (Not in Mondays), 2012/08/03 19:17:14]

Hmmm.  Say a bit more about this use case?  My immediate reaction was "but we
want to cleanup those .crdownload files; isn't overwriting a good way to do
that?" but that's probably wrong when we do download resumption.  What are your
thoughts here?

[asanka, 2012/08/03 19:50:48]

Interrupted downloads are what I was thinking about. We release the reservation
if the download is interrupted because the DRT no longer has a good signal for
whether the reservation should be considered valid. After that if the
.crdownload file is still around, we assume that the target name is "in use" and
skip over.

[Randy Smith (Not in Mondays), 2012/08/03 20:45:23]

[As discussed offline]  I've looked at the current code, and gone back to before
my refactoring, and I think we've always deleted the download file if it fails
in error.  I'm somewhat worried that that's not the behavior being seen by
users, but it is what the code looks like.  Given this, I think we should
consider .crdownload files that we don't know about turds, and ignore them for
reservation purposes (though we'll obviously have to change this behavior in
some fashion for download resumption).

+  if (file_util::PathExists(download_util::GetCrDownloadPath(path)))
+    return true;
 
-DownloadPathReservationTracker::~DownloadPathReservationTracker() {
-  DCHECK_EQ(0u, reservations_.size());
+  return false;
 }
 
-void DownloadPathReservationTracker::ReserveInternal(
+// Called on the FILE thread to reserve a download path. This method:
+// - Creates directory |default_download_path| if it doesn't exist.
+// - Verifies that the parent directory of |suggested_path| exists and is
+//   writeable.
+// - Uniquifies |suggested_path| if |should_uniquify_path| is true.
+// - Schedules |callback| on the UI thread with the reserved path and a flag
+//   indicating whether the returned path has been successfully verified.
+void CreateReservation(
     DownloadId download_id,
     const FilePath& suggested_path,
     const FilePath& default_download_path,
     bool should_uniquify,
-    const ReservedPathCallback& callback) {
+    const DownloadPathReservationTracker::ReservedPathCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DCHECK(download_id.IsValid());
-  DCHECK(!ContainsKey(reservations_, download_id));
   DCHECK(suggested_path.IsAbsolute());
 
+  // Create a reservation map if one doesn't exist. It will be automatically
+  // deleted when all the reservations are revoked.
+  if (g_reservation_map == NULL)
+    g_reservation_map = new ReservationMap;
+
+  ReservationMap& reservations = *g_reservation_map;
+  DCHECK(!ContainsKey(reservations, download_id));
+
   FilePath target_path(suggested_path.NormalizePathSeparators());
   bool is_path_writeable = true;
   bool has_conflicts = false;
@@ -187,7 +147,9 @@ void DownloadPathReservationTracker::ReserveInternal(
 
   if (is_path_writeable && should_uniquify && IsPathInUse(target_path)) {
     has_conflicts = true;
-    for (int uniquifier = 1; uniquifier <= kMaxUniqueFiles; ++uniquifier) {
+    for (int uniquifier = 1;
+         uniquifier <= DownloadPathReservationTracker::kMaxUniqueFiles;
+         ++uniquifier) {
       FilePath path_to_check(target_path.InsertBeforeExtensionASCII(
           StringPrintf(" (%d)", uniquifier)));
       if (!IsPathInUse(path_to_check)) {
@@ -197,59 +159,125 @@ void DownloadPathReservationTracker::ReserveInternal(
       }
     }
   }
-  reservations_[download_id] = target_path;
+  reservations[download_id] = target_path;
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(callback, target_path, (is_path_writeable && !has_conflicts)));
 }
 
-bool DownloadPathReservationTracker::IsPathInUse(const FilePath& path) const {
-  // Unfortunately path normalization doesn't work reliably for non-existant
-  // files. So given a FilePath, we can't derive a normalized key that we can
-  // use for lookups. We only expect a small number of concurrent downloads at
-  // any given time, so going through all of them shouldn't be too slow.
-  for (ReservationMap::const_iterator iter = reservations_.begin();
-       iter != reservations_.end(); ++iter) {
-    if (iter->second ==  path)
-      return true;
-  }
-  // If the path exists in the file system, then the path is in use.
-  if (file_util::PathExists(path))
-    return true;
-
-  // If the .crdownload path exists in the file system, then the path is also in
-  // use. This is to avoid potential collisions for the intermediate path if
-  // there is a .crdownload left around.
-  if (file_util::PathExists(download_util::GetCrDownloadPath(path)))
-    return true;
-
-  return false;
-}
-
-void DownloadPathReservationTracker::Update(DownloadId download_id,
-                                            const FilePath& new_path) {
+// Called on the FILE thread to update the path of the reservation associated
+// with |download_id| to |new_path|.
+void UpdateReservation(DownloadId download_id, const FilePath& new_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  ReservationMap::iterator iter = reservations_.find(download_id);
-  if (iter != reservations_.end()) {
+  DCHECK(g_reservation_map != NULL);
+  ReservationMap::iterator iter = g_reservation_map->find(download_id);
+  if (iter != g_reservation_map->end()) {
     iter->second = new_path;
   } else {
-    // This would happen if an Update() notification was scheduled on the FILE
-    // thread before ReserveInternal(), or after a Revoke() call. Neither should
-    // happen.
+    // This would happen if an UpdateReservation() notification was scheduled on
+    // the FILE thread before ReserveInternal(), or after a Revoke()
+    // call. Neither should happen.
     NOTREACHED();
   }
 }
 
-void DownloadPathReservationTracker::Revoke(DownloadId download_id) {
+// Called on the FILE thread to remove the path reservation associated with
+// |download_id|.
+void RevokeReservation(DownloadId download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(ContainsKey(reservations_, download_id));
-  reservations_.erase(download_id);
+  DCHECK(g_reservation_map != NULL);
+  DCHECK(ContainsKey(*g_reservation_map, download_id));
+  g_reservation_map->erase(download_id);
+  if (g_reservation_map->size() == 0) {
+    // No more reservations. Delete map.
+    delete g_reservation_map;
+    g_reservation_map = NULL;
+  }
+}
+
+DownloadItemObserver::DownloadItemObserver(DownloadItem& download_item)
+    : download_item_(download_item),
+      last_target_path_(download_item.GetTargetFilePath()) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  download_item_.AddObserver(this);
+}
+
+DownloadItemObserver::~DownloadItemObserver() {
+  download_item_.RemoveObserver(this);
+}
+
+void DownloadItemObserver::OnDownloadUpdated(DownloadItem* download) {
+  switch (download->GetState()) {
+    case DownloadItem::IN_PROGRESS: {
+      // Update the reservation.
+      FilePath new_target_path = download->GetTargetFilePath();
+      if (new_target_path != last_target_path_) {
+        BrowserThread::PostTask(
+            BrowserThread::FILE, FROM_HERE,
+            base::Bind(&UpdateReservation, download->GetGlobalId(),
+                       new_target_path));
+        last_target_path_ = new_target_path;
+      }
+      break;
+    }
+
+    case DownloadItem::COMPLETE:
+      // If the download is complete, then it has already been renamed to the
+      // final name. The existence of the file on disk is sufficient to prevent
+      // conflicts from now on.
+
+    case DownloadItem::CANCELLED:
+      // We no longer need the reservation if the download is being removed.
+
+    case DownloadItem::INTERRUPTED:
+      // The download filename will need to be re-generated when the download is
+      // restarted. Holding on to the reservation now would prevent the name
+      // from being used for a subsequent retry attempt.
+
+      BrowserThread::PostTask(
+          BrowserThread::FILE, FROM_HERE,
+          base::Bind(&RevokeReservation, download->GetGlobalId()));
+      delete this;
+      break;
+
+    case DownloadItem::MAX_DOWNLOAD_STATE:
+      // Compiler appeasement.
+      NOTREACHED();
+  }
 }
 
+void DownloadItemObserver::OnDownloadDestroyed(DownloadItem* download) {
+  // This shouldn't happen. We should catch either COMPLETE, CANCELLED, or
+  // INTERRUPTED first.
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&RevokeReservation, download->GetGlobalId()));
+  delete this;
+}
+
+}  // namespace
+
 // static
-DownloadPathReservationTracker* DownloadPathReservationTracker::GetInstance() {
+void DownloadPathReservationTracker::GetReservedPath(
+    DownloadItem& download_item,
+    const FilePath& target_path,
+    const FilePath& default_path,
+    bool uniquify_path,
+    const ReservedPathCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  static base::LazyInstance<DownloadPathReservationTracker>
-      reservation_tracker = LAZY_INSTANCE_INITIALIZER;
-  return reservation_tracker.Pointer();
+  // Attach an observer to the download item so that we know when the target
+  // path changes and/or the download is no longer active.
+  new DownloadItemObserver(download_item);
+  // DownloadItemObserver deletes itself.
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&CreateReservation, download_item.GetGlobalId(),
+                 target_path, default_path, uniquify_path, callback));
+}
+
+// static
+bool DownloadPathReservationTracker::IsPathInUseForTesting(
+    const FilePath& path) {
+  return IsPathInUse(path);
 }