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.

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

Suggestion: Modify now that it's not doing so directly?

[benjhayden, 2012/11/02 17:21:37]

Done.

+// 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.

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

Worth at least a note that this is redundant; I'm even inclined to say that we
shouldn't do it while we're still using CancelableRequestConsumer.   Having
multiple redundant coding paths that are being relied is confusing and can lead
to introduction of bugs that aren't caught.
Having said that, this code is more self-contained with the weak pointer
callbacks, so I'm ok with it.  But I would like a comment.

[benjhayden, 2012/11/02 17:21:37]

Done.

+// 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) {

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

Maybe add some blank lines in this function to group related functionality?  I
find this large a block of text without breaks daunting.

[benjhayden, 2012/11/02 17:21:37]

Done.

+  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) ||

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

Suggestion: Given that you've abstracted out GetPersistentStoreInfo into a
separate function, maybe this booleans worth while abstracting out and putting
near that function so they get modified together?  I'm tempted to suggest you
put both methods in a separate class to make very clear that they're linked, but
two methods and no state is probably not enough to justify creating a class.

[benjhayden, 2012/11/02 17:21:37]

Done.

+      (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) {
+    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);
+}