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

chrome/browser/ui/webui/history_ui.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 "chrome/browser/ui/webui/history_ui.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 368 matching lines @@
 
   return result.Pass();
 }
 
 bool BrowsingHistoryHandler::HistoryEntry::SortByTimeDescending(
     const BrowsingHistoryHandler::HistoryEntry& entry1,
     const BrowsingHistoryHandler::HistoryEntry& entry2) {
   return entry1.time > entry2.time;
 }
 
-BrowsingHistoryHandler::BrowsingHistoryHandler() {}
+BrowsingHistoryHandler::BrowsingHistoryHandler()
+    : pending_delete_requests_(0),
+      weak_factory_(this) {
+}
 
 BrowsingHistoryHandler::~BrowsingHistoryHandler() {
   history_request_consumer_.CancelAllRequests();
   web_history_request_.reset();
 }
 
 void BrowsingHistoryHandler::RegisterMessages() {
   // Create our favicon data source.
   Profile* profile = Profile::FromWebUI(web_ui());
   content::URLDataSource::Add(
@@ skipping 122 matching lines @@
     NOTREACHED() << "Failed to convert argument 4.";
     return;
   }
 
   options.duplicate_policy = history::QueryOptions::REMOVE_DUPLICATES_PER_DAY;
   QueryHistory(search_text, options);
 }
 
 void BrowsingHistoryHandler::HandleRemoveVisits(const base::ListValue* args) {
   Profile* profile = Profile::FromWebUI(web_ui());
   if (delete_task_tracker_.HasTrackedTasks() ||

[Patrick Dubroy, 2014/02/20 12:56:55]

See comment below -- the deletion should also fail if the web history deletion
is still outstanding.

[davidben, 2014/02/20 18:08:01]

Done. And added a TODO because it doesn't look like history.js quite handles
this case correctly. (If the user tries to delete one set of entries while the
previous delete request is still running.)

       !profile->GetPrefs()->GetBoolean(prefs::kAllowDeletingBrowserHistory)) {
     web_ui()->CallJavascriptFunction("deleteFailed");
     return;
   }
 
   HistoryService* history_service =
       HistoryServiceFactory::GetForProfile(profile, Profile::EXPLICIT_ACCESS);
   history::WebHistoryService* web_history =
       WebHistoryServiceFactory::GetForProfile(profile);
 
@@ skipping 62 matching lines @@
       history_service->ProcessLocalDeleteDirective(delete_directive);
   }
 
   history_service->ExpireHistory(
       expire_list,
       base::Bind(&BrowsingHistoryHandler::RemoveComplete,
                  base::Unretained(this)),
       &delete_task_tracker_);
 
   if (web_history) {
-    web_history_delete_request_ = web_history->ExpireHistory(
+    pending_delete_requests_++;

[Patrick Dubroy, 2014/02/20 12:56:55]

This seems to imply that there can be multiple deletions outstanding, which
shouldn't be the case. I think this should instead be a boolean, and the check
above should not allow a new deletion to begin while the web history deletion is
still outstanding.

[davidben, 2014/02/20 18:08:01]

Done.

+    web_history->ExpireHistory(
         expire_list,
         base::Bind(&BrowsingHistoryHandler::RemoveWebHistoryComplete,
-                   base::Unretained(this)));
+                   weak_factory_.GetWeakPtr()));
   }
 
 #if defined(ENABLE_EXTENSIONS)
   // If the profile has activity logging enabled also clean up any URLs from
   // the extension activity log. The extension activity log contains URLS
   // which websites an extension has activity on so it will indirectly
   // contain websites that a user has visited.
   extensions::ActivityLog* activity_log =
       extensions::ActivityLog::GetInstance(profile);
   for (std::vector<history::ExpireHistoryArgs>::const_iterator it =
@@ skipping 250 matching lines @@
   results_info_value_.SetBoolean("hasSyncedResults", results_value != NULL);
   if (!history_request_consumer_.HasPendingRequests())
     ReturnResultsToFrontEnd();
 }
 
 void BrowsingHistoryHandler::RemoveComplete() {
   urls_to_be_deleted_.clear();
 
   // Notify the page that the deletion request is complete, but only if a web
   // history delete request is not still pending.
-  if (!(web_history_delete_request_.get() &&
-        web_history_delete_request_->is_pending())) {
+  if (pending_delete_requests_ == 0) {
     web_ui()->CallJavascriptFunction("deleteComplete");
   }
 }
 
-void BrowsingHistoryHandler::RemoveWebHistoryComplete(
-    history::WebHistoryService::Request* request, bool success) {
+void BrowsingHistoryHandler::RemoveWebHistoryComplete(bool success) {
+  pending_delete_requests_--;
   // TODO(dubroy): Should we handle failure somehow? Delete directives will
   // ensure that the visits are eventually deleted, so maybe it's not necessary.
   if (!delete_task_tracker_.HasTrackedTasks())
     RemoveComplete();
 }
 
 void BrowsingHistoryHandler::SetQueryTimeInWeeks(
     int offset, history::QueryOptions* options) {
   // LocalMidnight returns the beginning of the current day so get the
   // beginning of the next one.
@@ skipping 99 matching lines @@
   return GURL(std::string(chrome::kChromeUIHistoryURL) + "#q=" +
               net::EscapeQueryParamValue(base::UTF16ToUTF8(text), true));
 }
 
 // static
 base::RefCountedMemory* HistoryUI::GetFaviconResourceBytes(
       ui::ScaleFactor scale_factor) {
   return ResourceBundle::GetSharedInstance().
       LoadDataResourceBytesForScale(IDR_HISTORY_FAVICON, scale_factor);
 }