Ensure incognito TestingProfiles are incognito for their whole lifetime.

chrome/test/base/testing_profile.cc

Index: chrome/test/base/testing_profile.cc
diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc
index 84aa2519a04bdd64e3d25238ac7553df235557ec..acec04caf98694e0bafb1fe2d34016feb9bb1b50 100644
--- a/chrome/test/base/testing_profile.cc
+++ b/chrome/test/base/testing_profile.cc
@@ -184,7 +184,6 @@ const char TestingProfile::kTestUserProfileDir[] = "Default";
 TestingProfile::TestingProfile()
     : start_time_(Time::Now()),
       testing_prefs_(NULL),
-      incognito_(false),
       force_incognito_(false),
       original_profile_(NULL),
       guest_session_(false),
@@ -203,7 +202,6 @@ TestingProfile::TestingProfile()
 TestingProfile::TestingProfile(const base::FilePath& path)
     : start_time_(Time::Now()),
       testing_prefs_(NULL),
-      incognito_(false),
       force_incognito_(false),
       original_profile_(NULL),
       guest_session_(false),
@@ -221,7 +219,6 @@ TestingProfile::TestingProfile(const base::FilePath& path,
                                Delegate* delegate)
     : start_time_(Time::Now()),
       testing_prefs_(NULL),
-      incognito_(false),
       force_incognito_(false),
       original_profile_(NULL),
       guest_session_(false),
