Speculative fix for a bug in ProfileDestroyer destruction

This CL tentatively reverts following 6 CL's as a speculative fix
for an ANR issue around ProfileDestroyer:

 https://crrev.com/c/2171463
  "Clean up ProfileDestroyer."
 https://crrev.com/c/2134019
  "Add tracing to make it easier to debug shutdown behavior."
 https://crrev.com/c/2113391
  "Android: Handle multiple ProfileDestroyers correctly"
 https://crrev.com/c/2032385
  "Fix a crash in ProfileDestroyer due to delayed RPH destruction"
 https://crrev.com/c/2022791
  Revert "Only CHECK for RPHs that outlive an OTR profile in release"
 https://crrev.com/c/1971031
  "Android: Fix a bug when closing all incognito tabs for preview tab"

The first CL(1071031) landed to fix Issue 1029677, and subsequent CL's
handled the remaining/new issues reported after that.

The ANR bug (1095078/1095548) is suspected to have to do with
ProfileDestroyer destruction of incognito profile. Not having found
good clues, this is trying to see if the series of these changes
in the class caused it. With this, the state of ProfileDestroyer
is restored back to M80 which had no issues.

This is NOT a permanent solution for the problem.  Will reland
the above CL's if they are proven innocent, or find a right fix
if they indeed are the culprit.

TBR=msarda@chromium.org

Bug: 1029677, 1040839, 1095078, 1095548
Change-Id: I761809a90cd4f3a866ae7754da4dab34640dcb3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2308616Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791182}

chrome/browser/profiles/profile_destroyer.cc

@@ -27,65 +27,47 @@ const int64_t kTimerDelaySeconds = 1;
 
 }  // namespace
 
