Make DownloadHistory observe manager, items

chrome/browser/download/download_history.cc

Index: chrome/browser/download/download_history.cc
diff --git a/chrome/browser/download/download_history.cc b/chrome/browser/download/download_history.cc
index 9d701950dcb5375eba799c55229a3e71a0588c42..a275e2f0c985fc56cf0ec8e785965d2637f40fbe 100644
--- a/chrome/browser/download/download_history.cc
+++ b/chrome/browser/download/download_history.cc
@@ -2,153 +2,392 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// DownloadHistory manages persisting DownloadItems to the history service by
+// observing a single DownloadManager and all its DownloadItems.
+// DownloadHistory decides whether and when to add items to, remove items from,
+// and update items in the database. DownloadHistory uses DownloadHistoryData to
+// store per-DownloadItem data such as its db_handle, whether the item is being
+// added and waiting for its db_handle, and the last DownloadPersistentStoreInfo
+// that was passed to the database.  When the DownloadManager and its delegate
+// (ChromeDownloadManagerDelegate) are initialized, DownloadHistory is created
+// and queries the HistoryService. When the HistoryService calls back from
+// QueryDownloads() to QueryCallback(), DownloadHistory uses
+// DownloadManager::CreateDownloadItem() to inform DownloadManager of these
+// persisted DownloadItems. CreateDownloadItem() internally calls
+// OnDownloadCreated(), which normally adds items to the database, so
+// QueryCallback() uses |loading_db_handle_| to disable adding these items to
+// the database as it matches them up with their db_handles.  If a download is
+// removed via OnDownloadRemoved() while the item is still being added to the
+// database, DownloadHistory uses |removed_while_adding_| to remember to remove
+// the item when its ItemAdded() callback is called.  All callbacks are bound
+// with a weak pointer to DownloadHistory to prevent use-after-free bugs.
+// ChromeDownloadManagerDelegate owns DownloadHistory, and deletes it in
+// Shutdown(), which is called by DownloadManagerImpl::Shutdown() after all
+// DownloadItems are destroyed.
+
 #include "chrome/browser/download/download_history.h"
 