@@ -248,7 +245,7 @@ TestingProfile::TestingProfile(
     scoped_refptr<ExtensionSpecialStoragePolicy> extension_policy,
 #endif
     scoped_ptr<PrefServiceSyncable> prefs,
-    bool incognito,
+    TestingProfile* incognito_parent,
     bool guest_session,
     const std::string& supervised_user_id,
     scoped_ptr<policy::PolicyService> policy_service,
@@ -256,9 +253,8 @@ TestingProfile::TestingProfile(
     : start_time_(Time::Now()),
       prefs_(prefs.release()),
       testing_prefs_(NULL),
-      incognito_(incognito),
       force_incognito_(false),
-      original_profile_(NULL),
+      original_profile_(incognito_parent),
       guest_session_(guest_session),
       supervised_user_id_(supervised_user_id),
       last_session_exited_cleanly_(true),
@@ -271,6 +267,9 @@ TestingProfile::TestingProfile(
       resource_context_(NULL),
       delegate_(delegate),
       policy_service_(policy_service.release()) {
+  if (incognito_parent)
+    incognito_parent->SetTestingOffTheRecordProfile(this);  // Takes ownership.
+
   // If no profile path was supplied, create one.
   if (profile_path_.empty()) {
     CreateTempProfileDir();
@@ -342,6 +341,8 @@ void TestingProfile::Init() {
 
   if (prefs_.get())
     user_prefs::UserPrefs::Set(this, prefs_.get());
+  else if (IsOffTheRecord())
+    CreateIncognitoPrefService();
   else
     CreateTestingPrefService();
 
@@ -359,10 +360,10 @@ void TestingProfile::Init() {
       this, extensions::TestExtensionSystem::Build);
 #endif
 
-  // If no original profile was specified for this profile: register preferences
-  // even if this is an incognito profile - this allows tests to create a
-  // standalone incognito profile while still having prefs registered.
-  if (!IsOffTheRecord() || !original_profile_) {
+  // Prefs for incognito profiles are set in CreateIncognitoPrefService() by
+  // simulating ProfileImpl::GetOffTheRecordPrefs().
+  if (!IsOffTheRecord()) {
+    DCHECK(!original_profile_);
     user_prefs::PrefRegistrySyncable* pref_registry =
         static_cast<user_prefs::PrefRegistrySyncable*>(
             prefs_->DeprecatedGetPrefRegistry());
@@ -380,11 +381,13 @@ void TestingProfile::Init() {
 #endif
 
 #if defined(ENABLE_MANAGED_USERS)
-  SupervisedUserSettingsService* settings_service =
-      SupervisedUserSettingsServiceFactory::GetForProfile(this);
-  TestingPrefStore* store = new TestingPrefStore();
-  settings_service->Init(store);
-  store->SetInitializationCompleted();
+  if (!IsOffTheRecord()) {
+    SupervisedUserSettingsService* settings_service =
+        SupervisedUserSettingsServiceFactory::GetForProfile(this);
+    TestingPrefStore* store = new TestingPrefStore();
+    settings_service->Init(store);
+    store->SetInitializationCompleted();
+  }
 #endif
 
   profile_name_ = "testing_profile";
@@ -409,6 +412,9 @@ TestingProfile::~TestingProfile() {
   // Revert to non-incognito mode before shutdown.
   force_incognito_ = false;
 
+  // If this profile owns an incognito profile, tear it down first.
+  incognito_profile_.reset();

[tapted, 2014/09/12 07:54:09]

This was needed to resolve a lifetime issue in the
UserCloudPolicyManagerChromeOSTest tests, under
BrowserContextKeyedServiceFactory::Disassociate(). It matches what
ProfileImpl::~ProfileImpl() does.

+
   // Any objects holding live URLFetchers should be deleted before teardown.
   TemplateURLFetcherFactory::ShutdownForProfile(this);
 
@@ -601,8 +607,7 @@ scoped_refptr<base::SequencedTaskRunner> TestingProfile::GetIOTaskRunner() {
 }
 
 TestingPrefServiceSyncable* TestingProfile::GetTestingPrefService() {
-  if (!prefs_.get())
-    CreateTestingPrefService();
+  DCHECK(prefs_);
   DCHECK(testing_prefs_);
   return testing_prefs_;
 }
@@ -618,13 +623,13 @@ std::string TestingProfile::GetProfileName() {
 Profile::ProfileType TestingProfile::GetProfileType() const {
   if (guest_session_)
     return GUEST_PROFILE;
-  if (force_incognito_ || incognito_)
+  if (force_incognito_ || original_profile_)
     return INCOGNITO_PROFILE;
   return REGULAR_PROFILE;
 }
 
 bool TestingProfile::IsOffTheRecord() const {
-  return force_incognito_ || incognito_;
+  return force_incognito_ || original_profile_;
 }
 
 void TestingProfile::SetOffTheRecordProfile(scoped_ptr<Profile> profile) {

[sky, 2014/09/12 15:33:29]

Can this be changed to take a TestingProfile? It would be nice if it set up the
necessarily backpointers.

[tapted, 2014/09/15 05:05:16]

Not easily - that was actually my first attempt, but the
OffTheRecordProfileImplTest unit tests pass an instance of
OffTheRecordProfileImpl rather than TestingProfile. However,
OffTheRecordProfileImpl sets its original_profile in the constructor too, so
I've added `DCHECK_EQ(this, profile->GetOriginalProfile());` to ensure the
backpointer is already set.

@@ -632,21 +637,11 @@ void TestingProfile::SetOffTheRecordProfile(scoped_ptr<Profile> profile) {
   incognito_profile_ = profile.Pass();
 }
 
-void TestingProfile::SetOriginalProfile(Profile* profile) {
-  DCHECK(IsOffTheRecord());
-  original_profile_ = profile;
-}
-
 Profile* TestingProfile::GetOffTheRecordProfile() {
   if (IsOffTheRecord())
     return this;
-  if (!incognito_profile_) {
-    TestingProfile::Builder builder;
-    builder.SetIncognito();
-    scoped_ptr<TestingProfile> incognito_test_profile(builder.Build());
-    incognito_test_profile->SetOriginalProfile(this);
-    SetOffTheRecordProfile(incognito_test_profile.PassAs<Profile>());
-  }
+  if (!incognito_profile_)
+    TestingProfile::Builder().BuildIncognito(this);
   return incognito_profile_.get();
 }
 
@@ -697,6 +692,15 @@ void TestingProfile::CreateTestingPrefService() {
   chrome::RegisterUserProfilePrefs(testing_prefs_->registry());
 }
 
+void TestingProfile::CreateIncognitoPrefService() {
+  DCHECK(original_profile_);
+  DCHECK(!testing_prefs_);
+  // Simplified version of ProfileImpl::GetOffTheRecordPrefs(). Note this
+  // leaves testing_prefs_ unset.
+  prefs_.reset(original_profile_->prefs_->CreateIncognitoPrefService(NULL));
+  user_prefs::UserPrefs::Set(this, prefs_.get());
+}
+
 void TestingProfile::CreateProfilePolicyConnector() {
 #if defined(ENABLE_CONFIGURATION_POLICY)
   schema_registry_service_ =
@@ -722,10 +726,14 @@ if (!policy_service_) {
            policy::ProfilePolicyConnectorFactory::GetForProfile(this));
 }
 
+void TestingProfile::SetTestingOffTheRecordProfile(Profile* incognito_profile) {

[sky, 2014/09/12 15:33:29]

Is there a reason we need this? Why not have the constructor call
SetOffTheRecordProfile directly?

[tapted, 2014/09/15 05:05:16]

Nope! Removed. I think I had this taking a TestingProfile* at one point, but
that became unnecessary. 

+  DCHECK(!IsOffTheRecord());
+  DCHECK(!incognito_profile_);
+  SetOffTheRecordProfile(make_scoped_ptr(incognito_profile));
+}
+
 PrefService* TestingProfile::GetPrefs() {
-  if (!prefs_.get()) {
-    CreateTestingPrefService();
-  }
+  DCHECK(prefs_);
   return prefs_.get();
 }
 
@@ -921,7 +929,6 @@ Profile::ExitType TestingProfile::GetLastSessionExitType() {
 TestingProfile::Builder::Builder()
     : build_called_(false),
       delegate_(NULL),
-      incognito_(false),
       guest_session_(false) {
 }
 
@@ -948,10 +955,6 @@ void TestingProfile::Builder::SetPrefService(
   pref_service_ = prefs.Pass();
 }
 
-void TestingProfile::Builder::SetIncognito() {
-  incognito_ = true;
-}
-
 void TestingProfile::Builder::SetGuestSession() {
   guest_session_ = true;
 }
@@ -976,16 +979,35 @@ scoped_ptr<TestingProfile> TestingProfile::Builder::Build() {
   DCHECK(!build_called_);
   build_called_ = true;
 
-  return scoped_ptr<TestingProfile>(new TestingProfile(
-      path_,
-      delegate_,
+  return scoped_ptr<TestingProfile>(new TestingProfile(path_,
+                                                       delegate_,
+#if defined(ENABLE_EXTENSIONS)
+                                                       extension_policy_,
+#endif
+                                                       pref_service_.Pass(),
+                                                       NULL,
+                                                       guest_session_,
+                                                       supervised_user_id_,
+                                                       policy_service_.Pass(),
+                                                       testing_factories_));
+}
+
+TestingProfile* TestingProfile::Builder::BuildIncognito(
+    TestingProfile* original_profile) {
+  DCHECK(!build_called_);
+  DCHECK(original_profile);
+  build_called_ = true;
+
+  // Note: Owned by |original_profile|.
+  return new TestingProfile(path_,
+                            delegate_,
 #if defined(ENABLE_EXTENSIONS)
-      extension_policy_,
+                            extension_policy_,
 #endif
-      pref_service_.Pass(),
-      incognito_,
-      guest_session_,
-      supervised_user_id_,
-      policy_service_.Pass(),
-      testing_factories_));
+                            pref_service_.Pass(),
+                            original_profile,
+                            guest_session_,
+                            supervised_user_id_,
+                            policy_service_.Pass(),
+                            testing_factories_);
 }