Use common App Service profile checks for incognito redirect.

https://crrev.com/c/2318888 added a "GetOriginalProfile" call to fix a
crash in incognito. Unfortunally that introduced a new crash for Guest
sessions. GetOriginalProfile() returns something that crashes when
trying to open a browser - see https://crbug.com/1121553. Add a new
method to AppService to handle the profile redirects correctly and in a
single place.

Also adds more tests.

Bug: 1122480
Change-Id: I63e3d2766d61b416d1e7590bc67a937b817848ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2381230Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Rachel Carpenter <carpenterr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803399}

chrome/browser/apps/app_service/app_service_proxy_factory.cc

@@ -70,6 +70,18 @@ AppServiceProxy* AppServiceProxyFactory::GetForProfile(Profile* profile) {
   return proxy;
 }
 
+// static
+AppServiceProxy* AppServiceProxyFactory::GetForProfileRedirectInIncognito(
+    Profile* profile) {
+  // TODO(https://crbug.com/1122463): replace this API and GetForProfile() with
+  // one that allows clients to specify different levels of incognito tolerance,
+  // where the default is to not leak out of incognito.
+  if (!IsAppServiceAvailableForProfile(profile)) {
+    profile = profile->GetOriginalProfile();
+  }
+  return GetForProfile(profile);
+}
+
 // static
 AppServiceProxyFactory* AppServiceProxyFactory::GetInstance() {
   return base::Singleton<AppServiceProxyFactory>::get();

chrome/browser/apps/app_service/app_service_proxy_factory.h

@@ -22,6 +22,11 @@ class AppServiceProxyFactory : public BrowserContextKeyedServiceFactory {
 
   static AppServiceProxy* GetForProfile(Profile* profile);
 
+  // Explicitly avoids DumpWithoutCrashing() when App Service is not available
+  // for a Profile. Avoid using this unless you have spoken with App Service
+  // OWNERs.
+  static AppServiceProxy* GetForProfileRedirectInIncognito(Profile* profile);
+
   static AppServiceProxyFactory* GetInstance();
 
  private:

chrome/browser/apps/app_service/app_service_proxy_unittest.cc

@@ -214,3 +214,37 @@ TEST_F(AppServiceProxyTest, ProxyAccessPerProfile) {
   EXPECT_TRUE(guest_proxy);
   EXPECT_NE(guest_proxy, proxy);
 }
+
+TEST_F(AppServiceProxyTest, RedirectInIncognitoProxyAccessPerProfile) {
+  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 get the same App Service using GetForProfileRedirectInIncognito.
+  auto* redirected_proxy =
+      apps::AppServiceProxyFactory::GetForProfileRedirectInIncognito(
+          profile.get());
+  EXPECT_EQ(proxy, redirected_proxy);
+
+  // We expect the same App Service in the incognito profile branched from that
+  // regular profile.
+  TestingProfile::Builder incognito_builder;
+  auto* incognito_proxy =
+      apps::AppServiceProxyFactory::GetForProfileRedirectInIncognito(
+          incognito_builder.BuildIncognito(profile.get()));
+  EXPECT_EQ(proxy, incognito_proxy);
+
+  // We expect a different (but still valid) 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::GetForProfileRedirectInIncognito(
+          guest_profile.get());
+  EXPECT_TRUE(guest_proxy);
+  EXPECT_NE(guest_proxy, proxy);
+}

chrome/browser/chromeos/web_applications/help_app_integration_browsertest.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/web_applications/system_web_app_manager.h"
 #include "chrome/browser/web_applications/system_web_app_manager_browsertest.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/url_constants.h"
 #include "chromeos/components/help_app_ui/url_constants.h"
 #include "chromeos/components/web_applications/test/sandboxed_web_ui_test_base.h"
 #include "chromeos/constants/chromeos_features.h"
@@ -45,6 +46,24 @@ class HelpAppIntegrationTest : public SystemWebAppIntegrationTest {
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
+// Waits for and expects that the correct url is opened.
+void WaitForAppToOpen(const GURL& expected_url) {
+  // Start with a number of browsers (may include an incognito browser).
+  size_t num_browsers = chrome::GetTotalBrowserCount();
+  content::TestNavigationObserver navigation_observer(expected_url);
+  navigation_observer.StartWatchingNewWebContents();
+  // If no navigation happens, then this test will time out due to the wait.
+  navigation_observer.Wait();
+
+  // There should be another browser window for the newly opened app.
+  EXPECT_EQ(num_browsers + 1, chrome::GetTotalBrowserCount());
+  // Help app should have opened at the expected page.
+  EXPECT_EQ(expected_url, chrome::FindLastActive()
+                              ->tab_strip_model()
+                              ->GetActiveWebContents()
+                              ->GetVisibleURL());
+}
+
 // Test that the Help App installs and launches correctly. Runs some spot
 // checks on the manifest.
 IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2) {
@@ -127,9 +146,20 @@ IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2InAppMetrics) {
 // Test that the Help App shortcut doesn't crash an incognito browser.
 IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2Incognito) {
   WaitForTestSystemAppInstall();
-  Browser* incognito_browser = CreateIncognitoBrowser();
-  EXPECT_NO_FATAL_FAILURE(
-      chrome::ShowHelp(incognito_browser, chrome::HELP_SOURCE_KEYBOARD));
+
+  chrome::ShowHelp(CreateIncognitoBrowser(), chrome::HELP_SOURCE_KEYBOARD);
+
+#if defined(OS_CHROMEOS) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
+  EXPECT_NO_FATAL_FAILURE(WaitForAppToOpen(GURL("chrome://help-app/")));
+#else
+  // We just have 2 browsers, the incognito and regular. Navigates chrome.
+  EXPECT_EQ(2u, chrome::GetTotalBrowserCount());
+  EXPECT_EQ(GURL(chrome::kChromeHelpViaKeyboardURL),
+            chrome::FindLastActive()
+                ->tab_strip_model()
+                ->GetActiveWebContents()
+                ->GetVisibleURL());
+#endif
 }
 
 // Test that launching the Help App's release notes opens the app on the Release
@@ -189,9 +219,15 @@ IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2ReleaseNotesMetrics) {
 IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2ReleaseNotesIncognito) {
   WaitForTestSystemAppInstall();
 
-  Browser* incognito_browser = CreateIncognitoBrowser();
-  EXPECT_NO_FATAL_FAILURE(chrome::LaunchReleaseNotes(
-      incognito_browser->profile(), apps::mojom::LaunchSource::kFromOtherApp));
+  chrome::LaunchReleaseNotes(CreateIncognitoBrowser()->profile(),
+                             apps::mojom::LaunchSource::kFromOtherApp);
+
+#if defined(OS_CHROMEOS) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
+  EXPECT_NO_FATAL_FAILURE(WaitForAppToOpen(GURL("chrome://help-app/updates")));
+#else
+  // We just have 2 browsers, the incognito and regular. No new app opens.
+  EXPECT_EQ(2u, chrome::GetTotalBrowserCount());
+#endif
 }
 
 // Test that the Help App does a navigation on launch even when it was already
