Now doesn't wait to destroy non-incognito profiles, and allow immediate destruction of incognito profiles from their parent profile. Also, doesn't wait infinitely for render process hosts to go away, uses a timer to complete destruction. And finally, adds DCHECKs to identify when render process hosts are dead soon enough.

BUG=130772
TEST=Make sure Chrome exits without a crash and that Profile info properly got saved. Also make sure info doesn't leak from one incognito session to another.

Review URL: https://chromiumcodereview.appspot.com/10645005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@144251 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 123 insertions, 11 deletions

diff --git a/chrome/browser/profiles/profile_destroyer.cc b/chrome/browser/profiles/profile_destroyer.cc
index 2437405..fcf5f10 100644
--- a/chrome/browser/profiles/profile_destroyer.cc
+++ b/chrome/browser/profiles/profile_destroyer.cc
@@ -12,8 +12,18 @@
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 
+
+namespace {
+
+const int64 kTimerDelaySeconds = 1;
+
+}  // namespace
+
+std::vector<ProfileDestroyer*>* ProfileDestroyer::pending_destroyers_ = NULL;
+
 // static
 void ProfileDestroyer::DestroyProfileWhenAppropriate(Profile* const profile) {
+  DCHECK(profile);
   std::vector<content::RenderProcessHost*> hosts;
   // Testing profiles can simply be deleted directly. Some tests don't setup
   // RenderProcessHost correctly and don't necessary run on the UI thread
@@ -23,45 +33,125 @@ void ProfileDestroyer::DestroyProfileWhenAppropriate(Profile* const profile) {
     if (!profile->IsOffTheRecord() && profile->HasOffTheRecordProfile())
       GetHostsForProfile(profile->GetOffTheRecordProfile(), &hosts);
   }
-  if (hosts.empty()) {
+  // This should never happen for non Off the record profile, this means that
+  // there is a leak in a render process host that MUST BE FIXED!!!
+  DCHECK(hosts.empty() || profile->IsOffTheRecord());
+  // Note that we still test for !profile->IsOffTheRecord here even though we
+  // DCHECK'd above because we want to protect Release builds against this even
+  // we need to identify if there are leaks when we run Debug builds.
+  if (hosts.empty() || !profile->IsOffTheRecord()) {
     if (profile->IsOffTheRecord())
       profile->GetOriginalProfile()->DestroyOffTheRecordProfile();
     else
       delete profile;
   } else {
     // The instance will destroy itself once all render process hosts referring
-    // to it and/or it's off the record profile are properly terminated.
+    // to it are properly terminated.
     scoped_refptr<ProfileDestroyer> profile_destroyer(
         new ProfileDestroyer(profile, hosts));
   }
 }
 
+// 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 (size_t i = 0; i < pending_destroyers_->size(); ++i) {
+      if ((*pending_destroyers_)[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.
+        NOTREACHED() << "A render process host wasn't destroyed early enough.";
+        (*pending_destroyers_)[i]->profile_ = NULL;
+        break;
+      }
+    }
+  }
+  DCHECK(profile->GetOriginalProfile());
+  profile->GetOriginalProfile()->DestroyOffTheRecordProfile();
+}
+
 ProfileDestroyer::ProfileDestroyer(
     Profile* const profile,
-    const std::vector<content::RenderProcessHost*>& hosts) : profile_(profile) {
+    const std::vector<content::RenderProcessHost*>& hosts)
+    : timer_(false, false),
+      num_hosts_(0),
+      profile_(profile) {
+  if (pending_destroyers_ == NULL)
+    pending_destroyers_ = new std::vector<ProfileDestroyer*>;
+  pending_destroyers_->push_back(this);
   for (size_t i = 0; i < hosts.size(); ++i) {
     registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
         content::Source<content::RenderProcessHost>(hosts[i]));
     // For each of the notifications, we bump up our reference count.
     // It will go back to 0 and free us when all hosts are terminated.
-    AddRef();
+    ++num_hosts_;
+  }
+  // If we are going to wait for render process hosts, we don't want to do it
+  // for longer than kTimerDelaySeconds.
+  if (num_hosts_) {
+    timer_.Start(FROM_HERE,
+                 base::TimeDelta::FromSeconds(kTimerDelaySeconds),
+                 base::Bind(&ProfileDestroyer::DestroyProfile, this));
   }
 }
 
 ProfileDestroyer::~ProfileDestroyer() {
   // Check again, in case other render hosts were added while we were
   // waiting for the previous ones to go away...
-  DestroyProfileWhenAppropriate(profile_);
+  if (profile_)
+    DestroyProfileWhenAppropriate(profile_);
+
+  // We shouldn't be deleted with pending notifications.
+  DCHECK(registrar_.IsEmpty());
+
+  DCHECK(pending_destroyers_ != NULL);
+  std::vector<ProfileDestroyer*>::iterator iter = std::find(
+      pending_destroyers_->begin(), pending_destroyers_->end(), this);
+  DCHECK(iter != pending_destroyers_->end());
+  pending_destroyers_->erase(iter);
+  DCHECK(pending_destroyers_->end() == std::find(pending_destroyers_->begin(),
+                                                 pending_destroyers_->end(),
+                                                 this));
+  if (pending_destroyers_->empty()) {
+    delete pending_destroyers_;
+    pending_destroyers_ = NULL;
+  }
 }
 
 void ProfileDestroyer::Observe(int type,
                                const content::NotificationSource& source,
                                const content::NotificationDetails& details) {
   DCHECK(type == content::NOTIFICATION_RENDERER_PROCESS_TERMINATED);
-  // Delay the destruction one step further in case other observers of this
-  // notification need to look at the profile attached to the host.
-  MessageLoop::current()->PostTask(
-      FROM_HERE, base::Bind(&ProfileDestroyer::Release, this));
+  DCHECK(num_hosts_ > 0);
+  --num_hosts_;
+  if (num_hosts_ == 0) {
+    // Delay the destruction one step further in case other observers of this
+    // notification need to look at the profile attached to the host.
+    MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(&ProfileDestroyer::DestroyProfile, this));
+  }
+}
+
+void ProfileDestroyer::DestroyProfile() {
+  // We might have been cancelled externally before the timer expired.
+  if (profile_ == NULL)
+    return;
+  DCHECK(profile_->IsOffTheRecord());
+  DCHECK(profile_->GetOriginalProfile());
+  profile_->GetOriginalProfile()->DestroyOffTheRecordProfile();
+  profile_ = NULL;
+
+  // Don't wait for pending registrations, if any, these hosts are buggy.
+  DCHECK(registrar_.IsEmpty()) << "Some render process hosts where not "
+                               << "destroyed early enough!";
+  registrar_.RemoveAll();
+
+  // And stop the timer so we can be released early too.
+  timer_.Stop();
 }
 
 // static
