Ensure that the Profile menu behaves appropriately in PWAs

When selecting a profile from the Profiles menu, it may be necessary
to load that profile. ExtensionAppShimHandler currently goes through
the hoops to do that in OnShimProcessConnected->OnProfileLoaded->
OnExtensionEnabled. Generalize that path into a LoadProfileAndApp
function that will make callback with a Profile* and Extension*.

Use the LoadProfileAndApp in both OnShimProcessConnected and
OnShimSelectedProfile.

When in multi-profile mode, no longer specify a valid Profile to
the AppShimHost. Clean up the places where the AppShimHost's profile
path was used, to avoid using the profile path.

Change the app shim to specify its full path (instead of a relative
path), and remove the GetFullProfilePath and ProfileExistsForPath
delegate functions.

And add some tests for all of this.

Bug: 982024
Change-Id: Icb1e7c688cea67758d9d0236856c8f6b612c1cd4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1825926
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700879}

chrome/app_shim/chrome_main_app_mode_mac.mm

@@ -155,7 +155,13 @@ int APP_SHIM_ENTRY_POINT_NAME(const app_mode::ChromeAppModeInfo* info) {
     // <user_data_dir>/<profile_dir>/Web Applications/_crx_extensionid/.
     controller_params.user_data_dir =
         base::FilePath(info->user_data_dir).DirName().DirName().DirName();
-    controller_params.profile_dir = base::FilePath(info->profile_dir);
+    // Similarly, extract the full profile path from |info->user_data_dir|.
+    // Ignore |info->profile_dir| because it is only the relative path (unless
+    // it is empty, in which case this is a profile-agnostic app).
+    if (!base::FilePath(info->profile_dir).empty()) {
+      controller_params.profile_dir =
+          base::FilePath(info->user_data_dir).DirName().DirName();
+    }
     controller_params.app_id = info->app_mode_id;
     controller_params.app_name = base::UTF8ToUTF16(info->app_mode_name);
     controller_params.app_url = GURL(info->app_mode_url);

chrome/browser/apps/app_shim/app_shim_host_mac.cc

@@ -157,6 +157,8 @@ void AppShimHost::ProfileSelectedFromMenu(const base::FilePath& profile_path) {
 }
 
 base::FilePath AppShimHost::GetProfilePath() const {
+  // This should only be used by single-profile-app paths.
+  DCHECK(!profile_path_.empty());
   return profile_path_;
 }
 

chrome/browser/apps/app_shim/app_shim_host_mac.h

@@ -92,7 +92,7 @@ class AppShimHost : public chrome::mojom::AppShimHost {
   // Return the app shim interface.
   chrome::mojom::AppShim* GetAppShim() const;
 
- private:
+ protected:
   void ChannelError(uint32_t custom_reason, const std::string& description);
 
   // Helper function to launch the app shim process.

chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h

@@ -54,42 +54,68 @@ class ExtensionAppShimHandler : public AppShimHostBootstrap::Client,
    public:
     virtual ~Delegate() {}
 
+    // Create an avatar menu (disable-able for tests).
     virtual std::unique_ptr<AvatarMenu> CreateAvatarMenu(
         AvatarMenuObserver* observer);
-    virtual base::FilePath GetFullProfilePath(
-        const base::FilePath& relative_path);
-    virtual bool ProfileExistsForPath(const base::FilePath& path);
+
+    // Return the profile for |path|, only if it is already loaded.
     virtual Profile* ProfileForPath(const base::FilePath& path);
+
+    // Load a profile and call |callback| when completed or failed.
     virtual void LoadProfileAsync(const base::FilePath& path,
                                   base::OnceCallback<void(Profile*)> callback);
+
+    // Return true if the specified path is for a valid profile that is also
+    // locked.
     virtual bool IsProfileLockedForPath(const base::FilePath& path);
 
+    // Return the app windows (not browser windows) for a legacy app.
     virtual extensions::AppWindowRegistry::AppWindowList GetWindows(
         Profile* profile,
         const std::string& extension_id);
 
+    // Look up an extension from its id.
     virtual const extensions::Extension* MaybeGetAppExtension(
         content::BrowserContext* context,
         const std::string& extension_id);
+
+    // Return true if the specified app should use an app shim (false, e.g, for
+    // bookmark apps that open in tabs).
     virtual bool AllowShimToConnect(Profile* profile,
                                     const extensions::Extension* extension);
+
+    // Create an AppShimHost for the specified parameters (intercept-able for
+    // tests).
     virtual std::unique_ptr<AppShimHost> CreateHost(
         AppShimHost::Client* client,
-        Profile* profile,
-        const extensions::Extension* extension);
+        const base::FilePath& profile_path,
+        const std::string& app_id,
+        bool use_remote_cocoa);
+
+    // Open a dialog to enable the specified extension. Call |callback| after
+    // the dialog is executed.
     virtual void EnableExtension(Profile* profile,
                                  const std::string& extension_id,
                                  base::OnceCallback<void()> callback);
+
+    // Launch the app in Chrome. This will (often) create a new window.
     virtual void LaunchApp(Profile* profile,
                            const extensions::Extension* extension,
                            const std::vector<base::FilePath>& files);
+
+    // Launch the shim process for an app.
     virtual void LaunchShim(Profile* profile,
                             const extensions::Extension* extension,
                             bool recreate_shims,
                             ShimLaunchedCallback launched_callback,
                             ShimTerminatedCallback terminated_callback);
+
+    // Launch the user manager (in response to attempting to access a locked
+    // profile).
     virtual void LaunchUserManager();
 
+    // Terminate Chrome if Chrome attempted to quit, but was prevented from
+    // quitting due to apps being open.
     virtual void MaybeTerminate();
   };
 