-#include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "chrome/browser/download/download_crx_util.h"
-#include "chrome/browser/history/history_marshaling.h"
-#include "chrome/browser/history/history_service_factory.h"
-#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/history/download_database.h"
+#include "chrome/browser/history/download_persistent_store_info.h"
+#include "chrome/browser/history/history.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_item.h"
-#include "content/public/browser/download_persistent_store_info.h"
+#include "content/public/browser/download_manager.h"
+
+namespace {
+
+// Per-DownloadItem data. This information does not belong inside DownloadItem,
+// and keeping maps in DownloadHistory from DownloadItem to this information is
+// error-prone and complicated. Unfortunately, DownloadHistory::removing_ and
+// removed_while_adding_ cannot be moved into this class partly because
+// DownloadHistoryData is destroyed when DownloadItems are destroyed, and we
+// have no control over when DownloadItems are destroyed.
+class DownloadHistoryData : public base::SupportsUserData::Data {
+ public:
+  static DownloadHistoryData* Get(content::DownloadItem* item) {
+    base::SupportsUserData::Data* data = item->GetUserData(kKey);
+    return (data == NULL) ? NULL :
+      static_cast<DownloadHistoryData*>(data);
+  }
+
+  DownloadHistoryData(content::DownloadItem* item, int64 handle)
+    : is_adding_(false),
+      db_handle_(handle),
+      info_(NULL) {
+    item->SetUserData(kKey, this);
+  }
+
+  virtual ~DownloadHistoryData() {
+  }
+
+  // Whether this item is currently being added to the database.
+  bool is_adding() const { return is_adding_; }
+  void set_is_adding(bool a) { is_adding_ = a; }
+
+  // Whether this item is already persisted in the database.
+  bool is_persisted() const {
+    return db_handle_ != history::DownloadDatabase::kUninitializedHandle;
+  }
+
+  int64 db_handle() const { return db_handle_; }
+  void set_db_handle(int64 h) { db_handle_ = h; }
+
+  // This allows DownloadHistory::OnDownloadUpdated() to see what changed in a
+  // DownloadItem if anything, in order to prevent writing to the database
+  // unnecessarily.  It is nullified when the item is no longer in progress in
+  // order to save memory.
+  DownloadPersistentStoreInfo* info() { return info_.get(); }
+  void set_info(const DownloadPersistentStoreInfo& i) {
+    info_.reset(new DownloadPersistentStoreInfo(i));
+  }
+  void clear_info() {
+    info_.reset();
+  }
+
+ private:
+  static const char kKey[];
 
-using content::DownloadItem;
-using content::DownloadPersistentStoreInfo;
+  bool is_adding_;
+  int64 db_handle_;
+  scoped_ptr<DownloadPersistentStoreInfo> info_;
 
-DownloadHistory::DownloadHistory(Profile* profile)
-    : profile_(profile),
-      next_fake_db_handle_(DownloadItem::kUninitializedHandle - 1) {
-  DCHECK(profile);
+  DISALLOW_COPY_AND_ASSIGN(DownloadHistoryData);
+};
+
+const char DownloadHistoryData::kKey[] =
+  "DownloadItem DownloadHistoryData";
+
+DownloadPersistentStoreInfo GetPersistentStoreInfo(
+    content::DownloadItem* item) {
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  return DownloadPersistentStoreInfo(
+      item->GetFullPath(),
+      item->GetURL(),
+      item->GetReferrerUrl(),
+      item->GetStartTime(),
+      item->GetEndTime(),
+      item->GetReceivedBytes(),
+      item->GetTotalBytes(),
+      item->GetState(),
+      ((dhd != NULL) ? dhd->db_handle()
+       : history::DownloadDatabase::kUninitializedHandle),
+      item->GetOpened());
 }
 
-DownloadHistory::~DownloadHistory() {}
+typedef std::vector<DownloadPersistentStoreInfo> InfoVector;
 
-void DownloadHistory::GetNextId(
-    const HistoryService::DownloadNextIdCallback& callback) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfile(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
-    return;
+}  // anonymous namespace
+
+DownloadHistory::Observer::Observer() {}
+DownloadHistory::Observer::~Observer() {}
 
-  hs->GetNextDownloadId(&history_consumer_, callback);
+DownloadHistory::DownloadHistory(
+    content::DownloadManager* manager,
+    HistoryService* history)
+  : ALLOW_THIS_IN_INITIALIZER_LIST(notifier_(manager, this)),
+    history_(history),
+    loading_db_handle_(history::DownloadDatabase::kUninitializedHandle),
+    history_size_(0),
+    ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  content::DownloadManager::DownloadVector items;
+  notifier_.GetManager()->GetAllDownloads(&items);
+  for (content::DownloadManager::DownloadVector::const_iterator
+       it = items.begin(); it != items.end(); ++it) {
+    OnDownloadCreated(notifier_.GetManager(), *it);
+  }
+  history_->QueryDownloads(&history_consumer_, base::Bind(
+      &DownloadHistory::QueryCallback, weak_ptr_factory_.GetWeakPtr()));
+}
+
+DownloadHistory::~DownloadHistory() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 }
 
