MacPWAs: Allow creating of AppShimHost before process exists

Allow for an AppShimHost to be created before the corresponding
AppShimHostBootstrap. Update OnAppLaunchComplete to save the launch
result and send it only once an AppShimHostBootstrap is attached.

Change ExtensionAppShimHandler profile paths to use the full path
(as is reported by the Profile object, as opposed to the shim's
message).

Change the method GetHostForBrowser to instead be
GetViewsBridgeFactoryHostForBrowser, because the ViewsBridgeFactory is
what all callers are after. Make this call trigger creation of an
AppShimHost if needed to provide a ViewsBridgeFactory. Keep this
guarded behind a flag for now.

Add tests for this functionality. In the tests, further decouple
the AppShimHostBootstrap from its AppShimHost.

Bug: 896917
Change-Id: Ib9e4ad6535ca87cd0e01303fbfc772b8892aec12
Reviewed-on: https://chromium-review.googlesource.com/c/1309307Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604440}

chrome/browser/apps/app_shim/app_shim_handler_mac.h

@@ -29,6 +29,9 @@ class AppShimHandler {
   // https://crbug.com/896917
   class Host {
    public:
+    // Returns true if an AppShimHostBootstrap has already connected to this
+    // host.
+    virtual bool HasBootstrapConnected() const = 0;
     // Invoked when the app shim process has finished launching. The |bootstrap|
     // object owns the lifetime of the app shim process.
     virtual void OnBootstrapConnected(

chrome/browser/apps/app_shim/app_shim_host_mac.cc

@@ -71,6 +71,17 @@ void AppShimHost::ChannelError(uint32_t custom_reason,
   Close();
 }
 
+void AppShimHost::SendLaunchResult() {
+  DCHECK(!has_sent_on_launch_complete_);
+  DCHECK(bootstrap_);
+  DCHECK(launch_result_);
+  if (*launch_result_ == apps::APP_SHIM_LAUNCH_SUCCESS)
+    bootstrap_->OnLaunchAppSucceeded(std::move(app_shim_request_));
+  else
+    bootstrap_->OnLaunchAppFailed(*launch_result_);
+  has_sent_on_launch_complete_ = true;
+}
+
 void AppShimHost::Close() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   // Note that we must call GetAppShimHandler here and not in the destructor
@@ -88,6 +99,10 @@ apps::AppShimHandler* AppShimHost::GetAppShimHandler() const {
 ////////////////////////////////////////////////////////////////////////////////
 // AppShimHost, chrome::mojom::AppShimHost
 
+bool AppShimHost::HasBootstrapConnected() const {
+  return bootstrap_ != nullptr;
+}
+
 void AppShimHost::OnBootstrapConnected(
     std::unique_ptr<AppShimHostBootstrap> bootstrap) {
   DCHECK(!bootstrap_);
@@ -97,6 +112,11 @@ void AppShimHost::OnBootstrapConnected(
   host_binding_.Bind(bootstrap_->GetLaunchAppShimHostRequest());
   host_binding_.set_connection_error_with_reason_handler(
       base::BindOnce(&AppShimHost::ChannelError, base::Unretained(this)));
+
+  // If we already have a launch result ready (e.g, because we launched the app
+  // from Chrome), send the result immediately.
+  if (launch_result_)
+    SendLaunchResult();
 }
 
 void AppShimHost::FocusApp(apps::AppShimFocusType focus_type,
@@ -126,12 +146,9 @@ void AppShimHost::QuitApp() {
 
 void AppShimHost::OnAppLaunchComplete(apps::AppShimLaunchResult result) {
   DCHECK(!has_sent_on_launch_complete_);
-  DCHECK(bootstrap_);
-  if (result == apps::APP_SHIM_LAUNCH_SUCCESS)
-    bootstrap_->OnLaunchAppSucceeded(std::move(app_shim_request_));
-  else
-    bootstrap_->OnLaunchAppFailed(result);
-  has_sent_on_launch_complete_ = true;
+  launch_result_.emplace(result);
+  if (bootstrap_)
+    SendLaunchResult();
 }
 
 void AppShimHost::OnAppClosed() {

chrome/browser/apps/app_shim/app_shim_host_mac.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/files/file_path.h"
+#include "base/optional.h"
 #include "base/threading/thread_checker.h"
 #include "chrome/browser/apps/app_shim/app_shim_handler_mac.h"
 #include "chrome/common/mac/app_shim.mojom.h"
@@ -35,6 +36,7 @@ class AppShimHost : public chrome::mojom::AppShimHost,
   AppShimHost(const std::string& app_id, const base::FilePath& profile_path);
 
   // apps::AppShimHandler::Host overrides:
+  bool HasBootstrapConnected() const override;
   void OnBootstrapConnected(
       std::unique_ptr<AppShimHostBootstrap> bootstrap) override;
   void OnAppLaunchComplete(apps::AppShimLaunchResult result) override;
@@ -51,6 +53,7 @@ class AppShimHost : public chrome::mojom::AppShimHost,
   // channel error and OnAppClosed).
   ~AppShimHost() override;
   void ChannelError(uint32_t custom_reason, const std::string& description);
+  void SendLaunchResult();
 
   // Closes the channel and destroys the AppShimHost.
   void Close();
@@ -75,6 +78,10 @@ class AppShimHost : public chrome::mojom::AppShimHost,
 
   std::string app_id_;
   base::FilePath profile_path_;
+
+  // The result passed to OnAppLaunchComplete, not set until OnAppLaunchComplete
+  // is called.
+  base::Optional<apps::AppShimLaunchResult> launch_result_;
   bool has_sent_on_launch_complete_ = false;
 
   THREAD_CHECKER(thread_checker_);

chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc

@@ -151,21 +151,25 @@ class EnableViaPrompt : public ExtensionEnableFlowDelegate {
 
 namespace apps {
 
+base::FilePath ExtensionAppShimHandler::Delegate::GetFullProfilePath(
+    const base::FilePath& relative_profile_path) {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  return profile_manager->user_data_dir().Append(relative_profile_path);
+}
+
 bool ExtensionAppShimHandler::Delegate::ProfileExistsForPath(
-    const base::FilePath& path) {
+    const base::FilePath& full_path) {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   // Check for the profile name in the profile info cache to ensure that we
   // never access any directory that isn't a known profile.
-  base::FilePath full_path = profile_manager->user_data_dir().Append(path);
   ProfileAttributesEntry* entry;
   return profile_manager->GetProfileAttributesStorage().
       GetProfileAttributesWithPath(full_path, &entry);
 }
 
 Profile* ExtensionAppShimHandler::Delegate::ProfileForPath(
-    const base::FilePath& path) {
+    const base::FilePath& full_path) {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
-  base::FilePath full_path = profile_manager->user_data_dir().Append(path);
   Profile* profile = profile_manager->GetProfileByPath(full_path);
 
   // Use IsValidProfile to check if the profile has been created.
@@ -173,17 +177,14 @@ Profile* ExtensionAppShimHandler::Delegate::ProfileForPath(
 }
 
 void ExtensionAppShimHandler::Delegate::LoadProfileAsync(
-    const base::FilePath& path,
+    const base::FilePath& full_path,
     base::OnceCallback<void(Profile*)> callback) {
   ProfileManager* profile_manager = g_browser_process->profile_manager();
-  base::FilePath full_path = profile_manager->user_data_dir().Append(path);
   profile_manager->LoadProfileByPath(full_path, false, std::move(callback));
 }
 
 bool ExtensionAppShimHandler::Delegate::IsProfileLockedForPath(
-    const base::FilePath& path) {
-  ProfileManager* profile_manager = g_browser_process->profile_manager();
-  base::FilePath full_path = profile_manager->user_data_dir().Append(path);
+    const base::FilePath& full_path) {
   return profiles::IsProfileLocked(full_path);
 }
 
@@ -277,13 +278,26 @@ AppShimHandler::Host* ExtensionAppShimHandler::FindHost(
   return it == hosts_.end() ? NULL : it->second;
 }
 
-AppShimHandler::Host* ExtensionAppShimHandler::FindHostForBrowser(
-    Browser* browser) {
+AppShimHandler::Host* ExtensionAppShimHandler::FindOrCreateHost(
+    Profile* profile,
+    const std::string& app_id) {
+  Host*& host = hosts_[make_pair(profile, app_id)];
+  if (!host)
+    host = delegate_->CreateHost(app_id, profile->GetPath());
+  return host;
+}
+
+views::BridgeFactoryHost*
+ExtensionAppShimHandler::GetViewsBridgeFactoryHostForBrowser(Browser* browser) {
+  if (!features::HostWindowsInAppShimProcess())
+    return nullptr;
+
   const Extension* extension =
       apps::ExtensionAppShimHandler::MaybeGetAppForBrowser(browser);
-  if (extension) {
-    return FindHost(Profile::FromBrowserContext(browser->profile()),
-                    extension->id());
+  if (extension && extension->is_hosted_app()) {
+    Host* host = FindOrCreateHost(
+        Profile::FromBrowserContext(browser->profile()), extension->id());
+    return host->GetViewsBridgeFactoryHost();
   }
   return nullptr;
 }
@@ -417,8 +431,10 @@ void ExtensionAppShimHandler::OnShimLaunch(
   AppShimLaunchType launch_type = bootstrap->GetLaunchType();
   DCHECK(crx_file::id_util::IdIsValid(app_id));
 
-  const base::FilePath& profile_path = bootstrap->GetProfilePath();
-  DCHECK(!profile_path.empty());
+  const base::FilePath& relative_profile_path = bootstrap->GetProfilePath();
+  DCHECK(!relative_profile_path.empty());
+  base::FilePath profile_path =
+      delegate_->GetFullProfilePath(relative_profile_path);
 
   if (!delegate_->ProfileExistsForPath(profile_path)) {
     // User may have deleted the profile this shim was originally created for.
@@ -499,12 +515,11 @@ void ExtensionAppShimHandler::OnProfileLoaded(
   AppShimLaunchType launch_type = bootstrap->GetLaunchType();
   const std::vector<base::FilePath>& files = bootstrap->GetLaunchFiles();
 
-  // The first host to claim this (profile, app_id) becomes the main host.
-  // For any others, focus or relaunch the app.
-  Host*& host = hosts_[make_pair(profile, app_id)];
-  if (!host) {
-    host = delegate_->CreateHost(app_id, bootstrap->GetProfilePath());
-  } else {
+  Host* host = FindOrCreateHost(profile, app_id);
+  if (host->HasBootstrapConnected()) {
+    // If another app shim process has already connected to this (profile,
+    // app_id) pair, then focus the windows for the existing process, and
+    // close the new process.
     OnShimFocus(host,
                 launch_type == APP_SHIM_LAUNCH_NORMAL ?
                     APP_SHIM_FOCUS_REOPEN : APP_SHIM_FOCUS_NORMAL,
@@ -713,13 +728,15 @@ void ExtensionAppShimHandler::OnAppActivated(content::BrowserContext* context,
 
   Profile* profile = static_cast<Profile*>(context);
   Host* host = FindHost(profile, app_id);
-  if (host) {
+  if (host && host->HasBootstrapConnected()) {
+    // If there is a connected app shim process, notify it of success and focus
+    // the app windows.
     host->OnAppLaunchComplete(APP_SHIM_LAUNCH_SUCCESS);
     OnShimFocus(host, APP_SHIM_FOCUS_NORMAL, std::vector<base::FilePath>());
-    return;
+  } else {
+    // Otherwise, launch an app shim.
+    delegate_->LaunchShim(profile, extension);
   }
-
-  delegate_->LaunchShim(profile, extension);
 }
 
 void ExtensionAppShimHandler::OnAppDeactivated(content::BrowserContext* context,

chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h

@@ -26,16 +26,20 @@ class Profile;
 
 namespace base {
 class FilePath;
-}
+}  // namespace base
 
 namespace content {
 class BrowserContext;
-}
+}  // namespace content
 
 namespace extensions {
 class AppWindow;
 class Extension;
-}
+}  // namespace extensions
+
+namespace views {
+class BridgeFactoryHost;
+}  // namespace views
 
 namespace apps {
 
@@ -50,6 +54,8 @@ class ExtensionAppShimHandler : public AppShimHandler,
    public:
     virtual ~Delegate() {}
 
+    virtual base::FilePath GetFullProfilePath(
+        const base::FilePath& relative_path);
     virtual bool ProfileExistsForPath(const base::FilePath& path);
     virtual Profile* ProfileForPath(const base::FilePath& path);
     virtual void LoadProfileAsync(const base::FilePath& path,
@@ -90,9 +96,15 @@ class ExtensionAppShimHandler : public AppShimHandler,
   virtual AppShimHandler::Host* FindHost(Profile* profile,
                                          const std::string& app_id);
 
-  // Get the host corresponding to a browser instance, or nullptr if none
-  // exists.
-  AppShimHandler::Host* FindHostForBrowser(Browser* browser);
+  // Return the host corresponding to |profile| and |app_id|, or create one if
+  // needed.
+  AppShimHandler::Host* FindOrCreateHost(Profile* profile,
+                                         const std::string& app_id);
+
+  // Get the ViewBridgeFactoryHost, which may be used to create remote
+  // NSWindows, corresponding to a browser instance (or nullptr if none exists).
+  views::BridgeFactoryHost* GetViewsBridgeFactoryHostForBrowser(
+      Browser* browser);
 
   void SetHostedAppHidden(Profile* profile,
                           const std::string& app_id,

chrome/browser/ui/views/frame/browser_frame_mac.mm

@@ -190,13 +190,10 @@ NativeWidgetMacNSWindow* BrowserFrameMac::CreateNSWindow(
 
 views::BridgeFactoryHost* BrowserFrameMac::GetBridgeFactoryHost() {
   auto* shim_handler = apps::ExtensionAppShimHandler::Get();
-  if (shim_handler) {
-    apps::AppShimHandler::Host* host =
-        shim_handler->FindHostForBrowser(browser_view_->browser());
-    if (host)
-      return host->GetViewsBridgeFactoryHost();
-  }
-  return nullptr;
+  if (!shim_handler)
+    return nullptr;
+  return shim_handler->GetViewsBridgeFactoryHostForBrowser(
+      browser_view_->browser());
 }
 
 void BrowserFrameMac::OnWindowDestroying(gfx::NativeWindow native_window) {