@@ -268,20 +304,11 @@ IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2ShowParentalControls) {
 IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppOpenGestures) {
   WaitForTestSystemAppInstall();
   base::HistogramTester histogram_tester;
-  const GURL expected_url("chrome://help-app/help/sub/3399710/id/9739838");
-  content::TestNavigationObserver navigation_observer(expected_url);
-  navigation_observer.StartWatchingNewWebContents();
 
   SystemTrayClient::Get()->ShowGestureEducationHelp();
-  navigation_observer.Wait();
 
-  // There should be two browser windows, one regular and one for the help app.
-  EXPECT_EQ(2u, chrome::GetTotalBrowserCount());
-  // Help app should have opened at the gesture article.
-  EXPECT_EQ(expected_url, chrome::FindLastActive()
-                              ->tab_strip_model()
-                              ->GetActiveWebContents()
-                              ->GetVisibleURL());
+  EXPECT_NO_FATAL_FAILURE(
+      WaitForAppToOpen(GURL("chrome://help-app/help/sub/3399710/id/9739838")));
   // The HELP app is 18, see DefaultAppName in
   // src/chrome/browser/apps/app_service/app_service_metrics.cc
   histogram_tester.ExpectUniqueSample("Apps.DefaultAppLaunch.FromOtherApp", 18,
@@ -312,10 +339,47 @@ class HelpAppGuestSessionIntegrationTest : public HelpAppIntegrationTest {
 // Test that the Help App shortcut doesn't crash in guest mode.
 IN_PROC_BROWSER_TEST_P(HelpAppGuestSessionIntegrationTest, HelpAppShowHelp) {
   WaitForTestSystemAppInstall();
-  // TODO(carpenterr): Verify the right windows are launched in the chrome
-  // branded and non-chrome branded codepaths.
+
+  chrome::ShowHelp(browser(), chrome::HELP_SOURCE_KEYBOARD);
+
+#if defined(OS_CHROMEOS) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
+  EXPECT_NO_FATAL_FAILURE(WaitForAppToOpen(GURL("chrome://help-app/")));
+#else
+  // No new app should open on non-branded builds. Navigates chrome.
+  EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
+  EXPECT_EQ(GURL(chrome::kChromeHelpViaKeyboardURL),
+            chrome::FindLastActive()
+                ->tab_strip_model()
+                ->GetActiveWebContents()
+                ->GetVisibleURL());
+#endif
+}
+
+// Test that the Help App release notes entry point doesn't crash in guest mode.
+IN_PROC_BROWSER_TEST_P(HelpAppGuestSessionIntegrationTest,
+                       HelpAppLaunchReleaseNotes) {
+  WaitForTestSystemAppInstall();
+
+  chrome::LaunchReleaseNotes(profile(),
+                             apps::mojom::LaunchSource::kFromOtherApp);
+
+#if defined(OS_CHROMEOS) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
+  EXPECT_NO_FATAL_FAILURE(WaitForAppToOpen(GURL("chrome://help-app/updates")));
+#else
+  // Nothing should happen on non-branded builds.
+  EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
+#endif
+}
+
+// Test that Gesture help works in guest mode.
+IN_PROC_BROWSER_TEST_P(HelpAppGuestSessionIntegrationTest,
+                       HelpAppOpenGestures) {
+  WaitForTestSystemAppInstall();
+
+  SystemTrayClient::Get()->ShowGestureEducationHelp();
+
   EXPECT_NO_FATAL_FAILURE(
-      chrome::ShowHelp(browser(), chrome::HELP_SOURCE_KEYBOARD));
+      WaitForAppToOpen(GURL("chrome://help-app/help/sub/3399710/id/9739838")));
 }
 
 INSTANTIATE_TEST_SUITE_P(

chrome/browser/ui/ash/system_tray_client.cc

@@ -292,10 +292,8 @@ void SystemTrayClient::ShowGestureEducationHelp() {
   if (!profile)
     return;
 
-  // Note that AppServiceProxy is null for off-the-record profiles. For more
-  // context, see https://crbug.com/1112197.
-  apps::AppServiceProxy* proxy = apps::AppServiceProxyFactory::GetForProfile(
-      profile->GetOriginalProfile());
+  apps::AppServiceProxy* proxy =
+      apps::AppServiceProxyFactory::GetForProfileRedirectInIncognito(profile);
   proxy->LaunchAppWithUrl(
       chromeos::default_web_apps::kHelpAppId, ui::EventFlags::EF_NONE,
       GURL(chrome::kChromeOSGestureEducationHelpURL),

chrome/browser/ui/chrome_pages.cc

@@ -156,10 +156,8 @@ void LaunchReleaseNotesImpl(Profile* profile,
   base::RecordAction(UserMetricsAction("ReleaseNotes.ShowReleaseNotes"));
   // If the flag is enabled, launch the Help app and show the release notes.
   if (base::FeatureList::IsEnabled(chromeos::features::kHelpAppReleaseNotes)) {
-    // Note that AppServiceProxy is null for off-the-record profiles. For more
-    // context, see https://crbug.com/1112197.
-    apps::AppServiceProxy* proxy = apps::AppServiceProxyFactory::GetForProfile(
-        profile->GetOriginalProfile());
+    apps::AppServiceProxy* proxy =
+        apps::AppServiceProxyFactory::GetForProfileRedirectInIncognito(profile);
     proxy->LaunchAppWithUrl(
         chromeos::default_web_apps::kHelpAppId, ui::EventFlags::EF_NONE,
         GURL("chrome://help-app/updates"), source, display::kDefaultDisplayId);
@@ -212,14 +210,8 @@ void ShowHelpImpl(Browser* browser, Profile* profile, HelpSource source) {
     default:
       NOTREACHED() << "Unhandled help source" << source;
   }
-  // Use the original profile here, which is the same profile unless this is an
-  // OffTheRecord profile. The help app is not installed into the incognito /
-  // OffTheRecord profile.
-  if (profile->IsOffTheRecord() && !profile->IsGuestSession()) {
-    profile = profile->GetOriginalProfile();
-  }
   apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(profile);
+      apps::AppServiceProxyFactory::GetForProfileRedirectInIncognito(profile);
   proxy->Launch(chromeos::default_web_apps::kHelpAppId, ui::EventFlags::EF_NONE,
                 app_launch_source, display::kDefaultDisplayId);
 #else