Don't leak WebHistoryService::Request on HistoryService shutdown.

chrome/browser/history/history_service.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.
 
 // The history system runs on a background thread so that potentially slow
 // database operations don't delay the browser. This backend processing is
 // represented by HistoryBackend. The HistoryService's job is to dispatch to
 // that thread.
 //
 // Main thread                       History thread
@@ skipping 11 matching lines @@
 
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_service.h"
+#include "base/stl_util.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "chrome/browser/autocomplete/history_url_provider.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/history/download_row.h"
 #include "chrome/browser/history/history_backend.h"
@@ skipping 65 matching lines @@
     return itr_ != end_;
   }
 
  private:
   history::URLRows::const_iterator itr_;
   history::URLRows::const_iterator end_;
 
   DISALLOW_COPY_AND_ASSIGN(URLIteratorFromURLRows);
 };
 
-// Callback from WebHistoryService::ExpireWebHistory().
-void ExpireWebHistoryComplete(
-    history::WebHistoryService::Request* request,
-    bool success) {
-  // Ignore the result and delete the request.
-  delete request;
-}
-
 }  // namespace
 
 // Sends messages from the backend to us on the main thread. This must be a
 // separate class from the history service so that it can hold a reference to
 // the history service (otherwise we would have to manually AddRef and
 // Release when the Backend has a reference to us).
 class HistoryService::BackendDelegate : public HistoryBackend::Delegate {
  public:
   BackendDelegate(
       const base::WeakPtr<HistoryService>& history_service,
@@ skipping 151 matching lines @@
   thread_->message_loop()->ReleaseSoon(FROM_HERE, raw_ptr);
 }
 
 void HistoryService::Cleanup() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!thread_) {
     // We've already cleaned up.
     return;
   }
 
+  // Cancel all pending WebHistoryService requests.
+  STLDeleteElements(&pending_web_history_service_requests_);
+
   weak_ptr_factory_.InvalidateWeakPtrs();
 
   // Unload the backend.
   UnloadBackend();
 
   // Delete the thread, which joins with the background thread. We defensively
   // NULL the pointer before deleting it in case somebody tries to use it
   // during shutdown, but this shouldn't happen.
   base::Thread* thread = thread_;
   thread_ = NULL;
@@ skipping 686 matching lines @@
 
 void HistoryService::ScheduleTask(SchedulePriority priority,
                                   const base::Closure& task) {
   DCHECK(thread_checker_.CalledOnValidThread());
   CHECK(thread_);
   CHECK(thread_->message_loop());
   // TODO(brettw): Do prioritization.
   thread_->message_loop()->PostTask(FROM_HERE, task);
 }
 
+// Callback from WebHistoryService::ExpireWebHistory().
+void HistoryService::WebHistoryServiceRequestComplete(
+    history::WebHistoryService::Request* request,
+    bool success) {
+  // Ignore the result and delete the request.
+  pending_web_history_service_requests_.erase(request);
+  delete request;
+}
+
 // static
 bool HistoryService::CanAddURL(const GURL& url) {
   if (!url.is_valid())
     return false;
 
   // TODO: We should allow kChromeUIScheme URLs if they have been explicitly
   // typed.  Right now, however, these are marked as typed even when triggered
   // by a shortcut or menu action.
   if (url.SchemeIs(content::kJavaScriptScheme) ||
       url.SchemeIs(chrome::kChromeDevToolsScheme) ||
@@ skipping 145 matching lines @@
   if (web_history) {
     // TODO(dubroy): This API does not yet support deletion of specific URLs.
     DCHECK(restrict_urls.empty());
 
     delete_directive_handler_.CreateDeleteDirectives(
         std::set<int64>(), begin_time, end_time);
 
     // Attempt online deletion from the history server, but ignore the result.
     // Deletion directives ensure that the results will eventually be deleted.
     // Pass ownership of the request to the callback.
     scoped_ptr<history::WebHistoryService::Request> request =

[sky, 2014/02/19 17:33:21]

This seems like a hack to me. Expiring history doesn't seem like it should be
something that is cancelable. That is, you can't really cancel it part way.
Seems like WebHistoryService::ExpireHistoryBetween should not have a return
value.

         web_history->ExpireHistoryBetween(
             restrict_urls, begin_time, end_time,
-            base::Bind(&ExpireWebHistoryComplete));
+            base::Bind(&HistoryService::WebHistoryServiceRequestComplete,
+                       weak_ptr_factory_.GetWeakPtr()));
 
-    // The request will be freed when the callback is called.
-    CHECK(request.release());
+    // The request will be freed when the callback is called, or on
+    // HistoryService shutdown.
+    pending_web_history_service_requests_.insert(request.release());
   }
   ExpireHistoryBetween(restrict_urls, begin_time, end_time, callback, tracker);
 }
 
 void HistoryService::BroadcastNotificationsHelper(
     int type,
     history::HistoryDetails* details) {
   DCHECK(thread_checker_.CalledOnValidThread());
   // TODO(evanm): this is currently necessitated by generate_profile, which
   // runs without a browser process. generate_profile should really create
@@ skipping 71 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   visit_database_observers_.RemoveObserver(observer);
 }
 
 void HistoryService::NotifyVisitDBObserversOnAddVisit(
     const history::BriefVisitInfo& info) {
   DCHECK(thread_checker_.CalledOnValidThread());
   FOR_EACH_OBSERVER(history::VisitDatabaseObserver, visit_database_observers_,
                     OnAddVisit(info));
 }