-void DownloadHistory::Load(
-    const HistoryService::DownloadQueryCallback& callback) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfile(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
+void DownloadHistory::QueryCallback(InfoVector* infos) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  // ManagerGoingDown() may have happened before the history loaded.
+  if (!notifier_.GetManager())
     return;
+  for (InfoVector::const_iterator it = infos->begin();
+       it != infos->end(); ++it) {
+    // OnDownloadCreated() is called inside DM::CreateDownloadItem(), so set
+    // loading_db_handle_ to match up the created item with its db_handle.  All
+    // methods run on the UI thread and CreateDownloadItem() is synchronous.
+    loading_db_handle_ = it->db_handle;
+    content::DownloadItem* download_item =
+      notifier_.GetManager()->CreateDownloadItem(
+        it->path,
+        it->url,
+        it->referrer_url,
+        it->start_time,
+        it->end_time,
+        it->received_bytes,
+        it->total_bytes,
+        it->state,
+        it->opened);
+    DownloadHistoryData* dhd = DownloadHistoryData::Get(download_item);
+
+    // If this DCHECK fails, then you probably added an Observer that
+    // synchronously creates a DownloadItem in response to
+    // DownloadManager::OnDownloadCreated(), and your observer runs before
+    // DownloadHistory, and DownloadManager creates items synchronously. Just
+    // bounce your DownloadItem creation off the message loop to flush
+    // DownloadHistory::OnDownloadCreated.
+    DCHECK_EQ(it->db_handle, dhd->db_handle());
+    ++history_size_;
+  }
+  notifier_.GetManager()->CheckForHistoryFilesRemoval();
+}
 
-  hs->QueryDownloads(&history_consumer_, callback);
+void DownloadHistory::OnDownloadCreated(
+    content::DownloadManager* manager, content::DownloadItem* item) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  // This is the initial load, so do a cleanup of corrupt in-progress entries.
-  hs->CleanUpInProgressEntries();
+  // All downloads should pass through OnDownloadCreated exactly once.
+  CHECK(!DownloadHistoryData::Get(item));
+  DownloadHistoryData* dhd = new DownloadHistoryData(item, loading_db_handle_);
+  loading_db_handle_ = history::DownloadDatabase::kUninitializedHandle;
+  if (item->GetState() == content::DownloadItem::IN_PROGRESS) {
+    dhd->set_info(GetPersistentStoreInfo(item));
+  }
+  MaybeAddToHistory(item);
 }
 
-void DownloadHistory::CheckVisitedReferrerBefore(
-    int32 download_id,
-    const GURL& referrer_url,
-    const VisitedBeforeDoneCallback& callback) {
-  if (referrer_url.is_valid()) {
-    HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-        profile_, Profile::EXPLICIT_ACCESS);
-    if (hs) {
-      HistoryService::Handle handle =
-          hs->GetVisibleVisitCountToHost(referrer_url, &history_consumer_,
-              base::Bind(&DownloadHistory::OnGotVisitCountToHost,
-                         base::Unretained(this)));
-      visited_before_requests_[handle] = callback;
-      return;
-    }
+void DownloadHistory::MaybeAddToHistory(content::DownloadItem* item) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  int32 download_id = item->GetId();
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  bool removing = (removing_.find(dhd->db_handle()) != removing_.end());
+  // TODO(benjhayden): Remove IsTemporary().
+  if (download_crx_util::IsExtensionDownload(*item) ||
+      dhd->is_persisted() ||
+      item->IsTemporary() ||
+      removing ||
+      dhd->is_adding())
+    return;
+  dhd->set_is_adding(true);
+  if (dhd->info() == NULL) {
+    // Keep the info here regardless of whether the item is in progress so
+    // that, when ItemAdded() calls OnDownloadUpdated(), it can choose more
+    // intelligently whether to Update the db and/or discard the info.
+    dhd->set_info(GetPersistentStoreInfo(item));
   }
-  callback.Run(false);
+  history_->CreateDownload(*dhd->info(), &history_consumer_, base::Bind(
+      &DownloadHistory::ItemAdded, weak_ptr_factory_.GetWeakPtr(),
+      download_id));
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadStored(*dhd->info()));
 }
 
