Make ProfileSigninConfirmationHelper delete itself instead of being refcounted.

chrome/browser/ui/sync/profile_signin_confirmation_helper.cc

Index: chrome/browser/ui/sync/profile_signin_confirmation_helper.cc
diff --git a/chrome/browser/ui/sync/profile_signin_confirmation_helper.cc b/chrome/browser/ui/sync/profile_signin_confirmation_helper.cc
index 4e9ce18ea9897fe4f313df7529d6e8ee59985325..e62d7ed8e992cd9e7dab79438c04dc1db0c0d2de 100644
--- a/chrome/browser/ui/sync/profile_signin_confirmation_helper.cc
+++ b/chrome/browser/ui/sync/profile_signin_confirmation_helper.cc
@@ -21,6 +21,7 @@
 #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"
@@ -56,6 +57,7 @@ class HasTypedURLsTask : public history::HistoryDBTask {
 
  private:
   virtual ~HasTypedURLsTask() {}
+
   bool has_typed_urls_;
   base::Callback<void(bool)> cb_;
 };
@@ -69,19 +71,16 @@ bool HasBookmarks(Profile* profile) {
 }
 
 // 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,
@@ -116,6 +115,7 @@ ProfileSigninConfirmationHelper::ProfileSigninConfirmationHelper(
 }
 
 ProfileSigninConfirmationHelper::~ProfileSigninConfirmationHelper() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 }
 
 void ProfileSigninConfirmationHelper::OnHistoryQueryResults(
@@ -144,9 +144,10 @@ void ProfileSigninConfirmationHelper::CheckHasHistory(int max_entries) {
       base::string16(),
       opts,
       base::Bind(&ProfileSigninConfirmationHelper::OnHistoryQueryResults,
-                 this,
+                 base::Unretained(this),
                  max_entries),
       &task_tracker_);
+  pending_requests_++;
 }
 
 void ProfileSigninConfirmationHelper::CheckHasTypedURLs() {
@@ -158,21 +159,21 @@ void ProfileSigninConfirmationHelper::CheckHasTypedURLs() {
   }
   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.

   }
 }
 
@@ -220,6 +221,8 @@ bool HasSyncedExtensions(Profile* profile) {
 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)) {
@@ -227,10 +230,8 @@ void CheckShouldPromptForNewProfile(
     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();
 }