Also delay regular profile destruction...

A previous CL delayed the destruction of Off The Record profiles, but there are cases where they are delayed post destruction of their original profile... no good...

So we now delay the destruction of both the off the record profile and it's original profile when either of them is still referenced by a render process host.

BUG=114245
TEST=unittest --gtest_filter=ProfileDestroyerTest.*

Review URL: http://codereview.chromium.org/9420036

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

12 files changed, 188 insertions, 81 deletions

diff --git a/chrome/browser/profiles/profile_destroyer.cc b/chrome/browser/profiles/profile_destroyer.cc
index 3e955c7..a724432 100644
--- a/chrome/browser/profiles/profile_destroyer.cc
+++ b/chrome/browser/profiles/profile_destroyer.cc
@@ -13,16 +13,22 @@
 #include "content/public/browser/render_process_host.h"
 
 // static
-void ProfileDestroyer::DestroyOffTheRecordProfile(Profile* const profile) {
+void ProfileDestroyer::DestroyProfileWhenAppropriate(Profile* const profile) {
   std::vector<content::RenderProcessHost*> hosts;
-  if (GetHostsForProfile(profile, &hosts)) {
+  GetHostsForProfile(profile, &hosts);
+  if (!profile->IsOffTheRecord() && profile->HasOffTheRecordProfile())
+    GetHostsForProfile(profile->GetOffTheRecordProfile(), &hosts);
+
+  if (hosts.empty()) {
+    if (profile->IsOffTheRecord())
+      profile->GetOriginalProfile()->DestroyOffTheRecordProfile();
+    else
+      delete profile;
+  } else {
     // The instance will destroy itself once all render process hosts referring
-    // to it are properly terminated.
+    // to it and/or it's off the record profile are properly terminated.
     scoped_refptr<ProfileDestroyer> profile_destroyer(
         new ProfileDestroyer(profile, hosts));
-  } else {
-    // Safe to destroy now... We're done...
-    profile->GetOriginalProfile()->DestroyOffTheRecordProfile();
   }
 }
 
@@ -41,7 +47,7 @@ ProfileDestroyer::ProfileDestroyer(
 ProfileDestroyer::~ProfileDestroyer() {
   // Check again, in case other render hosts were added while we were
   // waiting for the previous ones to go away...
-  DestroyOffTheRecordProfile(profile_);
+  DestroyProfileWhenAppropriate(profile_);
 }
 
 void ProfileDestroyer::Observe(int type,
diff --git a/chrome/browser/profiles/profile_destroyer.h b/chrome/browser/profiles/profile_destroyer.h
index 9db48c5..a238e41 100644
--- a/chrome/browser/profiles/profile_destroyer.h
+++ b/chrome/browser/profiles/profile_destroyer.h
@@ -23,7 +23,7 @@ class ProfileDestroyer
     : public content::NotificationObserver,
       public base::RefCounted<ProfileDestroyer> {
  public:
-  static void DestroyOffTheRecordProfile(Profile* const profile);
+  static void DestroyProfileWhenAppropriate(Profile* const profile);
 
  private:
   friend class base::RefCounted<ProfileDestroyer>;
diff --git a/chrome/browser/profiles/profile_destroyer_unittest.cc b/chrome/browser/profiles/profile_destroyer_unittest.cc
new file mode 100644
index 0000000..c9777a4
--- /dev/null
+++ b/chrome/browser/profiles/profile_destroyer_unittest.cc
@@ -0,0 +1,138 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/profiles/profile_destroyer.h"
+
+#include "chrome/test/base/browser_with_test_window_test.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/site_instance.h"
+
+class TestingOffTheRecordDestructionProfile : public TestingProfile {
+ public:
+  TestingOffTheRecordDestructionProfile() : destroyed_otr_profile_(false) {
+    set_incognito(true);
+  }
+  virtual void DestroyOffTheRecordProfile() OVERRIDE {
+    destroyed_otr_profile_ = true;
+  }
+  bool destroyed_otr_profile_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestingOffTheRecordDestructionProfile);
+};
+
+class TestingOriginalDestructionProfile : public TestingProfile {
+ public:
+  TestingOriginalDestructionProfile() : destroyed_otr_profile_(false) {
+    DCHECK_EQ(kNull, living_instance_);
+    living_instance_ = this;
+  }
+  virtual ~TestingOriginalDestructionProfile() {
+    DCHECK_EQ(this, living_instance_);
+    living_instance_ = NULL;
+  }
+  virtual void DestroyOffTheRecordProfile() OVERRIDE {
+    SetOffTheRecordProfile(NULL);
+    destroyed_otr_profile_ = true;
+  }
+  bool destroyed_otr_profile_;
+  static TestingOriginalDestructionProfile* living_instance_;
+
+  // This is to avoid type casting in DCHECK_EQ & EXPECT_NE.
+  static const TestingOriginalDestructionProfile* kNull;
+
+  DISALLOW_COPY_AND_ASSIGN(TestingOriginalDestructionProfile);
+};
+const TestingOriginalDestructionProfile*
+    TestingOriginalDestructionProfile::kNull = NULL;
+
+TestingOriginalDestructionProfile*
+    TestingOriginalDestructionProfile::living_instance_ = NULL;
+
+class ProfileDestroyerTest : public BrowserWithTestWindowTest {
+ public:
+  ProfileDestroyerTest() : off_the_record_profile_(NULL) {}
+
+ protected:
+  virtual TestingProfile* CreateProfile() OVERRIDE {
+    if (off_the_record_profile_ == NULL)
+      off_the_record_profile_ = new TestingOffTheRecordDestructionProfile();
+    return off_the_record_profile_;
+  }
+  TestingOffTheRecordDestructionProfile* off_the_record_profile_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProfileDestroyerTest);
+};
+
+TEST_F(ProfileDestroyerTest, DelayProfileDestruction) {
+  scoped_refptr<content::SiteInstance> instance1(
+      content::SiteInstance::Create(off_the_record_profile_));
+  scoped_ptr<content::RenderProcessHost> render_process_host1;
+  render_process_host1.reset(instance1->GetProcess());
+  ASSERT_TRUE(render_process_host1.get() != NULL);
+
+  scoped_refptr<content::SiteInstance> instance2(
+      content::SiteInstance::Create(off_the_record_profile_));
+  scoped_ptr<content::RenderProcessHost> render_process_host2;
+  render_process_host2.reset(instance2->GetProcess());
+  ASSERT_TRUE(render_process_host2.get() != NULL);
+
+  // destroying the browser should not destroy the off the record profile...
+  set_browser(NULL);
+  EXPECT_FALSE(off_the_record_profile_->destroyed_otr_profile_);
+
+  // until we destroy the render process host holding on to it...
+  render_process_host1.release()->Cleanup();
+
+  // And asynchronicity kicked in properly.
+  MessageLoop::current()->RunAllPending();
+  EXPECT_FALSE(off_the_record_profile_->destroyed_otr_profile_);
+
+  // I meant, ALL the render process hosts... :-)
+  render_process_host2.release()->Cleanup();
+  MessageLoop::current()->RunAllPending();
+  EXPECT_TRUE(off_the_record_profile_->destroyed_otr_profile_);
+}
+
+TEST_F(ProfileDestroyerTest, DelayOriginalProfileDestruction) {
+  TestingOriginalDestructionProfile* original_profile =
+      new TestingOriginalDestructionProfile;
+
+  TestingOffTheRecordDestructionProfile* off_the_record_profile =
+      new TestingOffTheRecordDestructionProfile;
+
+  original_profile->SetOffTheRecordProfile(off_the_record_profile);
+
+  scoped_refptr<content::SiteInstance> instance1(
+      content::SiteInstance::Create(off_the_record_profile));
+  scoped_ptr<content::RenderProcessHost> render_process_host1;
+  render_process_host1.reset(instance1->GetProcess());
+  ASSERT_TRUE(render_process_host1.get() != NULL);
+
+  // Trying to destroy the original profile should be delayed until associated
+  // off the record profile is released by all render process hosts.
+  ProfileDestroyer::DestroyProfileWhenAppropriate(original_profile);
+  EXPECT_NE(TestingOriginalDestructionProfile::kNull,
+            TestingOriginalDestructionProfile::living_instance_);
+  EXPECT_FALSE(original_profile->destroyed_otr_profile_);
+
+  render_process_host1.release()->Cleanup();
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(NULL, TestingOriginalDestructionProfile::living_instance_);
+
+  // And the same protection should apply to the main profile.
+  TestingOriginalDestructionProfile* main_profile =
+      new TestingOriginalDestructionProfile;
+  scoped_refptr<content::SiteInstance> instance2(
+      content::SiteInstance::Create(main_profile));
+  scoped_ptr<content::RenderProcessHost> render_process_host2;
+  render_process_host2.reset(instance2->GetProcess());
+  ASSERT_TRUE(render_process_host2.get() != NULL);
+
+  ProfileDestroyer::DestroyProfileWhenAppropriate(main_profile);
+  EXPECT_EQ(main_profile, TestingOriginalDestructionProfile::living_instance_);
+  render_process_host2.release()->Cleanup();
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(NULL, TestingOriginalDestructionProfile::living_instance_);
+}
diff --git a/chrome/browser/profiles/profile_manager.cc b/chrome/browser/profiles/profile_manager.cc
index 5da3be7..37c09c0 100644
--- a/chrome/browser/profiles/profile_manager.cc
+++ b/chrome/browser/profiles/profile_manager.cc
@@ -20,6 +20,7 @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
+#include "chrome/browser/profiles/profile_destroyer.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_metrics.h"
 #include "chrome/browser/sessions/session_service_factory.h"
@@ -924,3 +925,7 @@ void ProfileManager::RunCallbacks(const std::vector<CreateCallback>& callbacks,
   for (size_t i = 0; i < callbacks.size(); ++i)
     callbacks[i].Run(profile, status);
 }
+
+ProfileManager::ProfileInfo::~ProfileInfo() {
+  ProfileDestroyer::DestroyProfileWhenAppropriate(profile.release());
+}
diff --git a/chrome/browser/profiles/profile_manager.h b/chrome/browser/profiles/profile_manager.h
index 7d8b1b8..b770020 100644
--- a/chrome/browser/profiles/profile_manager.h
+++ b/chrome/browser/profiles/profile_manager.h
@@ -219,7 +219,7 @@ class ProfileManager : public base::NonThreadSafe,
         : profile(profile), created(created) {
     }
 
-    ~ProfileInfo() {}
+    ~ProfileInfo();
 
     scoped_ptr<Profile> profile;
     // Whether profile has been fully loaded (created and initialized).
diff --git a/chrome/browser/profiles/profile_manager_unittest.cc b/chrome/browser/profiles/profile_manager_unittest.cc
index 7cfbb27..ee822b0 100644
--- a/chrome/browser/profiles/profile_manager_unittest.cc
+++ b/chrome/browser/profiles/profile_manager_unittest.cc
@@ -501,13 +501,16 @@ TEST_F(ProfileManagerTest, LastOpenedProfilesDoesNotContainIncognito) {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
 
   // Successfully create the profiles.
-  Profile* profile1 = profile_manager->GetProfile(dest_path1);
+  TestingProfile* profile1 =
+      static_cast<TestingProfile*>(profile_manager->GetProfile(dest_path1));
   ASSERT_TRUE(profile1);
 
-  TestingProfile* profile2 =
-      static_cast<TestingProfile*>(profile_manager->GetProfile(dest_path2));
+  // incognito profiles should not be managed by the profile manager but by the
+  // original profile.
+  TestingProfile* profile2 = new TestingProfile();
   ASSERT_TRUE(profile2);
   profile2->set_incognito(true);
+  profile1->SetOffTheRecordProfile(profile2);
 
   std::vector<Profile*> last_opened_profiles =
       profile_manager->GetLastOpenedProfiles();
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index c738146..4f0f584 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -462,7 +462,7 @@ Browser::~Browser() {
       !BrowserList::IsOffTheRecordSessionActiveForProfile(profile_)) {
     // An incognito profile is no longer needed, this indirectly frees
     // its cache and cookies once it gets destroyed at the appropriate time.
-    ProfileDestroyer::DestroyOffTheRecordProfile(profile_);
+    ProfileDestroyer::DestroyProfileWhenAppropriate(profile_);
   }
 
   // There may be pending file dialogs, we need to tell them that we've gone
diff --git a/chrome/browser/ui/browser_unittest.cc b/chrome/browser/ui/browser_unittest.cc
index b2a38b9..2995582 100644
--- a/chrome/browser/ui/browser_unittest.cc
+++ b/chrome/browser/ui/browser_unittest.cc
@@ -5,39 +5,9 @@
 #include "chrome/browser/ui/browser.h"
 
 #include "chrome/test/base/browser_with_test_window_test.h"
-#include "content/public/browser/render_process_host.h"
-#include "content/public/browser/site_instance.h"
 
 typedef BrowserWithTestWindowTest BrowserTest;
 
-class TestingOffTheRecordDestructionProfile : public TestingProfile {
- public:
-  TestingOffTheRecordDestructionProfile() : destroyed_profile_(false) {
-    set_incognito(true);
-  }
-  virtual void DestroyOffTheRecordProfile() OVERRIDE {
-    destroyed_profile_ = true;
-  }
-  bool destroyed_profile_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestingOffTheRecordDestructionProfile);
-};
-
-class BrowserTestOffTheRecord : public BrowserTest {
- public:
-  BrowserTestOffTheRecord() : off_the_record_profile_(NULL) {}
-
- protected:
-  virtual TestingProfile* CreateProfile() OVERRIDE {
-    if (off_the_record_profile_ == NULL)
-      off_the_record_profile_ = new TestingOffTheRecordDestructionProfile();
-    return off_the_record_profile_;
-  }
-  TestingOffTheRecordDestructionProfile* off_the_record_profile_;
-
-  DISALLOW_COPY_AND_ASSIGN(BrowserTestOffTheRecord);
-};
-
 // Various assertions around setting show state.
 TEST_F(BrowserTest, GetSavedWindowShowState) {
   // Default show state is SHOW_STATE_DEFAULT.
@@ -53,35 +23,3 @@ TEST_F(BrowserTest, GetSavedWindowShowState) {
   browser()->set_show_state(ui::SHOW_STATE_FULLSCREEN);
   EXPECT_EQ(ui::SHOW_STATE_FULLSCREEN, browser()->GetSavedWindowShowState());
 }
-
-TEST_F(BrowserTestOffTheRecord, DelayProfileDestruction) {
-  scoped_refptr<content::SiteInstance> instance1(
-      static_cast<content::SiteInstance*>(
-          content::SiteInstance::Create(off_the_record_profile_)));
-  scoped_ptr<content::RenderProcessHost> render_process_host1;
-  render_process_host1.reset(instance1->GetProcess());
-  ASSERT_TRUE(render_process_host1.get() != NULL);
-
-  scoped_refptr<content::SiteInstance> instance2(
-      static_cast<content::SiteInstance*>(
-          content::SiteInstance::Create(off_the_record_profile_)));
-  scoped_ptr<content::RenderProcessHost> render_process_host2;
-  render_process_host2.reset(instance2->GetProcess());
-  ASSERT_TRUE(render_process_host2.get() != NULL);
-
-  // destroying the browser should not destroy the off the record profile...
-  set_browser(NULL);
-  EXPECT_FALSE(off_the_record_profile_->destroyed_profile_);
-
-  // until we destroy the render process host holding on to it...
-  render_process_host1.release()->Cleanup();
-
-  // And asynchronicity kicked in properly.
-  MessageLoop::current()->RunAllPending();
-  EXPECT_FALSE(off_the_record_profile_->destroyed_profile_);
-
-  // I meant, ALL the render process hosts... :-)
-  render_process_host2.release()->Cleanup();
-  MessageLoop::current()->RunAllPending();
-  EXPECT_TRUE(off_the_record_profile_->destroyed_profile_);
-}
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 89cb276..1848d36 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1650,6 +1650,7 @@
         'browser/profiles/gaia_info_update_service_unittest.cc',
         'browser/profiles/off_the_record_profile_impl_unittest.cc',
         'browser/profiles/profile_dependency_manager_unittest.cc',
+        'browser/profiles/profile_destroyer_unittest.cc',
         'browser/profiles/profile_downloader_unittest.cc',
         'browser/profiles/profile_info_cache_unittest.cc',
         'browser/profiles/profile_info_cache_unittest.h',
diff --git a/chrome/test/base/browser_with_test_window_test.cc b/chrome/test/base/browser_with_test_window_test.cc
index 47be6d8..eed311b 100644
--- a/chrome/test/base/browser_with_test_window_test.cc
+++ b/chrome/test/base/browser_with_test_window_test.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -8,6 +8,7 @@
 #include <ole2.h>
 #endif  // defined(OS_WIN)
 
+#include "chrome//browser/profiles/profile_destroyer.h"
 #include "chrome/browser/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
@@ -40,7 +41,7 @@ BrowserWithTestWindowTest::BrowserWithTestWindowTest()
 void BrowserWithTestWindowTest::SetUp() {
   testing::Test::SetUp();
 
-  profile_.reset(CreateProfile());
+  set_profile(CreateProfile());
   browser_.reset(new Browser(Browser::TYPE_TABBED, profile()));
   window_.reset(new TestBrowserWindow(browser()));
   browser_->SetWindowForTesting(window_.get());
@@ -60,6 +61,13 @@ BrowserWithTestWindowTest::~BrowserWithTestWindowTest() {
 #endif
 }
 
+void BrowserWithTestWindowTest::set_profile(TestingProfile* profile) {
+  if (profile_.get() != NULL)
+    ProfileDestroyer::DestroyProfileWhenAppropriate(profile_.release());
+
+  profile_.reset(profile);
+}
+
 TestRenderViewHost* BrowserWithTestWindowTest::TestRenderViewHostForTab(
     WebContents* web_contents) {
   return static_cast<TestRenderViewHost*>(web_contents->GetRenderViewHost());
diff --git a/chrome/test/base/browser_with_test_window_test.h b/chrome/test/base/browser_with_test_window_test.h
index ab88c6a..8865a97 100644
--- a/chrome/test/base/browser_with_test_window_test.h
+++ b/chrome/test/base/browser_with_test_window_test.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -79,9 +79,7 @@ class BrowserWithTestWindowTest : public testing::Test {
   }
 
   TestingProfile* profile() const { return profile_.get(); }
-  void set_profile(TestingProfile* profile) {
-    profile_.reset(profile);
-  }
+  void set_profile(TestingProfile* profile);
 
   MessageLoop* message_loop() { return &ui_loop_; }
 
diff --git a/chrome/test/base/testing_browser_process.cc b/chrome/test/base/testing_browser_process.cc
index 5843223..cd56eb1 100644
--- a/chrome/test/base/testing_browser_process.cc
+++ b/chrome/test/base/testing_browser_process.cc
@@ -15,6 +15,8 @@
 #include "chrome/browser/printing/print_preview_tab_controller.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/test/test_browser_thread.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "ui/base/clipboard/clipboard.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -29,6 +31,14 @@ TestingBrowserProcess::TestingBrowserProcess()
 
 TestingBrowserProcess::~TestingBrowserProcess() {
   EXPECT_FALSE(local_state_);
+  // The profile manager must be destroyed in the UI thread.
+  if (profile_manager_.get() != NULL &&
+      !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
+    content::TestBrowserThread ui_thread_(content::BrowserThread::UI);
+    // There shouldn't be any render process host still alive at that point.
+    DCHECK(content::RenderProcessHost::AllHostsIterator().IsAtEnd());
+    profile_manager_.reset(NULL);
+  }
 }
 
 void TestingBrowserProcess::ResourceDispatcherHostCreated() {