Fix holding space in guest sessions.

Holding space is enabled for guest sessions but previously redirected to
use original profile instead of OTR profile. This caused:

- Thumbnails to fail to render
- Opening Files app via util to fail

These failures are due to expectations further downstream that guest
sessions use OTR profiles.

To address this:
- Redirect to primary OTR profile in holding space for guest sessions
- Allow OTR profiles in FileChangeService for guest sessions

Note that FileChangeService was previously *not* created for OTR
profiles but they will now be allowed if the profile is a guest session.

This CL also modified FileChangeService to *not* create the service w/
the browser since the service does not need to exist until interacted
with.

Bug: 1165733, 1164329
Change-Id: I032aea25171594bdb0666c87564a80f7c0cfe203
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2625799
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Sergei Datsenko <dats@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842893}

chrome/browser/chromeos/fileapi/file_change_service_factory.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/chromeos/fileapi/file_change_service_factory.h"
 
 #include "chrome/browser/chromeos/fileapi/file_change_service.h"
+#include "chrome/browser/profiles/profile.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 
 namespace chromeos {
@@ -28,13 +29,24 @@ FileChangeServiceFactory::FileChangeServiceFactory()
 
 FileChangeServiceFactory::~FileChangeServiceFactory() = default;
 
-KeyedService* FileChangeServiceFactory::BuildServiceInstanceFor(
+content::BrowserContext* FileChangeServiceFactory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
-  return new FileChangeService();
+  Profile* const profile = Profile::FromBrowserContext(context);
+
+  // Guest sessions are supported and guest OTR profiles are allowed.
+  if (profile->IsGuestSession())
+    return profile;
+
+  // Don't create the service for OTR profiles outside of guest sessions.
+  return profile->IsOffTheRecord() ? nullptr : profile;
 }
 
-bool FileChangeServiceFactory::ServiceIsCreatedWithBrowserContext() const {
-  return true;
+KeyedService* FileChangeServiceFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  Profile* const profile = Profile::FromBrowserContext(context);
+  if (profile->IsOffTheRecord())
+    CHECK(profile->IsGuestSession());
+  return new FileChangeService();
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/fileapi/file_change_service_factory.h

@@ -37,9 +37,10 @@ class FileChangeServiceFactory : public BrowserContextKeyedServiceFactory {
   ~FileChangeServiceFactory() override;
 
   // BrowserContextKeyedServiceFactory:
+  content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const override;
   KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* context) const override;
-  bool ServiceIsCreatedWithBrowserContext() const override;
 };
 
 }  // namespace chromeos

chrome/browser/chromeos/fileapi/file_change_service_unittest.cc

@@ -278,6 +278,61 @@ TEST_F(FileChangeServiceTest, CreatesServiceInstancesPerProfile) {
   ASSERT_NE(primary_profile_service, secondary_profile_service);
 }
 
