Reland "system-web-apps: better handle incognito in LaunchSystemWebApp"

This is a reland of de6bbcbc

The reland deletes EXPECT_DCHECK_DEATH to all launch tests, because
death tests aren't reliable in browsertest. And manually test
LaunchSystemWebApp behaves correctly in debug / DCHECK builds.

Test=Manually verified debug build and release build behaves correctly.
- When DCHECK is on: DCHECKs when launching with a wrong profile
- When DCHECK is off: launches into an appropriate profile

Original change's description:
> system-web-apps: better handle incognito in LaunchSystemWebApp
>
> LaunchSystemWebApp used whatever profile is provided in the parameter to
> create SWA windows and launching SWAs, even if the profile may not
> suitable to host a SWA window.
>
> This CL adds logic to determine the appropriate profile if the provided
> one is unsuitable. This serves as a mid-term solution for launching
> SWAs, and acts as a safety net to prevent chrome crashing if unsuitable
> profiles are provided.
>
> Specifically, it catches these two types of profiles:
> - Guest session "original" profile: launch into guest session's
>   primary off the record profile (which is used for guest session
>   browsing and SWAs).
> - Normal session incognito profile: launch into the original profile,
>   because we don't support incognito SWAs.
>
> Fixed: 1109594
> Change-Id: Ibe4a3b1bf7c811ef1ee02fd967307ad9bf070ee7
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2423760
> Commit-Queue: Jiewei Qian  <qjw@chromium.org>
> Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#814602}

Change-Id: I5766a26c5376c7e59180e4d9dd2befe015c13e9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2459010
Commit-Queue: Jiewei Qian  <qjw@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816104}

chrome/browser/ui/web_applications/system_web_app_ui_utils.cc

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/check_op.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/metrics/histogram_functions.h"
@@ -18,6 +19,7 @@
 #include "chrome/browser/apps/app_service/app_service_metrics.h"
 #include "chrome/browser/apps/app_service/launch_utils.h"
 #include "chrome/browser/chromeos/printing/print_management/print_management_uma.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/installable/installable_params.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_list.h"
@@ -35,10 +37,45 @@
 #include "chrome/browser/web_launch/web_launch_files_helper.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
+#include "content/public/common/content_switches.h"
 #include "third_party/blink/public/common/features.h"
 #include "ui/base/window_open_disposition.h"
 #include "ui/display/types/display_constants.h"
 
