Make DownloadHistory observe manager, items

chrome/browser/download/download_history.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // 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 using an
+// AllDownloadItemNotifier.
+//
+// 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.
+//
+// Strictly speaking, the weak pointers in the callbacks from the history system
+// are redundant with the CancelableRequestConsumer.
+// TODO(benjhayden) Use PostTaskAndReply with the weak pointers instead of
+// CancelableRequestConsumer. This requires modifying the downloads-related
+// portion of the  HistoryService interface.
+
 #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"
-
-using content::DownloadItem;
-using content::DownloadPersistentStoreInfo;
-
-DownloadHistory::DownloadHistory(Profile* profile)
-    : profile_(profile),
-      next_fake_db_handle_(DownloadItem::kUninitializedHandle - 1) {
-  DCHECK(profile);
-}
-
-DownloadHistory::~DownloadHistory() {}
-
-void DownloadHistory::GetNextId(
-    const HistoryService::DownloadNextIdCallback& callback) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfile(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
-    return;
-
-  hs->GetNextDownloadId(&history_consumer_, callback);
-}
-
-void DownloadHistory::Load(
-    const HistoryService::DownloadQueryCallback& callback) {
-  HistoryService* hs = HistoryServiceFactory::GetForProfile(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
-    return;
-
-  hs->QueryDownloads(&history_consumer_, callback);
-
-  // This is the initial load, so do a cleanup of corrupt in-progress entries.
-  hs->CleanUpInProgressEntries();
+#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[];
+
+  bool is_adding_;
+  int64 db_handle_;
+  scoped_ptr<DownloadPersistentStoreInfo> info_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadHistoryData);
+};
+
+const char DownloadHistoryData::kKey[] =
+  "DownloadItem DownloadHistoryData";
+
+DownloadPersistentStoreInfo GetPersistentStoreInfo(
+    content::DownloadItem* item) {
+  DownloadHistoryData* data = DownloadHistoryData::Get(item);
+  return DownloadPersistentStoreInfo(
+      item->GetFullPath(),
+      item->GetURL(),
+      item->GetReferrerUrl(),
+      item->GetStartTime(),
+      item->GetEndTime(),
+      item->GetReceivedBytes(),
+      item->GetTotalBytes(),
+      item->GetState(),
+      ((data != NULL) ? data->db_handle()
+       : history::DownloadDatabase::kUninitializedHandle),
+      item->GetOpened());
+}
+
+bool ShouldUpdateHistory(const DownloadPersistentStoreInfo* previous,
+                         const DownloadPersistentStoreInfo& current) {
+  // Ignore url, referrer, start_time, db_handle, which don't change.
+  return ((previous == NULL) ||
+          (previous->path != current.path) ||
+          (previous->end_time != current.end_time) ||
+          (previous->received_bytes != current.received_bytes) ||
+          (previous->total_bytes != current.total_bytes) ||
+          (previous->state != current.state) ||
+          (previous->opened != current.opened));
+}
+
+typedef std::vector<DownloadPersistentStoreInfo> InfoVector;
+
+}  // anonymous namespace
+
+DownloadHistory::Observer::Observer() {}
+DownloadHistory::Observer::~Observer() {}
+
+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::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);
 }
 
 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;