@@ -182,14 +208,31 @@ class ExtensionAppShimHandler : public AppShimHostBootstrap::Client,
   // Close one specified app.
   void CloseShimForApp(Profile* profile, const std::string& app_id);
 
-  // This is passed to Delegate::LoadProfileAsync for shim-initiated launches
-  // where the profile was not yet loaded.
-  void OnProfileLoaded(std::unique_ptr<AppShimHostBootstrap> bootstrap,
+  // Continuation of OnShimProcessConnected, once the profile has loaded.
+  void OnShimProcessConnectedAndAppLoaded(
+      std::unique_ptr<AppShimHostBootstrap> bootstrap,
+      Profile* profile,
+      const extensions::Extension* extension);
+
+  // Continuation of OnShimSelectedProfile, once the profile has loaded.
+  void OnShimSelectedProfileAndAppLoaded(
+      Profile* profile,
+      const extensions::Extension* extension);
+
+  // Load the specified profile and extension, and run |callback| with
+  // the result. The callback's arguments may be nullptr on failure.
+  using LoadProfileAppCallback =
+      base::OnceCallback<void(Profile*, const extensions::Extension*)>;
+  void LoadProfileAndApp(const base::FilePath& profile_path,
+                         const std::string& app_id,
+                         LoadProfileAppCallback callback);
+  void OnProfileLoaded(const base::FilePath& profile_path,
+                       const std::string& app_id,
+                       LoadProfileAppCallback callback,
                        Profile* profile);
-
-  // This is passed to Delegate::EnableViaPrompt for shim-initiated launches
-  // where the extension is disabled.
-  void OnExtensionEnabled(std::unique_ptr<AppShimHostBootstrap> bootstrap);
+  void OnAppEnabled(const base::FilePath& profile_path,
+                    const std::string& app_id,
+                    LoadProfileAppCallback callback);
 
   // Update the profiles menu for the specified host.
   void UpdateHostProfileMenu(AppShimHost* host);

chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc

@@ -52,7 +52,6 @@ class MockDelegate : public ExtensionAppShimHandler::Delegate {
   base::FilePath GetFullProfilePath(const base::FilePath& relative_path) {
     return relative_path;
   }
-  MOCK_METHOD1(ProfileExistsForPath, bool(const base::FilePath&));
   MOCK_METHOD1(ProfileForPath, Profile*(const base::FilePath&));
   void LoadProfileAsync(const base::FilePath& path,
                         base::OnceCallback<void(Profile*)> callback) override {
@@ -113,10 +112,10 @@ class MockDelegate : public ExtensionAppShimHandler::Delegate {
   void SetHostForCreate(std::unique_ptr<AppShimHost> host_for_create) {
     host_for_create_ = std::move(host_for_create);
   }
-  std::unique_ptr<AppShimHost> CreateHost(
-      AppShimHost::Client* client,
-      Profile* profile,
-      const extensions::Extension* extension) override {
+  std::unique_ptr<AppShimHost> CreateHost(AppShimHost::Client* client,
+                                          const base::FilePath& profile_path,
+                                          const std::string& app_id,
+                                          bool use_remote_cocoa) override {
     DCHECK(host_for_create_);
     std::unique_ptr<AppShimHost> result = std::move(host_for_create_);
     return result;
@@ -179,10 +178,12 @@ class TestingAppShimHostBootstrap : public AppShimHostBootstrap {
   TestingAppShimHostBootstrap(
       const base::FilePath& profile_path,
       const std::string& app_id,
+      bool is_from_bookmark,
       base::Optional<apps::AppShimLaunchResult>* launch_result)
       : AppShimHostBootstrap(getpid()),
         profile_path_(profile_path),
         app_id_(app_id),
+        is_from_bookmark_(is_from_bookmark),
         launch_result_(launch_result),
         weak_factory_(this) {}
   void DoTestLaunch(apps::AppShimLaunchType launch_type,
@@ -191,7 +192,8 @@ class TestingAppShimHostBootstrap : public AppShimHostBootstrap {
     auto app_shim_info = chrome::mojom::AppShimInfo::New();
     app_shim_info->profile_path = profile_path_;
     app_shim_info->app_id = app_id_;
-    app_shim_info->app_url = GURL("https://example.com");
+    if (is_from_bookmark_)
+      app_shim_info->app_url = GURL("https://example.com");
     app_shim_info->launch_type = launch_type;
     app_shim_info->files = files;
     OnShimConnected(
@@ -213,8 +215,9 @@ class TestingAppShimHostBootstrap : public AppShimHostBootstrap {
   }
 
  private:
-  base::FilePath profile_path_;
-  std::string app_id_;
+  const base::FilePath profile_path_;
+  const std::string app_id_;
+  const bool is_from_bookmark_;
   // Note that |launch_result_| is optional so that we can track whether or not
   // the callback to set it has arrived.
   base::Optional<apps::AppShimLaunchResult>* launch_result_;
@@ -250,6 +253,8 @@ class TestHost : public AppShimHost {
     return test_weak_factory_.GetWeakPtr();
   }
 
+  using AppShimHost::ProfileSelectedFromMenu;
+
  private:
   bool did_connect_to_host_ = false;
 
@@ -266,20 +271,25 @@ class ExtensionAppShimHandlerTest : public testing::Test {
         profile_path_b_("Profile B") {
     AppShimHostBootstrap::SetClient(handler_.get());
     bootstrap_aa_ = (new TestingAppShimHostBootstrap(
-                         profile_path_a_, kTestAppIdA, &bootstrap_aa_result_))
+                         profile_path_a_, kTestAppIdA,
+                         true /* is_from_bookmark */, &bootstrap_aa_result_))
                         ->GetWeakPtr();
     bootstrap_ab_ = (new TestingAppShimHostBootstrap(
-                         profile_path_a_, kTestAppIdB, &bootstrap_ab_result_))
+                         profile_path_a_, kTestAppIdB,
+                         false /* is_from_bookmark */, &bootstrap_ab_result_))
                         ->GetWeakPtr();
     bootstrap_bb_ = (new TestingAppShimHostBootstrap(
-                         profile_path_b_, kTestAppIdB, &bootstrap_bb_result_))
+                         profile_path_b_, kTestAppIdB,
+                         false /* is_from_bookmark */, &bootstrap_bb_result_))
                         ->GetWeakPtr();
     bootstrap_aa_duplicate_ =
         (new TestingAppShimHostBootstrap(profile_path_a_, kTestAppIdA,
+                                         true /* is_from_bookmark */,
                                          &bootstrap_aa_duplicate_result_))
             ->GetWeakPtr();
     bootstrap_aa_thethird_ =
         (new TestingAppShimHostBootstrap(profile_path_a_, kTestAppIdA,
+                                         true /* is_from_bookmark */,
                                          &bootstrap_aa_thethird_result_))
             ->GetWeakPtr();
 
@@ -314,14 +324,10 @@ class ExtensionAppShimHandlerTest : public testing::Test {
                        .SetID(kTestAppIdB)
                        .Build();
 
-    EXPECT_CALL(*delegate_, ProfileExistsForPath(profile_path_a_))
-        .WillRepeatedly(Return(true));
     EXPECT_CALL(*delegate_, IsProfileLockedForPath(profile_path_a_))
         .WillRepeatedly(Return(false));
     EXPECT_CALL(*delegate_, ProfileForPath(profile_path_a_))
         .WillRepeatedly(Return(&profile_a_));
-    EXPECT_CALL(*delegate_, ProfileExistsForPath(profile_path_b_))
-        .WillRepeatedly(Return(true));
     EXPECT_CALL(*delegate_, IsProfileLockedForPath(profile_path_b_))
         .WillRepeatedly(Return(false));
     EXPECT_CALL(*delegate_, ProfileForPath(profile_path_b_))
@@ -448,10 +454,10 @@ class ExtensionAppShimHandlerTest : public testing::Test {
 
 TEST_F(ExtensionAppShimHandlerTest, LaunchProfileNotFound) {
   // Bad profile path.
-  EXPECT_CALL(*delegate_, ProfileExistsForPath(profile_path_a_))
-      .WillOnce(Return(false))
-      .WillRepeatedly(Return(true));
+  EXPECT_CALL(*delegate_, ProfileForPath(profile_path_a_))
+      .WillRepeatedly(Return(static_cast<Profile*>(nullptr)));
   NormalLaunch(bootstrap_aa_, nullptr);
+  delegate_->RunLoadProfileCallback(profile_path_a_, nullptr);
   EXPECT_EQ(APP_SHIM_LAUNCH_PROFILE_NOT_FOUND, *bootstrap_aa_result_);
 }
 
@@ -821,4 +827,77 @@ TEST_F(ExtensionAppShimHandlerTest, MultiProfile) {
   }
 }
 
+TEST_F(ExtensionAppShimHandlerTest, MultiProfileShimLaunch) {
+  base::test::ScopedFeatureList scoped_features;
+  scoped_features.InitWithFeatures(
+      /*enabled_features=*/{features::kAppShimMultiProfile},
+      /*disabled_features=*/{});
+
+  delegate_->SetHostForCreate(std::move(host_aa_unique_));
+  ShimLaunchedCallback launched_callback;
+  delegate_->SetCaptureShimLaunchedCallback(&launched_callback);
+  ShimTerminatedCallback terminated_callback;
+  delegate_->SetCaptureShimTerminatedCallback(&terminated_callback);
+
+  // Launch the app for profile A. This should trigger a shim launch request.
+  EXPECT_CALL(*delegate_, DoLaunchShim(&profile_a_, extension_a_.get(),
+                                       false /* recreate_shim */));
+  EXPECT_EQ(nullptr, handler_->FindHost(&profile_a_, kTestAppIdA));
+  handler_->OnAppActivated(&profile_a_, kTestAppIdA);
+  EXPECT_EQ(host_aa_.get(), handler_->FindHost(&profile_a_, kTestAppIdA));
+  EXPECT_FALSE(host_aa_->did_connect_to_host());
+
+  // Launch the app for profile B. This should not cause a shim launch request.
+  EXPECT_CALL(*delegate_, DoLaunchShim(_, _, _)).Times(0);
+  handler_->OnAppActivated(&profile_b_, kTestAppIdA);
+
+  // Indicate the profile A that its launch succeeded.
+  EXPECT_TRUE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+  std::move(launched_callback).Run(base::Process(5));
+  EXPECT_FALSE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+}
+
+TEST_F(ExtensionAppShimHandlerTest, MultiProfileSelectMenu) {
+  base::test::ScopedFeatureList scoped_features;
+  scoped_features.InitWithFeatures(
+      /*enabled_features=*/{features::kAppShimMultiProfile},
+      /*disabled_features=*/{});
+
+  delegate_->SetHostForCreate(std::move(host_aa_unique_));
+  ShimLaunchedCallback launched_callback;
+  delegate_->SetCaptureShimLaunchedCallback(&launched_callback);
+  ShimTerminatedCallback terminated_callback;
+  delegate_->SetCaptureShimTerminatedCallback(&terminated_callback);
+
+  // Launch the app for profile A. This should trigger a shim launch request.
+  EXPECT_CALL(*delegate_, DoLaunchShim(&profile_a_, extension_a_.get(),
+                                       false /* recreate_shim */));
+  EXPECT_EQ(nullptr, handler_->FindHost(&profile_a_, kTestAppIdA));
+  handler_->OnAppActivated(&profile_a_, kTestAppIdA);
+  EXPECT_EQ(host_aa_.get(), handler_->FindHost(&profile_a_, kTestAppIdA));
+  EXPECT_FALSE(host_aa_->did_connect_to_host());
+
+  // Indicate the profile A that its launch succeeded.
+  EXPECT_TRUE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+  std::move(launched_callback).Run(base::Process(5));
+  EXPECT_FALSE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+
+  // Select profile B from the menu. This should request that the app be
+  // launched.
+  EXPECT_CALL(*delegate_, LaunchApp(&profile_b_, extension_a_.get(), _));
+  host_aa_->ProfileSelectedFromMenu(profile_path_b_);
+  EXPECT_CALL(*delegate_, DoLaunchShim(_, _, _)).Times(0);
+  handler_->OnAppActivated(&profile_b_, kTestAppIdA);
+
+  // Select profile A and B from the menu -- this should not request a launch,
+  // because the profiles are already enabled.
+  EXPECT_CALL(*delegate_, LaunchApp(_, _, _)).Times(0);
+  host_aa_->ProfileSelectedFromMenu(profile_path_a_);
+  host_aa_->ProfileSelectedFromMenu(profile_path_b_);
+}
+
 }  // namespace apps