+// Verifies service instances are *not* created for OTR profiles.
+TEST_F(FileChangeServiceTest, DoesntCreateServiceInstanceForOTRProfile) {
+  auto* factory = FileChangeServiceFactory::GetInstance();
+  ASSERT_TRUE(factory);
+
+  // `FileChangeService` should be created for non-OTR profile.
+  auto* profile = GetProfile();
+  ASSERT_TRUE(profile);
+  ASSERT_FALSE(profile->IsOffTheRecord());
+  ASSERT_TRUE(factory->GetService(profile));
+
+  // `FileChangeService` should *not* be created for OTR profile.
+  auto* otr_profile =
+      TestingProfile::Builder().BuildIncognito(profile->AsTestingProfile());
+  ASSERT_TRUE(otr_profile);
+  ASSERT_TRUE(otr_profile->IsOffTheRecord());
+  ASSERT_FALSE(factory->GetService(otr_profile));
+}
+
+// Verifies service instance *are* created for guest OTR profiles.
+TEST_F(FileChangeServiceTest, CreatesServiceInstanceForOTRGuestProfile) {
+  auto* factory = FileChangeServiceFactory::GetInstance();
+  ASSERT_TRUE(factory);
+
+  // Construct a guest profile.
+  TestingProfile::Builder guest_profile_builder;
+  guest_profile_builder.SetGuestSession();
+  guest_profile_builder.SetProfileName("guest_profile");
+  std::unique_ptr<TestingProfile> guest_profile = guest_profile_builder.Build();
+
+  // Service instances should be created for guest profiles.
+  ASSERT_TRUE(guest_profile);
+  ASSERT_FALSE(guest_profile->IsOffTheRecord());
+  FileChangeService* const guest_profile_service =
+      factory->GetService(guest_profile.get());
+  ASSERT_TRUE(guest_profile_service);
+
+  // Construct an OTR profile from `guest_profile`.
+  TestingProfile::Builder otr_guest_profile_builder;
+  otr_guest_profile_builder.SetGuestSession();
+  otr_guest_profile_builder.SetProfileName(guest_profile->GetProfileUserName());
+  Profile* const otr_guest_profile =
+      otr_guest_profile_builder.BuildIncognito(guest_profile.get());
+  ASSERT_TRUE(otr_guest_profile);
+  ASSERT_TRUE(otr_guest_profile->IsOffTheRecord());
+
+  // Service instances *should* be created for OTR guest profiles.
+  FileChangeService* const otr_guest_profile_service =
+      factory->GetService(otr_guest_profile);
+  ASSERT_TRUE(otr_guest_profile_service);
+
+  // OTR service instances should be distinct from non-OTR service instances.
+  ASSERT_NE(otr_guest_profile_service, guest_profile_service);
+}
+
 // Verifies `OnFileCopied` events are propagated to observers.
 TEST_F(FileChangeServiceTest, PropagatesOnFileCopiedEvents) {
   auto* profile = GetProfile();

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_factory.cc

@@ -40,19 +40,13 @@ HoldingSpaceKeyedService* HoldingSpaceKeyedServiceFactory::GetService(
 content::BrowserContext*
 HoldingSpaceKeyedServiceFactory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
-  Profile* profile = Profile::FromBrowserContext(context);
+  Profile* const profile = Profile::FromBrowserContext(context);
 
-  user_manager::User* user =
-      chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
-  if (!user)
-    return nullptr;
-
-  // Guest users are supported but should redirect to create the holding space
-  // service for the original (e.g. non-incognito) profile.
-  if (user->GetType() == user_manager::USER_TYPE_GUEST)
-    return profile->GetOriginalProfile();
+  // Guest sessions are supported but redirect to the primary OTR profile.
+  if (profile->IsGuestSession())
+    return profile->GetPrimaryOTRProfile();
 
-  // Don't create the service for off the record profiles of other user types.
+  // Don't create the service for OTR profiles outside of guest sessions.
   return profile->IsOffTheRecord() ? nullptr : context;
 }
 
@@ -61,8 +55,8 @@ KeyedService* HoldingSpaceKeyedServiceFactory::BuildServiceInstanceFor(
   if (!features::IsTemporaryHoldingSpaceEnabled())
     return nullptr;
 
-  Profile* profile = Profile::FromBrowserContext(context);
-  DCHECK(!profile->IsOffTheRecord());
+  Profile* const profile = Profile::FromBrowserContext(context);
+  DCHECK_EQ(profile->IsGuestSession(), profile->IsOffTheRecord());
 
   user_manager::User* user =
       chromeos::ProfileHelper::Get()->GetUserByProfile(profile);

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_unittest.cc

@@ -305,20 +305,6 @@ class HoldingSpaceKeyedServiceTest : public BrowserWithTestWindowTest {
              base::BindRepeating(&BuildVolumeManager)}});
   }
 
