[signin] ScopedSyncDataDeletion supports outliving the AccountReconcilor

Bug: 892372
Change-Id: I139ead7a8a8c9ded7150c31e0779bb8e03728ea7
Reviewed-on: https://chromium-review.googlesource.com/c/1264496Reviewed-by: Thomas Tangl <tangltom@chromium.org>
Commit-Queue: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597126}

components/signin/core/browser/account_reconcilor.cc

@@ -125,12 +125,15 @@ AccountReconcilor::Lock::~Lock() {
 
 AccountReconcilor::ScopedSyncedDataDeletion::ScopedSyncedDataDeletion(
     AccountReconcilor* reconcilor)
-    : reconcilor_(reconcilor) {
+    : reconcilor_(reconcilor->weak_factory_.GetWeakPtr()) {
   DCHECK(reconcilor_);
   ++reconcilor_->synced_data_deletion_in_progress_count_;
 }
 
 AccountReconcilor::ScopedSyncedDataDeletion::~ScopedSyncedDataDeletion() {
+  if (!reconcilor_)
+    return;  // The reconcilor was destroyed.
+
   DCHECK_GT(reconcilor_->synced_data_deletion_in_progress_count_, 0);
   --reconcilor_->synced_data_deletion_in_progress_count_;
 }
@@ -156,7 +159,8 @@ AccountReconcilor::AccountReconcilor(
       chrome_accounts_changed_(false),
       account_reconcilor_lock_count_(0),
       reconcile_on_unblock_(false),
-      timer_(new base::OneShotTimer) {
+      timer_(new base::OneShotTimer),
+      weak_factory_(this) {
   VLOG(1) << "AccountReconcilor::AccountReconcilor";
   DCHECK(delegate_);
   delegate_->set_reconcilor(this);

components/signin/core/browser/account_reconcilor.h

@@ -12,6 +12,7 @@
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
@@ -66,7 +67,7 @@ class AccountReconcilor : public KeyedService,
    private:
     friend class AccountReconcilor;
     explicit ScopedSyncedDataDeletion(AccountReconcilor* reconcilor);
-    AccountReconcilor* reconcilor_;
+    base::WeakPtr<AccountReconcilor> reconcilor_;
     DISALLOW_COPY_AND_ASSIGN(ScopedSyncedDataDeletion);
   };
 
@@ -357,6 +358,8 @@ class AccountReconcilor : public KeyedService,
   // not invalidate the primary token while this is happening.
   int synced_data_deletion_in_progress_count_ = 0;
 
+  base::WeakPtrFactory<AccountReconcilor> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(AccountReconcilor);
 };
 

components/signin/core/browser/account_reconcilor_unittest.cc

@@ -271,6 +271,8 @@ class AccountReconcilorTest : public ::testing::Test {
 
   PrefService* pref_service() { return &pref_service_; }
 
+  void DeleteReconcilor() { mock_reconcilor_.reset(); }
+
  private:
   base::MessageLoop loop;
   signin::AccountConsistencyMethod account_consistency_;
@@ -2453,3 +2455,11 @@ TEST_F(AccountReconcilorTest, DelegateTimeoutIsNotCalledIfTimeoutIsNotReached) {
   EXPECT_EQ(1, spy_delegate->num_reconcile_finished_calls_);
   EXPECT_FALSE(reconcilor->is_reconcile_started_);
 }
+
+TEST_F(AccountReconcilorTest, ScopedSyncedDataDeletionDestructionOrder) {
+  AccountReconcilor* reconcilor = GetMockReconcilor();
+  std::unique_ptr<AccountReconcilor::ScopedSyncedDataDeletion> data_deletion =
+      reconcilor->GetScopedSyncDataDeletion();
+  DeleteReconcilor();
+  // data_deletion is destroyed after the reconcilor, this should not crash.
+}