Launch app shim for notifications if necessary.

Currently (when notification attribution is enabled), notification attribution only works when the corresponding app shims are already running. This CL adds support to launch app shims as needed. When launched for notifications the app shim will be launched silently, invisible to the user. Also refactors some of the app shim launching code a bit, passing around enums rather than booleans, and changing the LaunchShimUpdateBehavior enum from UPPER_CASE to kCamelCase. Bug: 938661 Change-Id: Iab4e421a362da12a4a22ca8d9cdd21158a01ff02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4708345 Reviewed-by: Trent Apted <tapted@chromium.org> Commit-Queue: Marijn Kruisselbrink <mek@chromium.org> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/main@{#1186654}
chromium · Aug 22, 2023 · e340c74 · e340c74
1 parent 9b42945
commit e340c74
Show file tree

Hide file tree

Showing 13 changed files with 278 additions and 99 deletions.
diff --git a/chrome/browser/apps/app_shim/app_shim_host_mac.cc b/chrome/browser/apps/app_shim/app_shim_host_mac.cc
@@ -62,20 +62,26 @@ void AppShimHost::ChannelError(uint32_t custom_reason,
   client_->OnShimProcessDisconnected(this);
 }
 
-void AppShimHost::LaunchShimInternal(bool recreate_shims) {
+void AppShimHost::LaunchShimInternal(
+    web_app::LaunchShimUpdateBehavior update_behavior,
+    web_app::ShimLaunchMode launch_mode) {
   DCHECK(launch_shim_has_been_called_);
   DCHECK(!bootstrap_);
   launch_weak_factory_.InvalidateWeakPtrs();
   client_->OnShimLaunchRequested(
-      this, recreate_shims,
+      this, update_behavior, launch_mode,
       base::BindOnce(&AppShimHost::OnShimProcessLaunched,
-                     launch_weak_factory_.GetWeakPtr(), recreate_shims),
+                     launch_weak_factory_.GetWeakPtr(), update_behavior,
+                     launch_mode),
       base::BindOnce(&AppShimHost::OnShimProcessTerminated,
-                     launch_weak_factory_.GetWeakPtr(), recreate_shims));
+                     launch_weak_factory_.GetWeakPtr(), update_behavior,
+                     launch_mode));
 }
 
