Reland ProfileDestroyer cleanup CLs

This CL reinstates following CLs reverted by https://crrev.com/c/2308616
as it was clarified that they have nothing to do with the jank issue:

https://crrev.com/c/2022791 Revert "Only CHECK for RenderProcessHosts
                            that outlive an OTR profile in release"
https://crrev.com/c/2134019 Add tracing to make it easier to debug
                            shutdown behavior.
https://crrev.com/c/2171463 Clean up ProfileDestroyer.

Bug: 1095078, 1095548
Change-Id: I1ebc38c20780ceecb2a9adc5128e3d850f65b6ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2371904Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801622}

chrome/browser/profiles/profile_destroyer.cc

@@ -27,47 +27,71 @@ const int64_t kTimerDelaySeconds = 1;
 
 }  // namespace
 
-ProfileDestroyer::DestroyerSet* ProfileDestroyer::pending_destroyers_ = NULL;
+ProfileDestroyer::DestroyerSet* ProfileDestroyer::pending_destroyers_ = nullptr;
 
 // static
 void ProfileDestroyer::DestroyProfileWhenAppropriate(Profile* const profile) {
-  TRACE_EVENT0("shutdown", "ProfileDestroyer::DestroyProfileWhenAppropriate");
+  TRACE_EVENT2("shutdown", "ProfileDestroyer::DestroyProfileWhenAppropriate",
+               "profile", profile, "is_off_the_record",
+               profile->IsOffTheRecord());
 
   DCHECK(profile);
   profile->MaybeSendDestroyedNotification();
 
-  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);
+  // TODO(https://crbug.com/1033903): If regular profile has OTRs and they have
+  // hosts, create a |ProfileDestroyer| instead.
+  if (!profile->IsOffTheRecord()) {
+    DestroyRegularProfileNow(profile);
+    return;
+  }
 
   // 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.
-  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);
-    }
+  HostSet profile_hosts = GetHostsForProfile(profile);
+  if (profile_hosts.empty()) {
+    DestroyOffTheRecordProfileNow(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);
+  if (ResetPendingDestroyers(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.
+    NOTREACHED() << "A render process host wasn't destroyed early enough.";
+  }
+  DCHECK(profile->GetOriginalProfile());
+  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_has_off_the_record ? profile->GetOffTheRecordProfile() : nullptr;
+  void* otr_profile_ptr = profile->HasOffTheRecordProfile()
+                              ? 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()
@@ -95,22 +119,6 @@ void ProfileDestroyer::DestroyProfileWhenAppropriate(Profile* const profile) {
 #endif  // DCHECK_IS_ON()
 }
 
-// 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 (ResetPendingDestroyers(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.
-    NOTREACHED() << "A render process host wasn't destroyed early enough.";
-  }
-  DCHECK(profile->GetOriginalProfile());
-  profile->GetOriginalProfile()->DestroyOffTheRecordProfile(profile);
-}
-
 bool ProfileDestroyer::ResetPendingDestroyers(Profile* const profile) {
   DCHECK(profile);
   bool found = false;
@@ -127,6 +135,8 @@ bool ProfileDestroyer::ResetPendingDestroyers(Profile* const profile) {
 
 ProfileDestroyer::ProfileDestroyer(Profile* const profile, HostSet* hosts)
     : num_hosts_(0), profile_(profile) {
+  TRACE_EVENT2("shutdown", "ProfileDestroyer::ProfileDestroyer", "profile",
+               profile, "host_count", hosts->size());
   if (pending_destroyers_ == NULL)
     pending_destroyers_ = new DestroyerSet;
   pending_destroyers_->insert(this);
@@ -146,18 +156,19 @@ ProfileDestroyer::ProfileDestroyer(Profile* const profile, HostSet* hosts)
 }
 
 ProfileDestroyer::~ProfileDestroyer() {
+  TRACE_EVENT1("shutdown", "ProfileDestroyer::~ProfileDestroyer", "profile",
+               profile_);
+
   // 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!";
-#endif  // NDEBUG
   DCHECK(pending_destroyers_ != NULL);
   auto iter = pending_destroyers_->find(this);
   DCHECK(iter != pending_destroyers_->end());
@@ -170,6 +181,8 @@ ProfileDestroyer::~ProfileDestroyer() {
 
 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) {
@@ -224,6 +237,8 @@ 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,6 +15,7 @@
 #include "content/public/browser/render_process_host_observer.h"
 
 class Profile;
+class ProfileImpl;
 
 namespace content {
 class RenderProcessHost;
@@ -24,15 +25,14 @@ 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);
-
-  // Reset pending destroyers whose target profile matches the given one
-  // to make it stop attempting to destroy it. Returns true if any object
-  // object was found to match and get reset.
-  static bool ResetPendingDestroyers(Profile* const profile);
 
  private:
+  friend class ProfileImpl;
   typedef std::set<content::RenderProcessHost*> HostSet;
   typedef std::set<ProfileDestroyer*> DestroyerSet;
 
@@ -52,6 +52,18 @@ class ProfileDestroyer : public content::RenderProcessHostObserver {
   // 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);
+
+  // Reset pending destroyers whose target profile matches the given one
+  // to make it stop attempting to destroy it. Returns true if any object
+  // object was found to match and get reset.
+  static bool ResetPendingDestroyers(Profile* const profile);
+
   // We need access to all pending destroyers so we can cancel them.
   static DestroyerSet* pending_destroyers_;