-  TestingProfile* CreateGuestProfile() {
-    user_manager::User* guest_user = fake_user_manager_->AddGuestUser();
-    fake_user_manager_->LoginUser(fake_user_manager_->GetGuestAccountId());
-
-    chromeos::ProfileHelper::Get()->SetProfileToUserMappingForTesting(
-        guest_user);
-
-    return profile_manager()->CreateTestingProfile(
-        guest_user->GetAccountId().GetUserEmail(),
-        /*testing_factories=*/{
-            {file_manager::VolumeManagerFactory::GetInstance(),
-             base::BindRepeating(&BuildVolumeManager)}});
-  }
-
   TestingProfile* CreateSecondaryProfile(
       std::unique_ptr<sync_preferences::PrefServiceSyncable> prefs = nullptr) {
     const std::string kSecondaryProfileName = "secondary_profile";
@@ -547,30 +533,56 @@ TEST_F(HoldingSpaceKeyedServiceTest, AddScreenshotItem) {
 }
 
 TEST_F(HoldingSpaceKeyedServiceTest, GuestUserProfile) {
-  // Service instances should be created for guest users.
-  TestingProfile* const guest_profile = CreateGuestProfile();
+  // Construct a guest session profile.
+  TestingProfile::Builder guest_profile_builder;
+  guest_profile_builder.SetGuestSession();
+  guest_profile_builder.SetProfileName("guest_profile");
+  guest_profile_builder.AddTestingFactories(
+      {{file_manager::VolumeManagerFactory::GetInstance(),
+        base::BindRepeating(&BuildVolumeManager)}});
+  std::unique_ptr<TestingProfile> guest_profile = guest_profile_builder.Build();
+
+  // Service instances should be created for guest sessions but note that the
+  // service factory will redirect to use the primary OTR profile.
   ASSERT_TRUE(guest_profile);
   ASSERT_FALSE(guest_profile->IsOffTheRecord());
   HoldingSpaceKeyedService* const guest_profile_service =
-      HoldingSpaceKeyedServiceFactory::GetInstance()->GetService(guest_profile);
+      HoldingSpaceKeyedServiceFactory::GetInstance()->GetService(
+          guest_profile.get());
   ASSERT_TRUE(guest_profile_service);
 
-  // Construct an incognito profile from `guest_profile`.
-  TestingProfile::Builder incognito_guest_profile_builder;
-  incognito_guest_profile_builder.SetGuestSession();
-  incognito_guest_profile_builder.SetProfileName(
+  // Since the service factory redirects to use the primary OTR profile in the
+  // case of guest sessions, retrieving the service instance for the primary OTR
+  // profile should yield the same result as retrieving the service instance for
+  // a non-OTR guest session profile.
+  ASSERT_TRUE(guest_profile->GetPrimaryOTRProfile());
+  HoldingSpaceKeyedService* const primary_otr_guest_profile_service =
+      HoldingSpaceKeyedServiceFactory::GetInstance()->GetService(
+          guest_profile->GetPrimaryOTRProfile());
+  ASSERT_EQ(guest_profile_service, primary_otr_guest_profile_service);
+
+  // Construct a second OTR profile from `guest_profile`.
+  TestingProfile::Builder secondary_otr_guest_profile_builder;
+  secondary_otr_guest_profile_builder.SetGuestSession();
+  secondary_otr_guest_profile_builder.SetProfileName(
       guest_profile->GetProfileUserName());
-  Profile* const incognito_guest_profile =
-      incognito_guest_profile_builder.BuildIncognito(guest_profile);
-  ASSERT_TRUE(incognito_guest_profile);
-  ASSERT_TRUE(incognito_guest_profile->IsOffTheRecord());
-
-  // Service instances should be created for guest users w/ OTR profiles but
-  // should redirect to use the original (e.g. non-incognito) profile.
-  HoldingSpaceKeyedService* const incognito_guest_profile_service =
+  TestingProfile* const secondary_otr_guest_profile =
+      secondary_otr_guest_profile_builder.BuildOffTheRecord(
+          guest_profile.get(), Profile::OTRProfileID("profile::secondary_otr"));
+  ASSERT_TRUE(secondary_otr_guest_profile);
+  ASSERT_TRUE(secondary_otr_guest_profile->IsOffTheRecord());
+
+  // Service instances should be created for non-primary OTR guest session
+  // profiles but as stated earlier the service factory will redirect to use the
+  // primary OTR profile. This means that the secondary OTR profile service
+  // instance should be equal to that explicitly created for the primary OTR
+  // profile.
+  HoldingSpaceKeyedService* const secondary_otr_guest_profile_service =
       HoldingSpaceKeyedServiceFactory::GetInstance()->GetService(
-          incognito_guest_profile);
-  ASSERT_EQ(incognito_guest_profile_service, guest_profile_service);
+          secondary_otr_guest_profile);
+  ASSERT_TRUE(secondary_otr_guest_profile_service);
+  ASSERT_EQ(primary_otr_guest_profile_service,
+            secondary_otr_guest_profile_service);
 }
 
 TEST_F(HoldingSpaceKeyedServiceTest, OffTheRecordProfile) {