Redirect non-guest incognito contexts when creating AppServiceProxy.

This CL stops crashes when the App Service is inadvertently accessed
from an incognito context, which is often inappropriate, but exists in
legacy codepaths that are not well tested.

A DumpWithoutCrashing is added to surface and audit these codepaths, and
a test is added to ensure the correct behaviour.

BUG=1122463

Change-Id: I83d3bdb2219d9fe22727df5c31422ed8d7d9d5f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2379451
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802465}

chrome/browser/apps/app_service/app_service_proxy_factory.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
 
+#include "base/debug/dump_without_crashing.h"
 #include "base/feature_list.h"
 #include "chrome/browser/apps/app_service/app_service_proxy.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
@@ -50,7 +51,17 @@ bool AppServiceProxyFactory::IsAppServiceAvailableForProfile(Profile* profile) {
 
 // static
 AppServiceProxy* AppServiceProxyFactory::GetForProfile(Profile* profile) {
-  DCHECK(IsAppServiceAvailableForProfile(profile));
+  // TODO(https://crbug.com/1122463): remove this and convert back to a DCHECK
+  // once we have audited and removed code paths that call here with a profile
+  // that doesn't have an App Service.
+  if (!IsAppServiceAvailableForProfile(profile)) {
+    DVLOG(1) << "Called AppServiceProxyFactory::GetForProfile() on a profile "
+                "which does not contain an AppServiceProxy. Please check "
+                "whether this is appropriate as you may be leaking information "
+                "out of this profile. Returning the AppServiceProxy attached "
+                "to the parent profile instead.";
+    base::debug::DumpWithoutCrashing();
+  }
 
   auto* proxy = static_cast<AppServiceProxy*>(
       AppServiceProxyFactory::GetInstance()->GetServiceForBrowserContext(
@@ -99,13 +110,16 @@ content::BrowserContext* AppServiceProxyFactory::GetBrowserContextToUse(
   }
 
   // We must have a proxy in guest mode to ensure default extension-based apps
-  // are served. Otherwise, don't create the app service for incognito profiles.
+  // are served.
   if (profile->IsGuestSession()) {
     return chrome::GetBrowserContextOwnInstanceInIncognito(context);
   }
 #endif  // OS_CHROMEOS
 
-  return BrowserContextKeyedServiceFactory::GetBrowserContextToUse(context);
+  // TODO(https://crbug.com/1122463): replace this with
+  // BrowserContextKeyedServiceFactory::GetBrowserContextToUse(context) once
+  // all non-guest incognito accesses have been removed.
+  return chrome::GetBrowserContextRedirectedInIncognito(context);
 }
 
 bool AppServiceProxyFactory::ServiceIsCreatedWithBrowserContext() const {

chrome/browser/apps/app_service/app_service_proxy_unittest.cc

@@ -2,11 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <memory>
 #include <utility>
 #include <vector>
 
 #include "base/callback.h"
 #include "chrome/browser/apps/app_service/app_service_proxy.h"
+#include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/browser_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image_skia_rep.h"
@@ -81,6 +85,8 @@ class AppServiceProxyTest : public testing::Test {
   int NumOuterFinishedCallbacks() { return num_outer_finished_callbacks_; }
 
   int num_outer_finished_callbacks_ = 0;
+
+  content::BrowserTaskEnvironment task_environment_;
 };
 
 TEST_F(AppServiceProxyTest, IconCache) {
@@ -181,3 +187,30 @@ TEST_F(AppServiceProxyTest, IconCoalescer) {
   EXPECT_EQ(3, fake.NumInnerFinishedCallbacks());
   EXPECT_EQ(6, NumOuterFinishedCallbacks());
 }
+
+TEST_F(AppServiceProxyTest, ProxyAccessPerProfile) {
+  TestingProfile::Builder profile_builder;
+
+  // We expect an App Service in a regular profile.
+  auto profile = profile_builder.Build();
+  auto* proxy = apps::AppServiceProxyFactory::GetForProfile(profile.get());
+  EXPECT_TRUE(proxy);
+
+  // We expect the same App Service in the incognito profile branched from that
+  // regular profile.
+  // TODO(https://crbug.com/1122463): this should be nullptr once we address all
+  // incognito access to the App Service.
+  TestingProfile::Builder incognito_builder;
+  auto* incognito_proxy = apps::AppServiceProxyFactory::GetForProfile(
+      incognito_builder.BuildIncognito(profile.get()));
+  EXPECT_EQ(proxy, incognito_proxy);
+
+  // We expect a different App Service in the Guest Session profile.
+  TestingProfile::Builder guest_builder;
+  guest_builder.SetGuestSession();
+  auto guest_profile = guest_builder.Build();
+  auto* guest_proxy =
+      apps::AppServiceProxyFactory::GetForProfile(guest_profile.get());
+  EXPECT_TRUE(guest_proxy);
+  EXPECT_NE(guest_proxy, proxy);
+}