Refactor History's CommitLaterTask with CancelableCallback

components/history/core/browser/history_backend.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.
 
 #include "components/history/core/browser/history_backend.h"
 
 #include <algorithm>
 #include <functional>
 #include <list>
 #include <map>
@@ skipping 96 matching lines @@
   } else {
     mv.redirects = redirects;
     if (mv.redirects.back() != mv.url) {
       // The last url must be the target url.
       mv.redirects.push_back(mv.url);
     }
   }
   return mv;
 }
 
-// This task is run on a timer so that commits happen at regular intervals
-// so they are batched together. The important thing about this class is that
-// it supports canceling of the task so the reference to the backend will be
-// freed. The problem is that when history is shutting down, there is likely
-// to be one of these commits still pending and holding a reference.
-//
-// The backend can call Cancel to have this task release the reference. The
-// task will still run (if we ever get to processing the event before
-// shutdown), but it will not do anything.
-//
-// Note that this is a refcounted object and is not a task in itself. It should
-// be assigned to a RunnableMethod.
-//
-// TODO(brettw): bug 1165182: This should be replaced with a

[Marc Treib, 2017/02/02 10:52:21]

Wow, this might be the oldest Chrome bug I've seen :D

-// base::WeakPtrFactory which will handle everything automatically (like we do
-// in ExpireHistoryBackend).
-class CommitLaterTask : public base::RefCounted<CommitLaterTask> {
- public:
-  explicit CommitLaterTask(HistoryBackend* history_backend)
-      : history_backend_(history_backend) {}
-
-  // The backend will call this function if it is being destroyed so that we
-  // release our reference.
-  void Cancel() { history_backend_ = nullptr; }
-
-  void RunCommit() {
-    if (history_backend_)
-      history_backend_->Commit();
-  }
-
- private:
-  friend class base::RefCounted<CommitLaterTask>;
-
-  ~CommitLaterTask() {}
-
-  scoped_refptr<HistoryBackend> history_backend_;
-};
-
 QueuedHistoryDBTask::QueuedHistoryDBTask(
     std::unique_ptr<HistoryDBTask> task,
     scoped_refptr<base::SingleThreadTaskRunner> origin_loop,
     const base::CancelableTaskTracker::IsCanceledCallback& is_canceled)
     : task_(std::move(task)),
       origin_loop_(origin_loop),
       is_canceled_(is_canceled) {
   DCHECK(task_);
   DCHECK(origin_loop_);
   DCHECK(!is_canceled_.is_null());
@@ skipping 42 matching lines @@
 HistoryBackend::HistoryBackend(
     Delegate* delegate,
     std::unique_ptr<HistoryBackendClient> backend_client,
     scoped_refptr<base::SequencedTaskRunner> task_runner)
     : delegate_(delegate),
       scheduled_kill_db_(false),
       expirer_(this, backend_client.get(), task_runner),
       recent_redirects_(kMaxRedirectCount),
       segment_queried_(false),
       backend_client_(std::move(backend_client)),
-      task_runner_(task_runner) {}
+      task_runner_(task_runner),
+      weak_ptr_factory_(this) {}
 
 HistoryBackend::~HistoryBackend() {
-  DCHECK(!scheduled_commit_) << "Deleting without cleanup";
+  DCHECK(!scheduled_commit_tracker_.HasTrackedTasks())
+      << "Deleting without cleanup";
   queued_history_db_tasks_.clear();
 
   // Release stashed embedder object before cleaning up the databases.
   supports_user_data_helper_.reset();
 
   // First close the databases before optionally running the "destroy" task.
   CloseAllDatabases();
 
   if (!backend_destroy_task_.is_null()) {
     // Notify an interested party (typically a unit test) that we're done.
@@ skipping 26 matching lines @@
 void HistoryBackend::SetOnBackendDestroyTask(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
     const base::Closure& task) {
   if (!backend_destroy_task_.is_null())
     DLOG(WARNING) << "Setting more than one destroy task, overriding";
   backend_destroy_task_runner_ = std::move(task_runner);
   backend_destroy_task_ = task;
 }
 
 void HistoryBackend::Closing() {
-  // Any scheduled commit will have a reference to us, we must make it
-  // release that reference before we can be destroyed.
+  weak_ptr_factory_.InvalidateWeakPtrs();
   CancelScheduledCommit();
 
   // Release our reference to the delegate, this reference will be keeping the
   // history service alive.
   delegate_.reset();
 }
 
 #if defined(OS_IOS)
 void HistoryBackend::PersistState() {
   Commit();
@@ skipping 1956 matching lines @@
 
   if (thumbnail_db_) {
     thumbnail_db_->CommitTransaction();
     DCHECK_EQ(thumbnail_db_->transaction_nesting(), 0)
         << "Somebody left a transaction open";
     thumbnail_db_->BeginTransaction();
   }
 }
 
 void HistoryBackend::ScheduleCommit() {
-  if (scheduled_commit_)
+  if (scheduled_commit_tracker_.HasTrackedTasks())
     return;
-  scheduled_commit_ = new CommitLaterTask(this);
-  task_runner_->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&CommitLaterTask::RunCommit, scheduled_commit_),
+  scheduled_commit_tracker_.PostDelayedTask(
+      task_runner_.get(), FROM_HERE,
+      base::Bind(&HistoryBackend::Commit, weak_ptr_factory_.GetWeakPtr()),
       base::TimeDelta::FromSeconds(kCommitIntervalSeconds));
 }
 
 void HistoryBackend::CancelScheduledCommit() {
-  if (scheduled_commit_) {
-    scheduled_commit_->Cancel();
-    scheduled_commit_ = nullptr;
-  }
+  scheduled_commit_tracker_.TryCancelAll();
 }
 
 void HistoryBackend::ProcessDBTaskImpl() {
   if (!db_) {
     // db went away, release all the refs.
     queued_history_db_tasks_.clear();
     return;
   }
 
   // Remove any canceled tasks.
@@ skipping 377 matching lines @@
   // transaction is currently open.
   db_->CommitTransaction();
   db_->Vacuum();
   db_->BeginTransaction();
   db_->GetStartDate(&first_recorded_time_);
 
   return true;
 }
 
 }  // namespace history