-    }
-  }
-  callback.Run(false);
-}
-
-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());
-    return;
-  }
-
-  int32 id = download_item->GetId();
-  DownloadPersistentStoreInfo history_info =
-      download_item->GetPersistentStoreInfo();
-  hs->CreateDownload(id, history_info, &history_consumer_, callback);
-}
-
-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)
-    return;
-
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (!hs)
-    return;
-  hs->UpdateDownload(download_item->GetPersistentStoreInfo());
-}
-
-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)
-    return;
-
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->UpdateDownloadPath(new_path, download_item->GetDbHandle());
-}
-
-void DownloadHistory::RemoveEntry(DownloadItem* download_item) {
-  // No update necessary if the download was initiated while in incognito mode.
-  if (download_item->GetDbHandle() <= DownloadItem::kUninitializedHandle)
-    return;
-
-  HistoryService* hs = HistoryServiceFactory::GetForProfileIfExists(
-      profile_, Profile::EXPLICIT_ACCESS);
-  if (hs)
-    hs->RemoveDownload(download_item->GetDbHandle());
-}
-
-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);
-}
-
-int64 DownloadHistory::GetNextFakeDbHandle() {
-  return next_fake_db_handle_--;
-}
-
-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);
+  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(
+    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::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* data = 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, data->db_handle());
+    ++history_size_;
+  }
+  notifier_.GetManager()->CheckForHistoryFilesRemoval();
+}
+
+void DownloadHistory::MaybeAddToHistory(content::DownloadItem* item) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  int32 download_id = item->GetId();
+  DownloadHistoryData* data = DownloadHistoryData::Get(item);
+  bool removing = (removing_handles_.find(data->db_handle()) !=
+                   removing_handles_.end());
+
+  // TODO(benjhayden): Remove IsTemporary().
+  if (download_crx_util::IsExtensionDownload(*item) ||
+      item->IsTemporary() ||
+      data->is_adding() ||
+      data->is_persisted() ||
+      removing)
+    return;
+
+  data->set_is_adding(true);
+  if (data->info() == NULL) {
+    // Keep the info here regardless of whether the item is in progress so that,
+    // when ItemAdded() calls OnDownloadUpdated(), it can decide whether to
+    // Update the db and/or clear the info.
+    data->set_info(GetPersistentStoreInfo(item));
+  }
+
+  history_->CreateDownload(*data->info(), &history_consumer_, base::Bind(
+      &DownloadHistory::ItemAdded, weak_ptr_factory_.GetWeakPtr(),
+      download_id));
+}
+
+void DownloadHistory::ItemAdded(int32 download_id, int64 db_handle) {

[asanka, 2012/11/02 19:56:57]

If CreateDownload() failed, then db_handle == 0. Does that case need special
handling?

[benjhayden, 2012/11/06 20:01:14]

Yes and no.
I made DownloadDatabase::CreateDownload return kUninitializedHandle so that we
only have one special value.
I made DownloadHistory::ItemAdded unset is_adding regardless of db_handle, but
return early right after that, without automatically retrying. The next
OnDownloadUpdated/OnDownloadOpened, it will re-try adding it to the db.
How does that look?

+  if (removed_while_adding_.find(download_id) !=
+      removed_while_adding_.end()) {
+    removed_while_adding_.erase(download_id);
+    ScheduleRemoveDownload(download_id, db_handle);
+    return;
+  }
+
+  if (!notifier_.GetManager())
+    return;
+
+  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* data = DownloadHistoryData::Get(item);
+  data->set_is_adding(false);
+  DCHECK_NE(db_handle, history::DownloadDatabase::kUninitializedHandle);
+  data->set_db_handle(db_handle);
+
+  // Send to observers the actual DownloadPersistentStoreInfo that was sent to
+  // the db, plus the db_handle, instead of completely regenerating the
+  // DownloadPersistentStoreInfo, in order to accurately reflect the contents of
+  // the database.
+  data->info()->db_handle = db_handle;
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadStored(
+      item, *data->info()));
+
+  // 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);
+}
+
+void DownloadHistory::OnDownloadCreated(
+    content::DownloadManager* manager, content::DownloadItem* item) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // All downloads should pass through OnDownloadCreated exactly once.
+  CHECK(!DownloadHistoryData::Get(item));
+  DownloadHistoryData* data = new DownloadHistoryData(item, loading_db_handle_);
+  loading_db_handle_ = history::DownloadDatabase::kUninitializedHandle;
+  if (item->GetState() == content::DownloadItem::IN_PROGRESS) {
+    data->set_info(GetPersistentStoreInfo(item));
+  }
+  MaybeAddToHistory(item);
+}
+
+void DownloadHistory::OnDownloadUpdated(
+    content::DownloadManager* manager, content::DownloadItem* item) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  DownloadHistoryData* data = DownloadHistoryData::Get(item);
+  if (!data->is_persisted()) {
+    MaybeAddToHistory(item);
+    return;
+  }
+  if (item->IsTemporary()) {
+    OnDownloadRemoved(notifier_.GetManager(), item);
+    return;
+  }
+
+  // TODO(asanka): Persist GetTargetFilePath() as well.
+  DownloadPersistentStoreInfo current_info(GetPersistentStoreInfo(item));
+  bool should_update = ShouldUpdateHistory(data->info(), current_info);
+  UMA_HISTOGRAM_ENUMERATION("Download.HistoryPropagatedUpdate",
+                            should_update, 2);
+  if (should_update) {
+    history_->UpdateDownload(current_info);
+    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadStored(
+        item, current_info));
+  }
+  if (item->GetState() == content::DownloadItem::IN_PROGRESS) {
+    data->set_info(current_info);
+  } else {
+    data->clear_info();
+  }
+}
+
+void DownloadHistory::OnDownloadOpened(
+    content::DownloadManager* manager, content::DownloadItem* item) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DownloadHistoryData* data = DownloadHistoryData::Get(item);
+  if (!data->is_persisted()) {
+    MaybeAddToHistory(item);
+    return;
+  }
+  if (item->IsTemporary()) {
+    OnDownloadRemoved(manager, item);
+    return;
+  }
+  // Downloads may be opened after they are completed, so don't rely on
+  // DownloadHistoryData::info().
+
+  DownloadPersistentStoreInfo current_info(GetPersistentStoreInfo(item));
+  history_->UpdateDownload(current_info);
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadStored(item, current_info));
+}
+
+void DownloadHistory::OnDownloadRemoved(
+    content::DownloadManager* manager, content::DownloadItem* item) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  DownloadHistoryData* data = DownloadHistoryData::Get(item);
+  if (!data->is_persisted()) {
+    if (data->is_adding()) {
+      // ScheduleRemoveDownload will be called when history_ calls ItemAdded().
+      removed_while_adding_.insert(item->GetId());
+    }
+    return;
+  }
+  ScheduleRemoveDownload(item->GetId(), data->db_handle());
+  data->set_db_handle(history::DownloadDatabase::kUninitializedHandle);
+  // ItemAdded increments history_size_ only if the item wasn't
+  // removed_while_adding_, so the next line does not belong in
+  // ScheduleRemoveDownload().
+  --history_size_;
+}
+
+void DownloadHistory::ScheduleRemoveDownload(
+    int32 download_id, int64 db_handle) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // For database efficiency, batch removals together if they happen all at
+  // once.
+  if (removing_handles_.empty()) {
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadHistory::RemoveDownloadsBatch,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+  removing_handles_.insert(db_handle);
+  removing_ids_.insert(download_id);
+}
+
+void DownloadHistory::RemoveDownloadsBatch() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  HandleSet remove_handles;
+  IdSet remove_ids;
+  removing_handles_.swap(remove_handles);
+  removing_ids_.swap(remove_ids);
+  history_->RemoveDownloads(remove_handles);
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadsRemoved(remove_ids));
+}