+namespace {
+// Returns the profile where we should launch System Web Apps into. It returns
+// the most appropriate profile for launching, if the provided |profile| is
+// unsuitable. It returns nullptr if the we can't find a suitable profile.
+Profile* GetProfileForSystemWebAppLaunch(Profile* profile) {
+  DCHECK(profile);
+
+  // We can't launch into certain profiles, and we can't find a suitable
+  // alternative.
+  if (profile->IsSystemProfile())
+    return nullptr;
+#if defined(OS_CHROMEOS)
+  if (chromeos::ProfileHelper::IsSigninProfile(profile))
+    return nullptr;
+#endif
+
+  // For a guest sessions, launch into the primary off-the-record profile, which
+  // is used for browsing in guest sessions. We do this because the "original"
+  // profile of the guest session can't create windows.
+  if (profile->IsGuestSession())
+    return profile->GetPrimaryOTRProfile();
+
+  // We don't support launching SWA in incognito profiles, use the original
+  // profile if an incognito profile is provided (with the exception of guest
+  // session, which is implemented with an incognito profile, thus it is handled
+  // above).
+  if (profile->IsIncognitoProfile())
+    return profile->GetOriginalProfile();
+
+  // Use the profile provided in other scenarios.
+  return profile;
+}
+}  // namespace
+
 namespace web_app {
 namespace {
 
@@ -122,20 +159,42 @@ Browser* LaunchSystemWebApp(Profile* profile,
                             const GURL& url,
                             base::Optional<apps::AppLaunchParams> params,
                             bool* did_create) {
-  auto* provider = WebAppProvider::Get(profile);
+  Profile* profile_for_launch = GetProfileForSystemWebAppLaunch(profile);
+  if (profile_for_launch == nullptr || profile_for_launch != profile) {
+    // The provided profile can't launch system web apps. Complain about this so
+    // we can catch the call site, and ask them to pick the right profile.
+    base::debug::DumpWithoutCrashing();
+
+    DVLOG(1) << "LaunchSystemWebApp is called on a profile that can't launch "
+                "system  web apps. Please check the profile you are using is "
+                "correct."
+             << (profile_for_launch
+                     ? "Instead, launch the app into a suitable profile "
+                       "based on your intention."
+                     : "Can't find a suitable profile based on the provided "
+                       "argument. Thus ignore the launch request.");
+
+    NOTREACHED();
+
+    if (profile_for_launch == nullptr)
+      return nullptr;
+  }
+
+  auto* provider = WebAppProvider::Get(profile_for_launch);
 
   if (!provider)
     return nullptr;
 
   if (!params) {
-    params = CreateSystemWebAppLaunchParams(profile, app_type,
+    params = CreateSystemWebAppLaunchParams(profile_for_launch, app_type,
                                             display::kInvalidDisplayId);
   }
   if (!params)
     return nullptr;
   params->override_url = url;
 
-  DCHECK_EQ(params->app_id, *GetAppIdForSystemWebApp(profile, app_type));
+  DCHECK_EQ(params->app_id,
+            *GetAppIdForSystemWebApp(profile_for_launch, app_type));
 
   // TODO(crbug/1117655): The file manager records metrics directly when opening
   // a file registered to an app, but can't tell if an SWA will ultimately be
@@ -159,7 +218,8 @@ Browser* LaunchSystemWebApp(Profile* profile,
     browser_type = Browser::TYPE_APP_POPUP;
   if (browser_type == Browser::TYPE_APP_POPUP ||
       provider->system_web_app_manager().IsSingleWindow(app_type)) {
-    browser = FindSystemWebAppBrowser(profile, app_type, browser_type);
+    browser =
+        FindSystemWebAppBrowser(profile_for_launch, app_type, browser_type);
   }
 
   // We create the app window if no existing browser found.
@@ -172,9 +232,10 @@ Browser* LaunchSystemWebApp(Profile* profile,
   // CCA supports responsive UI.
   bool can_resize = app_type != SystemAppType::CAMERA;
   if (base::FeatureList::IsEnabled(features::kDesktopPWAsWithoutExtensions)) {
-    if (!browser)
-      browser = CreateWebApplicationWindow(profile, params->app_id,
+    if (!browser) {
+      browser = CreateWebApplicationWindow(profile_for_launch, params->app_id,
                                            params->disposition, can_resize);
+    }
 
     // Navigate application window to application's |url| if necessary.
     // Help app always navigates because its url might not match the url inside
@@ -186,8 +247,10 @@ Browser* LaunchSystemWebApp(Profile* profile,
           browser, params->app_id, url, WindowOpenDisposition::CURRENT_TAB);
     }
   } else {
-    if (!browser)
-      browser = CreateApplicationWindow(profile, *params, url, can_resize);
+    if (!browser) {
+      browser =
+          CreateApplicationWindow(profile_for_launch, *params, url, can_resize);
+    }
 
     // Navigate application window to application's |url| if necessary.
     // Help app always navigates because its url might not match the url inside

chrome/browser/ui/web_applications/test/system_web_app_ui_browsertest.cc

@@ -8,7 +8,9 @@
 #include <vector>
 
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/gtest_util.h"
 #include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
@@ -392,11 +394,19 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppLinkCaptureBrowserTest,
                           ->GetLastCommittedURL());
 }
 
-// TODO(crbug.com/1109594): Update this when we decide if SWAs can be
-// link-captured to standalone incognito windows. Currently, the SWA gets
-// launched into a standalone browser window in incognito profile.
+// TODO(crbug.com/1135863): Decide and formalize this behavior. This test is
+// disabled in DCHECK builds, because it hits a DCHECK in LaunchSystemWebApp. In
+// production builds, SWA is link captured to the original profile. The goal is
+// to behave reasonably, and not crashing.
+#if DCHECK_IS_ON()
+#define MAYBE_IncognitoBrowserOmniboxLinkCapture \
+  DISABLED_IncognitoBrowserOmniboxLinkCapture
+#else
+#define MAYBE_IncognitoBrowserOmniboxLinkCapture \
+  IncognitoBrowserOmniboxLinkCapture
+#endif
 IN_PROC_BROWSER_TEST_P(SystemWebAppLinkCaptureBrowserTest,
-                       IncognitoBrowserOmniboxLinkCapture) {
+                       MAYBE_IncognitoBrowserOmniboxLinkCapture) {
   WaitForTestSystemAppInstall();
 
   Browser* incognito_browser = CreateIncognitoBrowser();
@@ -410,8 +420,10 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppLinkCaptureBrowserTest,
       incognito_browser, maybe_installation_->GetAppUrl().spec());
   observer.Wait();
 
+  // We launch SWAs into the incognito profile's original profile.
   Browser* app_browser = FindSystemWebAppBrowser(
-      incognito_browser->profile(), maybe_installation_->GetType());
+      incognito_browser->profile()->GetOriginalProfile(),
+      maybe_installation_->GetType());
   EXPECT_TRUE(app_browser);
   EXPECT_EQ(app_browser, chrome::FindLastActive());
   EXPECT_EQ(2U, chrome::GetTotalBrowserCount());
@@ -419,7 +431,94 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppLinkCaptureBrowserTest,
   EXPECT_FALSE(app_browser->app_controller()->ShouldShowCustomTabBar());
 }
 
+// The following tests are disabled in DCHECK builds. LaunchSystemWebApp DCHECKs
+// if the wrong profile is used. EXPECT_DCHECK_DEATH (or its variants) aren't
+// reliable in browsertests, so we don't test this. This is okay because these
+// tests are used to verify that in release builds, LaunchSystemWebApp doesn't
+// crash and behaves reasonably (pick an appropriate profile).
+#if defined(OS_CHROMEOS) && !DCHECK_IS_ON()
+using SystemWebAppLaunchProfileBrowserTest = SystemWebAppManagerBrowserTest;
+
+IN_PROC_BROWSER_TEST_P(SystemWebAppLaunchProfileBrowserTest,
+                       LaunchFromNormalSessionIncognitoProfile) {
+  Profile* startup_profile = browser()->profile();
+  ASSERT_TRUE(startup_profile->IsRegularProfile());
+
+  WaitForTestSystemAppInstall();
+  Profile* incognito_profile = startup_profile->GetPrimaryOTRProfile();
+
+  Browser* result_browser =
+      LaunchSystemWebApp(incognito_profile, GetMockAppType(),
+                         GetStartUrl(LaunchParamsForApp(GetMockAppType())));
+  EXPECT_EQ(startup_profile, result_browser->profile());
+  EXPECT_TRUE(result_browser->profile()->IsRegularProfile());
+}
+
+IN_PROC_BROWSER_TEST_P(SystemWebAppLaunchProfileBrowserTest,
+                       LaunchFromSignInProfile) {
+  WaitForTestSystemAppInstall();
+
+  Profile* signin_profile = chromeos::ProfileHelper::GetSigninProfile();
+
+  Browser* result_browser =
+      LaunchSystemWebApp(signin_profile, GetMockAppType(),
+                         GetStartUrl(LaunchParamsForApp(GetMockAppType())));
+  EXPECT_EQ(nullptr, result_browser);
+}
+
+using SystemWebAppLaunchProfileGuestSessionBrowserTest =
+    SystemWebAppLaunchProfileBrowserTest;
+
+IN_PROC_BROWSER_TEST_P(SystemWebAppLaunchProfileGuestSessionBrowserTest,
+                       LaunchFromGuestSessionOriginalProfile) {
+  // We should start into the guest session browsing profile.
+  Profile* startup_profile = browser()->profile();
+  ASSERT_TRUE(startup_profile->IsGuestSession());
+  ASSERT_TRUE(startup_profile->IsPrimaryOTRProfile());
+
+  WaitForTestSystemAppInstall();
+
+  // We typically don't get the original profile as an argument, but it is a
+  // valid input to LaunchSystemWebApp.
+  Profile* original_profile = browser()->profile()->GetOriginalProfile();
+
+  Browser* result_browser =
+      LaunchSystemWebApp(original_profile, GetMockAppType(),
+                         GetStartUrl(LaunchParamsForApp(GetMockAppType())));
+
+  EXPECT_EQ(startup_profile, result_browser->profile());
+  EXPECT_TRUE(result_browser->profile()->IsGuestSession());
+  EXPECT_TRUE(result_browser->profile()->IsPrimaryOTRProfile());
+}
+
+IN_PROC_BROWSER_TEST_P(SystemWebAppLaunchProfileGuestSessionBrowserTest,
+                       LaunchFromGuestSessionPrimaryOTRProfile) {
+  // We should start into the guest session browsing profile.
+  Profile* startup_profile = browser()->profile();
+  ASSERT_TRUE(startup_profile->IsGuestSession());
+  ASSERT_TRUE(startup_profile->IsPrimaryOTRProfile());
+
+  WaitForTestSystemAppInstall();
+
+  Browser* result_browser =
+      LaunchSystemWebApp(startup_profile, GetMockAppType(),
+                         GetStartUrl(LaunchParamsForApp(GetMockAppType())));
+
+  EXPECT_EQ(startup_profile, result_browser->profile());
+  EXPECT_TRUE(result_browser->profile()->IsGuestSession());
+  EXPECT_TRUE(result_browser->profile()->IsPrimaryOTRProfile());
+}
+#endif  // defined(OS_CHROMEOS) && !DCHECK_IS_ON()
+
 INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_ALL_INSTALL_TYPES_P(
     SystemWebAppLinkCaptureBrowserTest);
 
+#if defined(OS_CHROMEOS) && !DCHECK_IS_ON()
+INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_ALL_INSTALL_TYPES_P(
+    SystemWebAppLaunchProfileBrowserTest);
+
+INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_GUEST_SESSION_P(
+    SystemWebAppLaunchProfileGuestSessionBrowserTest);
+#endif
+
 }  // namespace web_app

chrome/browser/web_applications/system_web_app_manager_browsertest.h

@@ -173,4 +173,13 @@ std::string SystemWebAppManagerTestParamsToString(
                                    TestProfileType::kIncognito,              \
                                    TestProfileType::kGuest)))
 
+// Instantiate |SUITE| for all install types, on a Chrome OS guest session.
+#define INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_GUEST_SESSION_P(SUITE) \
+  INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_P(                           \
+      SUITE,                                                                 \
+      ::testing::Combine(                                                    \
+          ::testing::Values(web_app::InstallationType::kManifestInstall,     \
+                            web_app::InstallationType::kWebAppInfoInstall),  \
+          ::testing::Values(TestProfileType::kGuest)))
+
 #endif  // CHROME_BROWSER_WEB_APPLICATIONS_SYSTEM_WEB_APP_MANAGER_BROWSERTEST_H_