diff --git a/chrome/browser/profiles/profile_destroyer.h b/chrome/browser/profiles/profile_destroyer.h
index a238e41..afa170c 100644
--- a/chrome/browser/profiles/profile_destroyer.h
+++ b/chrome/browser/profiles/profile_destroyer.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/memory/ref_counted.h"
+#include "base/timer.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
@@ -24,6 +25,7 @@ class ProfileDestroyer
       public base::RefCounted<ProfileDestroyer> {
  public:
   static void DestroyProfileWhenAppropriate(Profile* const profile);
+  static void DestroyOffTheRecordProfileNow(Profile* const profile);
 
  private:
   friend class base::RefCounted<ProfileDestroyer>;
@@ -37,6 +39,8 @@ class ProfileDestroyer
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
+  // Called by the timer to cancel the pending detruction and do it now.
+  void DestroyProfile();
 
   // Fetch the list of render process hosts that still refer to the profile.
   // Return true if we found at least one, false otherwise.
@@ -44,8 +48,19 @@ class ProfileDestroyer
       Profile* const profile,
       std::vector<content::RenderProcessHost*> *hosts);
 
+  // We need access to all pending destroyers so we can cancel them.
+  static std::vector<ProfileDestroyer*>* pending_destroyers_;
+
+  // We register for render process host termination.
   content::NotificationRegistrar registrar_;
-  Profile* const profile_;
+
+  // We don't want to wait forever, so we have a cancellation timer.
+  base::Timer timer_;
+
+  // Used to count down the number of render process host left.
+  uint32 num_hosts_;
+
+  Profile* profile_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileDestroyer);
 };
diff --git a/chrome/browser/profiles/profile_impl.cc b/chrome/browser/profiles/profile_impl.cc
index 47204f5..1a5d202 100644
--- a/chrome/browser/profiles/profile_impl.cc
+++ b/chrome/browser/profiles/profile_impl.cc
@@ -59,6 +59,7 @@
 #include "chrome/browser/profiles/chrome_version_service.h"
 #include "chrome/browser/profiles/gaia_info_update_service.h"
 #include "chrome/browser/profiles/profile_dependency_manager.h"
+#include "chrome/browser/profiles/profile_destroyer.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/search_engines/template_url_fetcher.h"
@@ -515,7 +516,13 @@ ProfileImpl::~ProfileImpl() {
   }
 
   // Destroy OTR profile and its profile services first.
-  DestroyOffTheRecordProfile();
+  if (off_the_record_profile_.get()) {
+    ProfileDestroyer::DestroyOffTheRecordProfileNow(
+        off_the_record_profile_.get());
+  } else {
+    ExtensionPrefValueMapFactory::GetForProfile(this)->
+        ClearAllIncognitoSessionOnlyPreferences();
+  }
 
   ProfileDependencyManager::GetInstance()->DestroyProfileServices(this);