-void DownloadHistory::AddEntry(
-    DownloadItem* download_item,
-    const HistoryService::DownloadCreateCallback& callback) {
-  DCHECK(download_item);
-  // Do not store the download in the history database for a few special cases:
-  // - incognito mode (that is the point of this mode)
-  // - extensions (users don't think of extension installation as 'downloading')
-  // - temporary download, like in drag-and-drop
-  // - history service is not available (e.g. in tests)
-  // We have to make sure that these handles don't collide with normal db
-  // handles, so we use a negative value. Eventually, they could overlap, but
-  // you'd have to do enough downloading that your ISP would likely stab you in
-  // the neck first. YMMV.
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (download_crx_util::IsExtensionDownload(*download_item) ||
-      download_item->IsTemporary() || !hs) {
-    callback.Run(download_item->GetId(), GetNextFakeDbHandle());
+void DownloadHistory::ItemAdded(int32 download_id, int64 db_handle) {
+  if (removed_while_adding_.find(download_id) !=
+      removed_while_adding_.end()) {
+    removed_while_adding_.erase(download_id);
+    if (removing_.empty()) {
+      content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+          base::Bind(&DownloadHistory::RemoveDownloadsBatch,
+                     weak_ptr_factory_.GetWeakPtr()));
+    }
+    removing_.insert(db_handle);
     return;
   }
 
-  int32 id = download_item->GetId();
-  DownloadPersistentStoreInfo history_info =
-      download_item->GetPersistentStoreInfo();
-  hs->CreateDownload(id, history_info, &history_consumer_, callback);
-}
+  if (!notifier_.GetManager())
+    return;
 
-void DownloadHistory::UpdateEntry(DownloadItem* download_item) {
-  // Don't store info in the database if the download was initiated while in
-  // incognito mode or if it hasn't been initialized in our database table.
-  if (download_item->GetDbHandle() <= DownloadItem::kUninitializedHandle)
+  content::DownloadItem* item = notifier_.GetManager()->GetDownload(
+      download_id);
+  if (!item) {
+    // This item will have called OnDownloadDestroyed().  If the item should
+    // have been removed from history, then it would have also called
+    // OnDownloadRemoved(), which would have put |download_id| in
+    // removed_while_adding_, handled above.
     return;
+  }
+
+  UMA_HISTOGRAM_CUSTOM_COUNTS("Download.HistorySize2",
+                              history_size_,
+                              0/*min*/,
+                              (1 << 23)/*max*/,
+                              (1 << 7)/*num_buckets*/);
+  ++history_size_;
+
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  dhd->set_is_adding(false);
+  DCHECK_NE(db_handle, history::DownloadDatabase::kUninitializedHandle);
+  dhd->set_db_handle(db_handle);
+
+  // In case the item changed or became temporary while it was being added.
+  // Don't just update all of the item's observers because we're the only
+  // observer that can also see db_handle, which is the only thing that
+  // ItemAdded changed.
+  OnDownloadUpdated(notifier_.GetManager(), item);
+}
 
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
+void DownloadHistory::OnDownloadUpdated(
+    content::DownloadManager* manager, content::DownloadItem* item) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  if (!dhd->is_persisted()) {
+    MaybeAddToHistory(item);
+    return;
+  }
+  if (item->IsTemporary()) {
+    OnDownloadRemoved(notifier_.GetManager(), item);
     return;
-  hs->UpdateDownload(download_item->GetPersistentStoreInfo());
+  }
+
+  // TODO(asanka): Persist GetTargetFilePath() as well.
+  DownloadPersistentStoreInfo current_info(GetPersistentStoreInfo(item));
+  DownloadPersistentStoreInfo* previous_info = dhd->info();
+  bool do_update = (
+      (previous_info == NULL) ||
+      (previous_info->path != current_info.path) ||
+      (previous_info->end_time != current_info.end_time) ||
+      (previous_info->received_bytes != current_info.received_bytes) ||
+      (previous_info->total_bytes != current_info.total_bytes) ||
+      (previous_info->state != current_info.state) ||
+      (previous_info->opened != current_info.opened));
+  UMA_HISTOGRAM_ENUMERATION("Download.HistoryPropagatedUpdate", do_update, 2);
+  if (do_update) {
+    history_->UpdateDownload(current_info);
+    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadStored(current_info));
+  }
+  if (item->GetState() == content::DownloadItem::IN_PROGRESS) {
+    dhd->set_info(current_info);
+  } else {
+    dhd->clear_info();
+  }
 }
 
