Avoid LazyInstance in DownloadPathReservationTracker.

BUG=78085


Review URL: https://chromiumcodereview.appspot.com/10824132

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

1 files changed, 162 insertions, 142 deletions

diff --git a/chrome/browser/download/download_path_reservation_tracker.cc b/chrome/browser/download/download_path_reservation_tracker.cc
index cec8173..3b6a158 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 DownloadIds that
+// are mapped to the same path. This can happen if a reservation is created that
+// is supposed to overwrite an existing reservation.
+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,23 +54,144 @@ 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)
+// 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)
+      return true;
+  }
+  return false;
+}
+
+// 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;
+
+  // If the path exists in the file system, then the path is in use.
+  if (file_util::PathExists(path))
+    return true;
+
+  return false;
+}
+
+// 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 DownloadPathReservationTracker::ReservedPathCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(download_id.IsValid());
+  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;
+
+  // Create the default download path if it doesn't already exist and is where
+  // we are going to create the downloaded file. |target_path| might point
+  // elsewhere if this was a programmatic download.
+  if (!default_download_path.empty() &&
+      default_download_path == target_path.DirName() &&
+      !file_util::DirectoryExists(default_download_path)) {
+    file_util::CreateDirectory(default_download_path);
+  }
+
+  // Check writability of the suggested path. If we can't write to it, default
+  // to the user's "My Documents" directory. We'll prompt them in this case.
+  FilePath dir = target_path.DirName();
+  FilePath filename = target_path.BaseName();
+  if (!file_util::PathIsWritable(dir)) {
+    DVLOG(1) << "Unable to write to directory \"" << dir.value() << "\"";
+    is_path_writeable = false;
+    PathService::Get(chrome::DIR_USER_DOCUMENTS, &dir);
+    target_path = dir.Append(filename);
+  }
+
+  if (is_path_writeable && should_uniquify && IsPathInUse(target_path)) {
+    has_conflicts = true;
+    for (int uniquifier = 1;
+         uniquifier <= DownloadPathReservationTracker::kMaxUniqueFiles;
+         ++uniquifier) {
+      FilePath path_to_check(target_path.InsertBeforeExtensionASCII(
+          StringPrintf(" (%d)", uniquifier)));
+      if (!IsPathInUse(path_to_check)) {
+        target_path = path_to_check;
+        has_conflicts = false;
+        break;
+      }
+    }
+  }
+  reservations[download_id] = target_path;
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(callback, target_path, (is_path_writeable && !has_conflicts)));
+}
+
+// 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));
+  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 UpdateReservation() notification was scheduled on
+    // the FILE thread before ReserveInternal(), or after a Revoke()
+    // call. Neither should happen.
+    NOTREACHED();
+  }
+}
+
+// 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(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()),
-      revoke_callback_(revoke),
-      update_callback_(update) {
+      last_target_path_(download_item.GetTargetFilePath()) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   download_item_.AddObserver(this);
 }
@@ -74,8 +206,10 @@ void DownloadItemObserver::OnDownloadUpdated(DownloadItem* download) {
       // 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));
+        BrowserThread::PostTask(
+            BrowserThread::FILE, FROM_HERE,
+            base::Bind(&UpdateReservation, download->GetGlobalId(),
+                       new_target_path));
         last_target_path_ = new_target_path;
       }
       break;
@@ -98,7 +232,9 @@ void DownloadItemObserver::OnDownloadUpdated(DownloadItem* download) {
       // 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_);
+      BrowserThread::PostTask(
+          BrowserThread::FILE, FROM_HERE,
+          base::Bind(&RevokeReservation, download->GetGlobalId()));
       delete this;
       break;
 
@@ -124,135 +260,19 @@ void DownloadPathReservationTracker::GetReservedPath(
     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()));
+  new DownloadItemObserver(download_item);
   // DownloadItemObserver deletes itself.
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadPathReservationTracker::ReserveInternal,
-                 base::Unretained(tracker), download_item.GetGlobalId(),
+      base::Bind(&CreateReservation, download_item.GetGlobalId(),
                  target_path, default_path, uniquify_path, callback));
 }
 
-DownloadPathReservationTracker::DownloadPathReservationTracker() {
-}
-
-DownloadPathReservationTracker::~DownloadPathReservationTracker() {
-  DCHECK_EQ(0u, reservations_.size());
-}
-
-void DownloadPathReservationTracker::ReserveInternal(
-    DownloadId download_id,
-    const FilePath& suggested_path,
-    const FilePath& default_download_path,
-    bool should_uniquify,
-    const ReservedPathCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(download_id.IsValid());
-  DCHECK(!ContainsKey(reservations_, download_id));
-  DCHECK(suggested_path.IsAbsolute());
-
-  FilePath target_path(suggested_path.NormalizePathSeparators());
-  bool is_path_writeable = true;
-  bool has_conflicts = false;
-
-  // Create the default download path if it doesn't already exist and is where
-  // we are going to create the downloaded file. |target_path| might point
-  // elsewhere if this was a programmatic download.
-  if (!default_download_path.empty() &&
-      default_download_path == target_path.DirName() &&
-      !file_util::DirectoryExists(default_download_path)) {
-    file_util::CreateDirectory(default_download_path);
-  }
-
-  // Check writability of the suggested path. If we can't write to it, default
-  // to the user's "My Documents" directory. We'll prompt them in this case.
-  FilePath dir = target_path.DirName();
-  FilePath filename = target_path.BaseName();
-  if (!file_util::PathIsWritable(dir)) {
-    DVLOG(1) << "Unable to write to directory \"" << dir.value() << "\"";
-    is_path_writeable = false;
-    PathService::Get(chrome::DIR_USER_DOCUMENTS, &dir);
-    target_path = dir.Append(filename);
-  }
-
-  if (is_path_writeable && should_uniquify && IsPathInUse(target_path)) {
-    has_conflicts = true;
-    for (int uniquifier = 1; uniquifier <= kMaxUniqueFiles; ++uniquifier) {
-      FilePath path_to_check(target_path.InsertBeforeExtensionASCII(
-          StringPrintf(" (%d)", uniquifier)));
-      if (!IsPathInUse(path_to_check)) {
-        target_path = path_to_check;
-        has_conflicts = false;
-        break;
-      }
-    }
-  }
-  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) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  ReservationMap::iterator iter = reservations_.find(download_id);
-  if (iter != reservations_.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.
-    NOTREACHED();
-  }
-}
-
-void DownloadPathReservationTracker::Revoke(DownloadId download_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(ContainsKey(reservations_, download_id));
-  reservations_.erase(download_id);
-}
-
 // static
-DownloadPathReservationTracker* DownloadPathReservationTracker::GetInstance() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  static base::LazyInstance<DownloadPathReservationTracker>
-      reservation_tracker = LAZY_INSTANCE_INITIALIZER;
-  return reservation_tracker.Pointer();
+bool DownloadPathReservationTracker::IsPathInUseForTesting(
+    const FilePath& path) {
+  return IsPathInUse(path);
 }