-void AppShimHost::OnShimProcessLaunched(bool recreate_shims_requested,
-                                        base::Process shim_process) {
+void AppShimHost::OnShimProcessLaunched(
+    web_app::LaunchShimUpdateBehavior update_behavior,
+    web_app::ShimLaunchMode launch_mode,
+    base::Process shim_process) {
   // If a bootstrap connected, then it should have invalidated all weak
   // pointers, preventing this from being called.
   DCHECK(!bootstrap_);
@@ -87,18 +93,22 @@ void AppShimHost::OnShimProcessLaunched(bool recreate_shims_requested,
 
   // Shim launch failing is treated the same as the shim launching but
   // terminating before connecting.
-  OnShimProcessTerminated(recreate_shims_requested);
+  OnShimProcessTerminated(update_behavior, launch_mode);
 }
 
-void AppShimHost::OnShimProcessTerminated(bool recreate_shims_requested) {
+void AppShimHost::OnShimProcessTerminated(
+    web_app::LaunchShimUpdateBehavior update_behavior,
+    web_app::ShimLaunchMode launch_mode) {
   DCHECK(!bootstrap_);
 
   // If this was a launch without recreating shims, then the launch may have
   // failed because the shims were not present, or because they were out of
   // date. Try again, recreating the shims this time.
-  if (!recreate_shims_requested) {
+  if (!web_app::RecreateShimsRequested(update_behavior)) {
     DLOG(ERROR) << "Failed to launch shim, attempting to recreate.";
-    LaunchShimInternal(true /* recreate_shims */);
+    LaunchShimInternal(
+        web_app::LaunchShimUpdateBehavior::kRecreateUnconditionally,
+        launch_mode);
     return;
   }
 
@@ -150,17 +160,22 @@ void AppShimHost::OnBootstrapConnected(
     std::move(on_shim_connected_for_testing_).Run();
 }
 
-void AppShimHost::LaunchShim() {
-  if (launch_shim_has_been_called_)
+void AppShimHost::LaunchShim(web_app::ShimLaunchMode launch_mode) {
+  if (launch_shim_has_been_called_) {
     return;
+  }
   launch_shim_has_been_called_ = true;
 
   if (bootstrap_) {
-    // If there is a connected app shim process, focus the app windows.
-    client_->OnShimFocus(this);
+    // If there is a connected app shim process, and this is not a background
+    // launch, focus the app windows.
+    if (launch_mode != web_app::ShimLaunchMode::kBackground) {
+      client_->OnShimFocus(this);
+    }
   } else {
     // Otherwise, attempt to launch whatever app shims we find.
-    LaunchShimInternal(false /* recreate_shims */);
+    LaunchShimInternal(web_app::LaunchShimUpdateBehavior::kDoNotRecreate,
+                       launch_mode);
   }
 }
 

diff --git a/chrome/browser/apps/app_shim/app_shim_host_mac.h b/chrome/browser/apps/app_shim/app_shim_host_mac.h
@@ -14,6 +14,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/process/process.h"
 #include "base/threading/thread_checker.h"
+#include "chrome/browser/web_applications/os_integration/web_app_shortcut_mac.h"
 #include "chrome/common/mac/app_shim.mojom.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/receiver.h"
@@ -42,7 +43,8 @@ class AppShimHost : public chrome::mojom::AppShimHost {
     // Request that the handler launch the app shim process.
     virtual void OnShimLaunchRequested(
         AppShimHost* host,
-        bool recreate_shims,
+        web_app::LaunchShimUpdateBehavior update_behavior,
+        web_app::ShimLaunchMode launch_mode,
         apps::ShimLaunchedCallback launched_callback,
         apps::ShimTerminatedCallback terminated_callback) = 0;
 
@@ -102,7 +104,8 @@ class AppShimHost : public chrome::mojom::AppShimHost {
 
   // Invoked to request that the shim be launched (if it has not been launched
   // already).
-  void LaunchShim();
+  void LaunchShim(
+      web_app::ShimLaunchMode launch_mode = web_app::ShimLaunchMode::kNormal);
 
   // Invoked when the app shim has launched and connected to the browser.
   virtual void OnBootstrapConnected(
@@ -128,15 +131,19 @@ class AppShimHost : public chrome::mojom::AppShimHost {
   void ChannelError(uint32_t custom_reason, const std::string& description);
 
   // Helper function to launch the app shim process.
-  void LaunchShimInternal(bool recreate_shims);
+  void LaunchShimInternal(web_app::LaunchShimUpdateBehavior update_behavior,
+                          web_app::ShimLaunchMode launch_mode);
 
   // Called when LaunchShim has launched (or failed to launch) a process.
-  void OnShimProcessLaunched(bool recreate_shims_requested,
+  void OnShimProcessLaunched(web_app::LaunchShimUpdateBehavior update_behavior,
+                             web_app::ShimLaunchMode launch_mode,
                              base::Process shim_process);
 
   // Called when a shim process returned via OnShimLaunchCompleted has
   // terminated.
-  void OnShimProcessTerminated(bool recreate_shims_requested);
+  void OnShimProcessTerminated(
+      web_app::LaunchShimUpdateBehavior update_behavior,
+      web_app::ShimLaunchMode launch_mode);
 
   // chrome::mojom::AppShimHost.
   void FocusApp() override;

diff --git a/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc b/chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc
@@ -173,7 +173,8 @@ class AppShimHostTest : public testing::Test,
   // AppShimHost::Client:
   void OnShimLaunchRequested(
       AppShimHost* host,
-      bool recreate_shims,
+      web_app::LaunchShimUpdateBehavior update_behavior,
+      web_app::ShimLaunchMode launch_mode,
       apps::ShimLaunchedCallback launched_callback,
       apps::ShimTerminatedCallback terminated_callback) override {}
   void OnShimProcessDisconnected(AppShimHost* host) override {

diff --git a/chrome/browser/apps/app_shim/app_shim_manager_mac.cc b/chrome/browser/apps/app_shim/app_shim_manager_mac.cc
@@ -378,6 +378,12 @@ bool AppShimManager::AppState::ShouldDeleteAppState() const {
   // https://crbug.com/1139254,1132223 for closing when profiles close.
   if (IsMultiProfile() &&
       base::FeatureList::IsEnabled(features::kAppShimNewCloseBehavior)) {
+    // This might get called late enough during shutdown for ProfileManager to
+    // no longer exist. GetInstalledProfilesForApp requires ProfileManager to
+    // still exist, so if we're shutting down, just return true.
+    if (g_browser_process->IsShuttingDown()) {
+      return true;
+    }
     return profiles.empty() &&
            AppShimRegistry::Get()->GetInstalledProfilesForApp(app_id).empty();
   }
@@ -469,25 +475,74 @@ AppShimManager::LaunchNotificationProvider(const web_app::AppId& app_id) {
       base::FeatureList::IsEnabled(features::kAppShimNotificationAttribution));
 
   mojo::Remote<mac_notifications::mojom::MacNotificationProvider> remote;
-  AppShimHost* shim = nullptr;
+  auto bind_provider = base::BindOnce(
+      [](mojo::PendingReceiver<
+             mac_notifications::mojom::MacNotificationProvider> receiver,
+         base::WeakPtr<AppShimManager> manager, AppShimHost* host) {
+        if (!host) {
+          LOG(ERROR) << "Failed to launch app shim for notifications";
+          if (manager) {
+            manager->dummy_notification_provider_receivers_.Add(
+                manager.get(), std::move(receiver));
+          }
+          return;
+        }
+        host->GetAppShim()->BindNotificationProvider(std::move(receiver));
+      },
+      remote.BindNewPipeAndPassReceiver(), weak_factory_.GetWeakPtr());
 
   auto found_app = apps_.find(app_id);
-  if (found_app != apps_.end()) {
-    AppState* app_state = found_app->second.get();
-    CHECK(app_state->IsMultiProfile());
-    shim = app_state->multi_profile_host.get();
+  if (found_app == apps_.end()) {
+    // To check or display a notification associated with a specific app, calls
+    // to the notifications API need to happen from within that app. If we don't
+    // already have a running app shim, launch a new one, but launch it in
+    // "background" mode, so to the user it isn't noticeable that this is
+    // happening.
+    LaunchShimInBackgroundMode(app_id, std::move(bind_provider));
+    return remote;
   }
 
-  if (shim) {
-    shim->GetAppShim()->BindNotificationProvider(
-        remote.BindNewPipeAndPassReceiver());
-  } else {
-    // TODO(mek): Support launching a new app shim.
-    dummy_notification_provider_receivers_.Add(
-        this, remote.BindNewPipeAndPassReceiver());
+  AppState* app_state = found_app->second.get();
+  CHECK(app_state->IsMultiProfile());
+  AppShimHost* shim = app_state->multi_profile_host.get();
+  std::move(bind_provider).Run(shim);
+  return remote;
+}
+
+Profile* AppShimManager::ProfileForBackgroundShimLaunch(
+    const web_app::AppId& app_id) {
+  if (profile_manager_) {
+    for (Profile* p : profile_manager_->GetLoadedProfiles()) {
+      if (!p->IsRegularProfile()) {
+        continue;
+      }
+      if (delegate_->AppIsInstalled(p, app_id)) {
+        return p;
+      }
+    }
   }
+  return nullptr;
+}
 
-  return remote;
+void AppShimManager::LaunchShimInBackgroundMode(
+    const web_app::AppId& app_id,
+    base::OnceCallback<void(AppShimHost*)> callback) {
+  // A shim can only be launched through an active profile, so find a profile
+  // through which to do the launch. This method should only be called for
+  // multi-profile apps, for which an arbitrary profile is good enough.
+  Profile* profile = ProfileForBackgroundShimLaunch(app_id);
+
+  if (!profile) {
+    LOG(ERROR) << "Failed to find loaded profile with " << app_id
+               << " installed";
+    std::move(callback).Run(nullptr);
+    return;
+  }
+
+  CHECK(delegate_->AppIsMultiProfile(profile, app_id));
+  auto* profile_state = GetOrCreateProfileState(profile, app_id);
+  std::move(callback).Run(profile_state->GetHost());
+  profile_state->GetHost()->LaunchShim(web_app::ShimLaunchMode::kBackground);
 }
 
 void AppShimManager::BindNotificationService(
@@ -552,7 +607,8 @@ bool AppShimManager::BrowserUsesRemoteCocoa(Browser* browser) {
 
 void AppShimManager::OnShimLaunchRequested(
     AppShimHost* host,
-    bool recreate_shims,
+    web_app::LaunchShimUpdateBehavior update_behavior,
+    web_app::ShimLaunchMode launch_mode,
     apps::ShimLaunchedCallback launched_callback,
     apps::ShimTerminatedCallback terminated_callback) {
   // A shim can only be launched through an active profile, so find a profile
@@ -571,19 +627,20 @@ void AppShimManager::OnShimLaunchRequested(
     }
   }
 
-  // If `recreate_shims` is true, it is possible that the app got uninstalled
-  // while an initial launch attempt took place (and failed). So check first
-  // if the app is still installed.
+  // If `update_behavior` was set to possible recreate shims, it can happen that
+  // the app got uninstalled while an initial launch attempt took place (and
+  // failed). So check first if the app is still installed.
   // TODO(mek): Rather than this workaround, we should make sure to destroy
   // AppShimHost and terminate app shims when an app is uninstalled.
-  if (recreate_shims && !delegate_->AppIsInstalled(profile, host->GetAppId())) {
+  if (web_app::RecreateShimsRequested(update_behavior) &&
+      !delegate_->AppIsInstalled(profile, host->GetAppId())) {
     LOG(ERROR)
         << "Attempting to launch shim for an app that is no longer installed.";
     std::move(terminated_callback).Run();
     return;
   }
 
-  delegate_->LaunchShim(profile, host->GetAppId(), recreate_shims,
+  delegate_->LaunchShim(profile, host->GetAppId(), update_behavior, launch_mode,
                         std::move(launched_callback),
                         std::move(terminated_callback));
 }
@@ -1375,8 +1432,9 @@ void AppShimManager::OnAppDeactivated(content::BrowserContext* context,
   // happens.
   std::string inconsistent_app_ids;
   for (const auto& [id, state] : apps_) {
-    if (state->ShouldDeleteAppState())
+    if (state->ShouldDeleteAppState()) {
       inconsistent_app_ids += id + " ";
+    }
   }
   if (!inconsistent_app_ids.empty())
     DumpError(inconsistent_app_ids);

diff --git a/chrome/browser/apps/app_shim/app_shim_manager_mac.h b/chrome/browser/apps/app_shim/app_shim_manager_mac.h
@@ -27,6 +27,7 @@
 #include "chrome/browser/profiles/profile_observer.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list_observer.h"
+#include "chrome/browser/web_applications/os_integration/web_app_shortcut_mac.h"
 #include "chrome/browser/web_applications/web_app_id.h"
 #include "chrome/services/mac_notifications/public/mojom/mac_notifications.mojom.h"
 
@@ -120,7 +121,8 @@ class AppShimManager
     // installed for |profile| when this method is called.
     virtual void LaunchShim(Profile* profile,
                             const web_app::AppId& app_id,
-                            bool recreate_shims,
+                            web_app::LaunchShimUpdateBehavior update_behavior,
+                            web_app::ShimLaunchMode launch_mode,
                             ShimLaunchedCallback launched_callback,
                             ShimTerminatedCallback terminated_callback) = 0;
 
@@ -189,7 +191,8 @@ class AppShimManager
   // AppShimHost::Client:
   void OnShimLaunchRequested(
       AppShimHost* host,
-      bool recreate_shims,
+      web_app::LaunchShimUpdateBehavior update_behavior,
+      web_app::ShimLaunchMode launch_mode,
       base::OnceCallback<void(base::Process)> launched_callback,
       base::OnceClosure terminated_callback) override;
   void OnShimProcessDisconnected(AppShimHost* host) override;
@@ -256,6 +259,9 @@ class AppShimManager
   // Return the profile for |path|, only if it is already loaded.
   virtual Profile* ProfileForPath(const base::FilePath& path);
 
+  // Return a profile to use for a background shim launch, virtual for tests.
+  virtual Profile* ProfileForBackgroundShimLaunch(const web_app::AppId& app_id);
+
   // Load a profile and call |callback| when completed or failed.
   virtual void LoadProfileAsync(const base::FilePath& path,
                                 base::OnceCallback<void(Profile*)> callback);
@@ -407,6 +413,10 @@ class AppShimManager
   ProfileState* GetOrCreateProfileState(Profile* profile,
                                         const web_app::AppId& app_id);
 
+  void LaunchShimInBackgroundMode(
+      const web_app::AppId& app_id,
+      base::OnceCallback<void(AppShimHost*)> callback);
+
   // Returns a mapping of profile paths to how many of the files and urls passed
   // in in `params` each profile can handle.
   static std::map<base::FilePath, int> GetProfilesWithMatchingHandlers(