-void DownloadHistory::UpdateDownloadPath(DownloadItem* download_item,
-                                         const FilePath& new_path) {
-  // No update necessary if the download was initiated while in incognito mode.
-  if (download_item->GetDbHandle() <= DownloadItem::kUninitializedHandle)
+// Downloads may be opened after they are completed.
+void DownloadHistory::OnDownloadOpened(
+    content::DownloadManager* manager, content::DownloadItem* item) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  if (!dhd->is_persisted()) {
+    MaybeAddToHistory(item);
+    return;
+  }
+  if (item->IsTemporary()) {
+    OnDownloadRemoved(manager, item);
     return;
+  }
 
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->UpdateDownloadPath(new_path, download_item->GetDbHandle());
+  DownloadPersistentStoreInfo current_info(GetPersistentStoreInfo(item));
+  history_->UpdateDownload(current_info);
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadStored(current_info));
+  if (item->GetState() == content::DownloadItem::IN_PROGRESS) {

[asanka, 2012/09/24 18:39:43]

I think this condition will always be false.

[benjhayden, 2012/09/24 20:12:11]

Done.

+    dhd->set_info(current_info);
+  }
 }
 
-void DownloadHistory::RemoveEntry(DownloadItem* download_item) {
-  // No update necessary if the download was initiated while in incognito mode.
-  if (download_item->GetDbHandle() <= DownloadItem::kUninitializedHandle)
+void DownloadHistory::OnDownloadRemoved(
+    content::DownloadManager* manager, content::DownloadItem* item) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  DownloadHistoryData* dhd = DownloadHistoryData::Get(item);
+  if (!dhd->is_persisted()) {
+    if (dhd->is_adding()) {
+      removed_while_adding_.insert(item->GetId());
+    }
     return;
+  }
 
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->RemoveDownload(download_item->GetDbHandle());
+  // For database efficiency, batch removals together if they happen all at
+  // once.
+  if (removing_.empty()) {
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadHistory::RemoveDownloadsBatch,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+  removing_.insert(dhd->db_handle());
+  dhd->set_db_handle(history::DownloadDatabase::kUninitializedHandle);
+  --history_size_;
 }
 
-void DownloadHistory::RemoveEntriesBetween(const base::Time remove_begin,
-                                           const base::Time remove_end) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->RemoveDownloadsBetween(remove_begin, remove_end);
+void DownloadHistory::RemoveDownloadsBatch() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  HandleSet remove_handles;
+  removing_.swap(remove_handles);
+  history_->RemoveDownloads(remove_handles);
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadsRemoved(remove_handles));
 }
 
-int64 DownloadHistory::GetNextFakeDbHandle() {
-  return next_fake_db_handle_--;
+void DownloadHistory::CheckVisitedReferrerBefore(
+    const GURL& referrer_url,
+    const VisitedBeforeDoneCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if (!referrer_url.is_valid()) {
+    callback.Run(false);
+    return;
+  }
+  history_->GetVisibleVisitCountToHost(
+      referrer_url, &history_consumer_, base::Bind(
+          &DownloadHistory::OnGotVisitCountToHost,
+          weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
-void DownloadHistory::OnGotVisitCountToHost(HistoryService::Handle handle,
-                                            bool found_visits,
-                                            int count,
-                                            base::Time first_visit) {
-  VisitedBeforeRequestsMap::iterator request =
-      visited_before_requests_.find(handle);
-  DCHECK(request != visited_before_requests_.end());
-  VisitedBeforeDoneCallback callback = request->second;
-  visited_before_requests_.erase(request);
+void DownloadHistory::OnGotVisitCountToHost(
+    const VisitedBeforeDoneCallback& callback,
+    HistoryService::Handle unused_handle,
+    bool found_visits,
+    int count,
+    base::Time first_visit) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   callback.Run(found_visits && count &&
       (first_visit.LocalMidnight() < base::Time::Now().LocalMidnight()));
 }
+
+void DownloadHistory::AddObserver(DownloadHistory::Observer* observer) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  observers_.AddObserver(observer);
+}
+
+void DownloadHistory::RemoveObserver(DownloadHistory::Observer* observer) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  observers_.RemoveObserver(observer);
+}