-ProfileDestroyer::DestroyerSet* ProfileDestroyer::pending_destroyers_ = nullptr;
+ProfileDestroyer::DestroyerSet* ProfileDestroyer::pending_destroyers_ = NULL;
 
 // static
 void ProfileDestroyer::DestroyProfileWhenAppropriate(Profile* const profile) {
-  TRACE_EVENT2("shutdown", "ProfileDestroyer::DestroyProfileWhenAppropriate",
-               "profile", profile, "is_off_the_record",
-               profile->IsOffTheRecord());
+  TRACE_EVENT0("shutdown", "ProfileDestroyer::DestroyProfileWhenAppropriate");
 
   DCHECK(profile);
   profile->MaybeSendDestroyedNotification();
 
-  // TODO(https://crbug.com/1033903): If regular profile has OTRs and they have
-  // hosts, create a |ProfileDestroyer| instead.
-  if (!profile->IsOffTheRecord()) {
-    DestroyRegularProfileNow(profile);
-    return;
-  }
+  HostSet profile_hosts = GetHostsForProfile(profile);
+  const bool profile_is_off_the_record = profile->IsOffTheRecord();
+  base::debug::Alias(&profile_is_off_the_record);
+  const bool profile_has_off_the_record =
+      !profile_is_off_the_record && profile->HasOffTheRecordProfile();
+  base::debug::Alias(&profile_has_off_the_record);
+
   // Off-the-record profiles have DestroyProfileWhenAppropriate() called before
   // their RenderProcessHosts are destroyed, to ensure private data is erased
   // promptly. In this case, defer deletion until all the hosts are gone.
-  HostSet profile_hosts = GetHostsForProfile(profile);
-  if (profile_hosts.empty()) {
-    DestroyOffTheRecordProfileNow(profile);
+  if (profile_is_off_the_record) {
+    DCHECK(!profile_has_off_the_record);
+    if (profile_hosts.size()) {
+      // The instance will destroy itself once all (non-spare) render process
+      // hosts referring to it are properly terminated.
+      new ProfileDestroyer(profile, &profile_hosts);
+    } else {
+      profile->GetOriginalProfile()->DestroyOffTheRecordProfile(profile);
+    }
     return;
   }
 
-  // The instance will destroy itself once all (non-spare) render process
-  // hosts referring to it are properly terminated.
-  new ProfileDestroyer(profile, &profile_hosts);
-}
-
-// static
-void ProfileDestroyer::DestroyOffTheRecordProfileNow(Profile* const profile) {
-  DCHECK(profile);
-  DCHECK(profile->IsOffTheRecord());
-  TRACE_EVENT1("shutdown", "ProfileDestroyer::DestroyOffTheRecordProfileNow",
-               "profile", profile);
-
-  RemovePendingDestroyers(profile);
-  profile->GetOriginalProfile()->DestroyOffTheRecordProfile(profile);
-}
-
-// static
-void ProfileDestroyer::DestroyRegularProfileNow(Profile* const profile) {
-  DCHECK(profile);
-  DCHECK(profile->IsRegularProfile());
-  TRACE_EVENT1("shutdown", "ProfileDestroyer::DestroyRegularProfileNow",
-               "profile", profile);
-
 #if DCHECK_IS_ON()
   // Save the raw pointers of profile and off-the-record profile for DCHECKing
   // on later.
-  HostSet profile_hosts = GetHostsForProfile(profile);
   void* profile_ptr = profile;
-  void* otr_profile_ptr = profile->HasOffTheRecordProfile()
-                              ? profile->GetOffTheRecordProfile()
-                              : nullptr;
+  void* otr_profile_ptr =
+      profile_has_off_the_record ? profile->GetOffTheRecordProfile() : nullptr;
 #endif  // DCHECK_IS_ON()
 
+  // TODO(https://crbug.com/1033903): If profile has OTRs and they have hosts,
+  // create a |ProfileDestroyer| instead.
   delete profile;
 
 #if DCHECK_IS_ON()
@@ -113,26 +95,31 @@ void ProfileDestroyer::DestroyRegularProfileNow(Profile* const profile) {
 #endif  // DCHECK_IS_ON()
 }
 
-void ProfileDestroyer::RemovePendingDestroyers(Profile* const profile) {
+// This can be called to cancel any pending destruction and destroy the profile
+// now, e.g., if the parent profile is being destroyed while the incognito one
+// still pending...
+void ProfileDestroyer::DestroyOffTheRecordProfileNow(Profile* const profile) {
+  DCHECK(profile);
   DCHECK(profile->IsOffTheRecord());
   if (pending_destroyers_) {
-    for (auto* i : *pending_destroyers_) {
-      if (i->profile_ == profile) {
+    for (auto i = pending_destroyers_->begin(); i != pending_destroyers_->end();
+         ++i) {
+      if ((*i)->profile_ == profile) {
         // We want to signal this in debug builds so that we don't lose sight of
         // these potential leaks, but we handle it in release so that we don't
         // crash or corrupt profile data on disk.
-        LOG(WARNING) << "A render process host wasn't destroyed early enough.";
-        i->profile_ = nullptr;
+        NOTREACHED() << "A render process host wasn't destroyed early enough.";
+        (*i)->profile_ = NULL;
+        break;
       }
     }
   }
+  DCHECK(profile->GetOriginalProfile());
+  profile->GetOriginalProfile()->DestroyOffTheRecordProfile(profile);
 }
 
 ProfileDestroyer::ProfileDestroyer(Profile* const profile, HostSet* hosts)
     : num_hosts_(0), profile_(profile) {
-  TRACE_EVENT2("shutdown", "ProfileDestroyer::ProfileDestroyer", "profile",
-               profile, "host_count", hosts->size());
-  DCHECK(profile_->IsOffTheRecord());
   if (pending_destroyers_ == NULL)
     pending_destroyers_ = new DestroyerSet;
   pending_destroyers_->insert(this);
@@ -146,59 +133,66 @@ ProfileDestroyer::ProfileDestroyer(Profile* const profile, HostSet* hosts)
   // for longer than kTimerDelaySeconds.
   if (num_hosts_) {
     timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(kTimerDelaySeconds),
-                 base::BindOnce(
-                     [](base::WeakPtr<ProfileDestroyer> ptr) {
-                       if (ptr)
-                         delete ptr.get();
-                     },
-                     weak_ptr_factory_.GetWeakPtr()));
+                 base::Bind(&ProfileDestroyer::DestroyProfile,
+                            weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
 ProfileDestroyer::~ProfileDestroyer() {
-  TRACE_EVENT1("shutdown", "ProfileDestroyer::~ProfileDestroyer", "profile",
-               profile_);
-
-  if (profile_) {
-    ProfileDestroyer::DestroyOffTheRecordProfileNow(profile_);
-    DCHECK(!profile_);
-  }
-
-  // Once the profile is deleted, all renderer hosts must have been deleted.
-  // Crash here if this is not the case instead of having a host dereference
-  // a deleted profile. http://crbug.com/248625
+  // Check again, in case other render hosts were added while we were
+  // waiting for the previous ones to go away...
+  if (profile_)
+    DestroyProfileWhenAppropriate(profile_);
+
+#ifdef NDEBUG
+  // Don't wait for pending registrations, if any, these hosts are buggy.
+  // Note: this can happen, but if so, it's better to crash here than wait
+  // for the host to dereference a deleted Profile. http://crbug.com/248625
   CHECK_EQ(0U, num_hosts_) << "Some render process hosts were not "
                            << "destroyed early enough!";
-
-  DCHECK(pending_destroyers_ != nullptr);
+#endif  // NDEBUG
+  DCHECK(pending_destroyers_ != NULL);
   auto iter = pending_destroyers_->find(this);
   DCHECK(iter != pending_destroyers_->end());
   pending_destroyers_->erase(iter);
   if (pending_destroyers_->empty()) {
     delete pending_destroyers_;
-    pending_destroyers_ = nullptr;
+    pending_destroyers_ = NULL;
   }
 }
 
 void ProfileDestroyer::RenderProcessHostDestroyed(
     content::RenderProcessHost* host) {
-  TRACE_EVENT2("shutdown", "ProfileDestroyer::RenderProcessHostDestroyed",
-               "profile", profile_, "render_process_host", host);
   DCHECK(num_hosts_ > 0);
   --num_hosts_;
   if (num_hosts_ == 0) {
     // Delay the destruction one step further in case other observers need to
     // look at the profile attached to the host.
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(
-                       [](base::WeakPtr<ProfileDestroyer> ptr) {
-                         if (ptr)
-                           delete ptr.get();
-                       },
-                       weak_ptr_factory_.GetWeakPtr()));
+        FROM_HERE, base::BindOnce(&ProfileDestroyer::DestroyProfile,
+                                  weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
+void ProfileDestroyer::DestroyProfile() {
+  // We might have been cancelled externally before the timer expired.
+  if (!profile_) {
+    delete this;
+    return;
+  }
+
+  DCHECK(profile_->IsOffTheRecord());
+  DCHECK(profile_->GetOriginalProfile());
+  profile_->GetOriginalProfile()->DestroyOffTheRecordProfile(profile_);
+
+  profile_ = nullptr;
+
+  // And stop the timer so we can be released early too.
+  timer_.Stop();
+
+  delete this;
+}
+
 // static
 ProfileDestroyer::HostSet ProfileDestroyer::GetHostsForProfile(
     void* const profile_ptr) {
@@ -216,8 +210,6 @@ ProfileDestroyer::HostSet ProfileDestroyer::GetHostsForProfile(
     if (render_process_host->HostHasNotBeenUsed())
       continue;
 
-    TRACE_EVENT2("shutdown", "ProfileDestroyer::GetHostsForProfile", "profile",
-                 profile_ptr, "render_process_host", render_process_host);
     hosts.insert(render_process_host);
   }
   return hosts;

chrome/browser/profiles/profile_destroyer.h

@@ -15,7 +15,6 @@
 #include "content/public/browser/render_process_host_observer.h"
 
 class Profile;
-class ProfileImpl;
 
 namespace content {
 class RenderProcessHost;
@@ -25,14 +24,10 @@ class RenderProcessHost;
 // sure it gets done asynchronously after all render process hosts are gone.
 class ProfileDestroyer : public content::RenderProcessHostObserver {
  public:
-  // Destroys the given profile either instantly, or after a short delay waiting
-  // for dependent renderer process hosts to destroy.
-  // Ownership of the profile is passed to profile destroyer and the profile
-  // should not be used after this call.
   static void DestroyProfileWhenAppropriate(Profile* const profile);
+  static void DestroyOffTheRecordProfileNow(Profile* const profile);
 
  private:
-  friend class ProfileImpl;
   typedef std::set<content::RenderProcessHost*> HostSet;
   typedef std::set<ProfileDestroyer*> DestroyerSet;
 
@@ -44,21 +39,14 @@ class ProfileDestroyer : public content::RenderProcessHostObserver {
   // content::RenderProcessHostObserver override.
   void RenderProcessHostDestroyed(content::RenderProcessHost* host) override;
 
+  // Called by the timer to cancel the pending destruction and do it now.
+  void DestroyProfile();
+
   // Fetch the list of render process hosts that still point to |profile_ptr|.
   // |profile_ptr| is a void* because the Profile object may be freed. Only
   // pointer comparison is allowed, it will never be dereferenced as a Profile.
   static HostSet GetHostsForProfile(void* const profile_ptr);
 
-  // Destroys a regular profile immediately.
-  static void DestroyRegularProfileNow(Profile* const profile);
-
-  // Destroys an OffTheRecord profile immediately and removes it from all
-  // pending destroyers.
-  static void DestroyOffTheRecordProfileNow(Profile* const profile);
-
-  // Removes the profile from pending destroyers.
-  static void RemovePendingDestroyers(Profile* const profile);
-
   // We need access to all pending destroyers so we can cancel them.
   static DestroyerSet* pending_destroyers_;