Make ProfileSigninConfirmationHelper delete itself instead of being refcounted.

chrome/browser/ui/sync/profile_signin_confirmation_helper.cc

 // Copyright 2013 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/sync/profile_signin_confirmation_helper.h"
 
 #include "base/bind.h"
 #include "base/memory/ref_counted.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string16.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/common/cancelable_request.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/history/history_backend.h"
 #include "chrome/browser/history/history_db_task.h"
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/sync_helper.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "content/public/browser/browser_thread.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_set.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/native_theme/native_theme.h"
 
 namespace {
 
 const int kHistoryEntriesBeforeNewProfilePrompt = 10;
 
@@ skipping 15 matching lines @@
     }
     return true;
   }
 
   virtual void DoneRunOnMainThread() OVERRIDE {
     cb_.Run(has_typed_urls_);
   }
 
  private:
   virtual ~HasTypedURLsTask() {}
+
   bool has_typed_urls_;
   base::Callback<void(bool)> cb_;
 };
 
 bool HasBookmarks(Profile* profile) {
   BookmarkModel* bookmarks = BookmarkModelFactory::GetForProfile(profile);
   bool has_bookmarks = bookmarks && bookmarks->HasBookmarks();
   if (has_bookmarks)
     DVLOG(1) << "ProfileSigninConfirmationHelper: profile contains bookmarks";
   return has_bookmarks;
 }
 
 // Helper functions for Chrome profile signin.
-class ProfileSigninConfirmationHelper
-    : public base::RefCounted<ProfileSigninConfirmationHelper> {
+class ProfileSigninConfirmationHelper {
  public:
   ProfileSigninConfirmationHelper(
       Profile* profile,
       const base::Callback<void(bool)>& return_result);
   void CheckHasHistory(int max_entries);
   void CheckHasTypedURLs();
-  void set_pending_requests(int requests);
 
  private:
-  friend class base::RefCounted<ProfileSigninConfirmationHelper>;
-
+  // Deletes itself.
   ~ProfileSigninConfirmationHelper();
 
   void OnHistoryQueryResults(size_t max_entries,
                              history::QueryResults* results);
   void ReturnResult(bool result);
 
   // Weak pointer to the profile being signed-in.
   Profile* profile_;
 
   // Used for async tasks.
@@ skipping 14 matching lines @@
 ProfileSigninConfirmationHelper::ProfileSigninConfirmationHelper(
     Profile* profile,
     const base::Callback<void(bool)>& return_result)
     : profile_(profile),
       pending_requests_(0),
       result_returned_(false),
       return_result_(return_result) {
 }
 
 ProfileSigninConfirmationHelper::~ProfileSigninConfirmationHelper() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 }
 
 void ProfileSigninConfirmationHelper::OnHistoryQueryResults(
     size_t max_entries,
     history::QueryResults* results) {
   history::QueryResults owned_results;
   results->Swap(&owned_results);
   bool too_much_history = owned_results.size() >= max_entries;
   if (too_much_history) {
     DVLOG(1) << "ProfileSigninConfirmationHelper: profile contains "
              << owned_results.size() << " history entries";
   }
   ReturnResult(too_much_history);
 }
 
 void ProfileSigninConfirmationHelper::CheckHasHistory(int max_entries) {
   HistoryService* service =
       HistoryServiceFactory::GetForProfileWithoutCreating(profile_);
   if (!service) {
     ReturnResult(false);
     return;
   }
   history::QueryOptions opts;
   opts.max_count = max_entries;
   service->QueryHistory(
       base::string16(),
       opts,
       base::Bind(&ProfileSigninConfirmationHelper::OnHistoryQueryResults,
-                 this,
+                 base::Unretained(this),
                  max_entries),
       &task_tracker_);
+  pending_requests_++;
 }
 
 void ProfileSigninConfirmationHelper::CheckHasTypedURLs() {
   HistoryService* service =
       HistoryServiceFactory::GetForProfileWithoutCreating(profile_);
   if (!service) {
     ReturnResult(false);
     return;
   }
   service->ScheduleDBTask(
       new HasTypedURLsTask(
-          base::Bind(&ProfileSigninConfirmationHelper::ReturnResult, this)),
+          base::Bind(&ProfileSigninConfirmationHelper::ReturnResult,
+                     base::Unretained(this))),
       &task_tracker_);
-}
-
-void ProfileSigninConfirmationHelper::set_pending_requests(int requests) {
-  pending_requests_ = requests;
+  pending_requests_++;
 }
 
 void ProfileSigninConfirmationHelper::ReturnResult(bool result) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   // Pass |true| into the callback as soon as one of the tasks passes a
   // result of |true|, otherwise pass the last returned result.
   if (!result_returned_ && (--pending_requests_ == 0 || result)) {
     result_returned_ = true;
     task_tracker_.TryCancelAll();
     return_result_.Run(result);
+    delete this;

[Andrew T Wilson (Slow), 2014/07/14 11:04:45]

Does this mean we leak if our callbacks aren't invoked? Not clear to me that
ScheduleDBTask() always invokes the callback.

[Bernhard Bauer, 2014/07/14 13:08:29]

True, if the history backend shuts down, this won't be called back, but that
might not actually be a bad thing, as this would happen during profile shutdown,
and the callbacks passed to CheckShouldPromptForNewProfile() usually pass in an
unretained profile pointer), and in any case it's a leak during shutdown, so all
the memory is freed anyway.

Fixing this would be relatively easy after
https://codereview.chromium.org/387923002/ and a little bit more involved before
that (as HasTypedURLsTask could be destroyed on any thread), but what I would
actually propose to do is change the contract so that the client will own this
object[*], which would make it moot. Let me know what you want me to do.

[*] Which would also allow us to cancel all DB operations on destruction.

[Andrew T Wilson (Slow), 2014/07/14 14:32:57]

Just comment that it leaks during profile shutdown, I guess. Have you run this
on the valgrind bots to make sure it's not triggering leaks in our existing
tests? If so, then I'm fine with landing this.

[dconnelly, 2014/07/15 11:20:01]

I agree with just adding the comment.

   }
 }
 
 }  // namespace
 
 namespace ui {
 
 SkColor GetSigninConfirmationPromptBarColor(SkAlpha alpha) {
   static const SkColor kBackgroundColor =
       ui::NativeTheme::instance()->GetSystemColor(
@@ skipping 27 matching lines @@
         return true;
       }
     }
   }
   return false;
 }
 
 void CheckShouldPromptForNewProfile(
     Profile* profile,
     const base::Callback<void(bool)>& return_result) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
   if (HasBeenShutdown(profile) ||
       HasBookmarks(profile) ||
       HasSyncedExtensions(profile)) {
     return_result.Run(true);
     return;
   }
   // Fire asynchronous queries for profile data.
-  scoped_refptr<ProfileSigninConfirmationHelper> helper =
+  ProfileSigninConfirmationHelper* helper =
       new ProfileSigninConfirmationHelper(profile, return_result);
-  const int requests = 2;
-  helper->set_pending_requests(requests);
   helper->CheckHasHistory(kHistoryEntriesBeforeNewProfilePrompt);
   helper->CheckHasTypedURLs();
 }
 
 }  // namespace ui