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

3 files changed, 175 insertions, 197 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);
 }
diff --git a/chrome/browser/download/download_path_reservation_tracker.h b/chrome/browser/download/download_path_reservation_tracker.h
index 50e7ca9..9bfeea7 100644
--- a/chrome/browser/download/download_path_reservation_tracker.h
+++ b/chrome/browser/download/download_path_reservation_tracker.h
@@ -5,12 +5,7 @@
 #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_PATH_RESERVATION_TRACKER_H_
 #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_PATH_RESERVATION_TRACKER_H_
 
-#include <map>
-
 #include "base/callback_forward.h"
-#include "base/file_path.h"
-#include "base/lazy_instance.h"
-#include "content/public/browser/download_id.h"
 
 // DownloadPathReservationTracker: Track download paths that are in use by
 // active downloads.
@@ -72,10 +67,12 @@ namespace content {
 class DownloadItem;
 }
 
+class FilePath;
+
 // Issues and tracks download paths that are in use by the download system. When
 // a target path is set for a download, this object tracks the path and the
 // associated download item so that subsequent downloads can avoid using the
-// same path. All non-static methods must be invoked on the FILE thread.
+// same path.
 class DownloadPathReservationTracker {
  public:
   // Callback used with |GetReservedPath|. |target_path| specifies the target
@@ -95,9 +92,8 @@ class DownloadPathReservationTracker {
   static const int kMaxUniqueFiles = 100;
 
   // Called on the UI thread to request a download path reservation. Begins
-  // observing |download_item| and invokes ReserveInternal() on the FILE thread
-  // to create the path reservation. Will not modify any state of
-  // |download_item|.
+  // observing |download_item| and initiates creating a reservation on the FILE
+  // thread. Will not modify any state of |download_item|.
   //
   // |default_download_path| is the user's default download path. If this
   // directory does not exist and is the parent directory of
@@ -108,47 +104,9 @@ class DownloadPathReservationTracker {
                               bool should_uniquify_path,
                               const ReservedPathCallback& callback);
 
- private:
-  friend class DownloadPathReservationTrackerTest;
-  friend struct base::DefaultLazyInstanceTraits<DownloadPathReservationTracker>;
-
-  typedef std::map<content::DownloadId, FilePath> ReservationMap;
-
-  DownloadPathReservationTracker();
-  ~DownloadPathReservationTracker();
-
-  // 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 ReserveInternal(content::DownloadId download_id,
-                       const FilePath& suggested_path,
-                       const FilePath& default_download_path,
-                       bool should_uniquify_path,
-                       const ReservedPathCallback& callback);
-
-  // 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) const;
-
-  // Called on the FILE thread to update the path of the reservation associated
-  // with |download_id| to |new_path|.
-  void Update(content::DownloadId download_id, const FilePath& new_path);
-
-  // Called on the FILE thread to remove the path reservation associated with
-  // |download_id|.
-  void Revoke(content::DownloadId download_id);
-
-  // Get a pointer to the browser global instace of this object. Called on the
-  // UI thread.
-  static DownloadPathReservationTracker* GetInstance();
-
-  ReservationMap reservations_;
-
-  DISALLOW_COPY_AND_ASSIGN(DownloadPathReservationTracker);
+  // Returns true if |path| is in use by an existing path reservation. Should
+  // only be called on the FILE thread. Currently only used by tests.
+  static bool IsPathInUseForTesting(const FilePath& path);
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_PATH_RESERVATION_TRACKER_H_
diff --git a/chrome/browser/download/download_path_reservation_tracker_unittest.cc b/chrome/browser/download/download_path_reservation_tracker_unittest.cc
index aceaf3b..a4085fc 100644
--- a/chrome/browser/download/download_path_reservation_tracker_unittest.cc
+++ b/chrome/browser/download/download_path_reservation_tracker_unittest.cc
@@ -61,8 +61,6 @@ class FakeDownloadItem : public MockDownloadItem {
   ObserverList<Observer> observers_;
 };
 
-}  // namespace
-
 class DownloadPathReservationTrackerTest : public testing::Test {
  public:
   DownloadPathReservationTrackerTest();
@@ -129,7 +127,7 @@ FilePath DownloadPathReservationTrackerTest::GetPathInDownloadsDirectory(
 }
 
 bool DownloadPathReservationTrackerTest::IsPathInUse(const FilePath& path) {
-  return DownloadPathReservationTracker::GetInstance()->IsPathInUse(path);
+  return DownloadPathReservationTracker::IsPathInUseForTesting(path);
 }
 
 void DownloadPathReservationTrackerTest::CallGetReservedPath(
@@ -160,6 +158,8 @@ void DownloadPathReservationTrackerTest::TestReservedPathCallback(
   *return_verified = verified;
 }
 
+}  // namespace
+
 // A basic reservation is acquired and committed.
 TEST_F(DownloadPathReservationTrackerTest, BasicReservation) {
   scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1));
@@ -238,10 +238,10 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingFiles) {
   EXPECT_TRUE(IsPathInUse(path));
   EXPECT_TRUE(IsPathInUse(reserved_path));
   EXPECT_TRUE(verified);
-  // The path should be uniquified, skipping over foo.txt and
+  // The path should be uniquified, skipping over foo.txt but not over
   // "foo (1).txt.crdownload"
   EXPECT_EQ(
-      GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo (2).txt")).value(),
+      GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo (1).txt")).value(),
       reserved_path.value());
 
   item.reset();