MacPWAs: Detect when app shims launch but terminate early

Change OpenApplicationWithPath to return the NSRunningApplication
instead of just its pid. Use this in LaunchShim to detect when the
process terminates, and issue a callback, which lands in
AppShimHost::OnShimProcessTerminated.

In AppShimHost, treat OnShimProcessTerminated just like a failure in
OnShimProcessLaunched -- attempt to recreate the app bundles and launch
again, and if that fails, close the app's windows. Ensure that only
the first call to AppShimHost::LaunchShim do anything.

Handle IsAcceptablyCodeSigned failing by closing the shim process
and re-launching. This makes us re-generate app shims when they are
not signed by the following sequence
- The launch will attempt to launch from the Applications folder
- If this is signed, success
- If this is not signed (more likely), then it will fall through the
  same path -- the second AppShimHost::LaunchShim call will do nothing,
  and the shim process will terminate.
- This termination will cause us to launch again, this time re-creating
  the shim, whereupon it should now be signed.
Add tests for this sequence.

Bug: 924482
Change-Id: I915c8fb3b9ee9b8fe109e7330d16673a4192e30a
Reviewed-on: https://chromium-review.googlesource.com/c/1433442
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625769}

base/mac/launch_services_util.h

@@ -10,7 +10,6 @@
 #include "base/base_export.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
-#include "base/process/process.h"
 
 namespace base {
 namespace mac {
@@ -19,11 +18,11 @@ namespace mac {
 // |command_line| as command line arguments if the app isn't already running.
 // |launch_options| are passed directly to
 // -[NSWorkspace launchApplicationAtURL:options:configuration:error:].
-// Returns a valid process if the app was successfully launched.
-BASE_EXPORT Process
-OpenApplicationWithPath(const FilePath& bundle_path,
-                        const CommandLine& command_line,
-                        NSWorkspaceLaunchOptions launch_options);
+// Returns a non-nil NSRunningApplication if the app was successfully launched.
+BASE_EXPORT NSRunningApplication* OpenApplicationWithPath(
+    const FilePath& bundle_path,
+    const CommandLine& command_line,
+    NSWorkspaceLaunchOptions launch_options);
 
 }  // namespace mac
 }  // namespace base

base/mac/launch_services_util.mm

@@ -10,14 +10,15 @@
 namespace base {
 namespace mac {
 
-Process OpenApplicationWithPath(const base::FilePath& bundle_path,
-                                const CommandLine& command_line,
-                                NSWorkspaceLaunchOptions launch_options) {
+NSRunningApplication* OpenApplicationWithPath(
+    const base::FilePath& bundle_path,
+    const CommandLine& command_line,
+    NSWorkspaceLaunchOptions launch_options) {
   NSString* bundle_url_spec = base::SysUTF8ToNSString(bundle_path.value());
   NSURL* bundle_url = [NSURL fileURLWithPath:bundle_url_spec isDirectory:YES];
   DCHECK(bundle_url);
   if (!bundle_url) {
-    return Process();
+    return nil;
   }
 
   // NSWorkspace automatically adds the binary path as the first argument and
@@ -42,10 +43,10 @@ Process OpenApplicationWithPath(const base::FilePath& bundle_path,
                                                       error:&launch_error];
   if (launch_error) {
     LOG(ERROR) << base::SysNSStringToUTF8([launch_error localizedDescription]);
-    return Process();
+    return nil;
   }
   DCHECK(app);
-  return Process([app processIdentifier]);
+  return app;
 }
 
 }  // namespace mac

chrome/app_shim/chrome_main_app_mode_mac.mm

@@ -277,9 +277,9 @@ int ChromeAppModeStart_v5(const app_mode::ChromeAppModeInfo* info) {
                                     base::FilePath(info->profile_dir));
     }
 
-    base::Process app = base::mac::OpenApplicationWithPath(
+    NSRunningApplication* running_app = base::mac::OpenApplicationWithPath(
         base::mac::OuterBundlePath(), command_line, NSWorkspaceLaunchDefault);
-    if (!app.IsValid())
+    if (!running_app)
       return 1;
 
     base::Callback<void(bool)> on_ping_chrome_reply = base::Bind(

chrome/browser/apps/app_shim/app_shim_host_mac.cc

@@ -29,10 +29,11 @@ AppShimHost::AppShimHost(const std::string& app_id,
                          bool uses_remote_views)
     : host_binding_(this),
       app_shim_request_(mojo::MakeRequest(&app_shim_)),
+      launch_shim_has_been_called_(false),
       app_id_(app_id),
       profile_path_(profile_path),
       uses_remote_views_(uses_remote_views),
-      weak_factory_(this) {
+      launch_weak_factory_(this) {
   // Create the interfaces used to host windows, so that browser windows may be
   // created before the host process finishes launching.
   if (uses_remote_views_) {
@@ -85,28 +86,39 @@ apps::AppShimHandler* AppShimHost::GetAppShimHandler() const {
 }
 
 void AppShimHost::LaunchShimInternal(bool recreate_shims) {
+  DCHECK(launch_shim_has_been_called_);
   DCHECK(!bootstrap_);
   apps::AppShimHandler* handler = GetAppShimHandler();
   if (!handler)
     return;
+  launch_weak_factory_.InvalidateWeakPtrs();
   handler->OnShimLaunchRequested(
       this, recreate_shims,
       base::BindOnce(&AppShimHost::OnShimProcessLaunched,
-                     weak_factory_.GetWeakPtr(), recreate_shims),
+                     launch_weak_factory_.GetWeakPtr(), recreate_shims),
       base::BindOnce(&AppShimHost::OnShimProcessTerminated,
-                     weak_factory_.GetWeakPtr()));
+                     launch_weak_factory_.GetWeakPtr(), recreate_shims));
 }
 
 void AppShimHost::OnShimProcessLaunched(bool recreate_shims_requested,
                                         base::Process shim_process) {
-  // If the shim process was created, assume that it will successfully create
-  // an AppShimHostBootstrap.
-  // TODO(https://crbug.com/913362): We should add a callback for when
-  // |shim_process| exits, and handle the case where that happens without the
-  // creation of an AppShimHostBootstrap.
+  // If a bootstrap connected, then it should have invalidated all weak
+  // pointers, preventing this from being called.
+  DCHECK(!bootstrap_);
+
+  // If the shim process was created, then await either an AppShimHostBootstrap
+  // connecting or the process exiting.
   if (shim_process.IsValid())
     return;
 
+  // Shim launch failing is treated the same as the shim launching but
+  // terminating before connecting.
+  OnShimProcessTerminated(recreate_shims_requested);
+}
+
+void AppShimHost::OnShimProcessTerminated(bool recreate_shims_requested) {
+  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.
@@ -125,8 +137,6 @@ void AppShimHost::OnShimProcessLaunched(bool recreate_shims_requested,
   OnAppClosed();
 }
 
-void AppShimHost::OnShimProcessTerminated() {}
-
 ////////////////////////////////////////////////////////////////////////////////
 // AppShimHost, chrome::mojom::AppShimHost
 
@@ -136,6 +146,10 @@ bool AppShimHost::HasBootstrapConnected() const {
 
 void AppShimHost::OnBootstrapConnected(
     std::unique_ptr<AppShimHostBootstrap> bootstrap) {
+  // Prevent any callbacks from any pending launches (e.g, if an internal and
+  // external launch happen to race).
+  launch_weak_factory_.InvalidateWeakPtrs();
+
   DCHECK(!bootstrap_);
   bootstrap_ = std::move(bootstrap);
   bootstrap_->OnConnectedToHost(std::move(app_shim_request_));
@@ -147,6 +161,10 @@ void AppShimHost::OnBootstrapConnected(
 }
 
 void AppShimHost::LaunchShim() {
+  if (launch_shim_has_been_called_)
+    return;
+  launch_shim_has_been_called_ = true;
+
   apps::AppShimHandler* handler = GetAppShimHandler();
   if (!handler)
     return;

chrome/browser/apps/app_shim/app_shim_host_mac.h

@@ -81,6 +81,11 @@ class AppShimHost : public chrome::mojom::AppShimHost {
   // AppShimHost is owned by itself. It will delete itself in Close (called on
   // channel error and OnAppClosed).
   ~AppShimHost() override;
+
+  // Return the AppShimHandler for this app (virtual for tests).
+  virtual apps::AppShimHandler* GetAppShimHandler() const;
+
+ private:
   void ChannelError(uint32_t custom_reason, const std::string& description);
 
   // Closes the channel and destroys the AppShimHost.
@@ -95,10 +100,7 @@ class AppShimHost : public chrome::mojom::AppShimHost {
 
   // Called when a shim process returned via OnShimLaunchCompleted has
   // terminated.
-  void OnShimProcessTerminated();
-
-  // Return the AppShimHandler for this app (virtual for tests).
-  virtual apps::AppShimHandler* GetAppShimHandler() const;
+  void OnShimProcessTerminated(bool recreate_shims_requested);
 
   // chrome::mojom::AppShimHost.
   void FocusApp(apps::AppShimFocusType focus_type,
@@ -110,6 +112,11 @@ class AppShimHost : public chrome::mojom::AppShimHost {
   chrome::mojom::AppShimPtr app_shim_;
   chrome::mojom::AppShimRequest app_shim_request_;
 
+  // Only allow LaunchShim to have any effect on the first time it is called. If
+  // that launch fails, it will re-launch (requesting that the shim be
+  // re-created).
+  bool launch_shim_has_been_called_;
+
   std::unique_ptr<AppShimHostBootstrap> bootstrap_;
 
   std::unique_ptr<views::BridgeFactoryHost> views_bridge_factory_host_;
@@ -121,7 +128,9 @@ class AppShimHost : public chrome::mojom::AppShimHost {
 
   // This class is only ever to be used on the UI thread.
   THREAD_CHECKER(thread_checker_);
-  base::WeakPtrFactory<AppShimHost> weak_factory_;
+
+  // This weak factory is used for launch callbacks only.
+  base::WeakPtrFactory<AppShimHost> launch_weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(AppShimHost);
 };
 

chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm

@@ -368,9 +368,10 @@ IN_PROC_BROWSER_TEST_F(AppShimInteractiveTest, MAYBE_HostedAppLaunch) {
     HostedAppBrowserListObserver listener(app->id());
     base::CommandLine shim_cmdline(base::CommandLine::NO_PROGRAM);
     shim_cmdline.AppendSwitch(app_mode::kLaunchedForTest);
-    base::Process shim_process = base::mac::OpenApplicationWithPath(
+    NSRunningApplication* shim_app = base::mac::OpenApplicationWithPath(
         shim_path_, shim_cmdline, NSWorkspaceLaunchDefault);
-    ASSERT_TRUE(shim_process.IsValid());
+    ASSERT_TRUE(shim_app);
+    base::Process shim_process([shim_app processIdentifier]);
     listener.WaitUntilAdded();
 
     ASSERT_TRUE(GetFirstHostedAppWindow());
@@ -438,9 +439,10 @@ IN_PROC_BROWSER_TEST_F(AppShimInteractiveTest, MAYBE_Launch) {
     ExtensionTestMessageListener launched_listener("Launched", false);
     base::CommandLine shim_cmdline(base::CommandLine::NO_PROGRAM);
     shim_cmdline.AppendSwitch(app_mode::kLaunchedForTest);
-    base::Process shim_process = base::mac::OpenApplicationWithPath(
+    NSRunningApplication* shim_app = base::mac::OpenApplicationWithPath(
         shim_path_, shim_cmdline, NSWorkspaceLaunchDefault);
-    ASSERT_TRUE(shim_process.IsValid());
+    ASSERT_TRUE(shim_app);
+    base::Process shim_process([shim_app processIdentifier]);
     ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
 
     ASSERT_TRUE(GetFirstAppWindow());
@@ -657,9 +659,9 @@ IN_PROC_BROWSER_TEST_F(AppShimInteractiveTest, MAYBE_RebuildShim) {
   //     behave normally.
   ExtensionTestMessageListener launched_listener("Launched", false);
   base::CommandLine shim_cmdline(base::CommandLine::NO_PROGRAM);
-  base::Process shim_process = base::mac::OpenApplicationWithPath(
+  NSRunningApplication* shim_app = base::mac::OpenApplicationWithPath(
       shim_path, shim_cmdline, NSWorkspaceLaunchDefault);
-  ASSERT_TRUE(shim_process.IsValid());
+  ASSERT_TRUE(shim_app);
 
   // Wait for the app to start (1). At this point there is no shim host.
   ASSERT_TRUE(launched_listener.WaitUntilSatisfied());

chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc

@@ -124,7 +124,7 @@ std::string CertificateSHA1Digest(SecCertificateRef certificate) {
 // - True if the caller is unsigned (there's nothing to verify).
 // - True if |pid| satisfies the caller's designated requirement.
 // - False otherwise (|pid| does not satisfy caller's designated requirement).
-bool IsAcceptablyCodeSigned(pid_t pid) {
+bool IsAcceptablyCodeSignedInternal(pid_t pid) {
   base::ScopedCFTypeRef<SecCodeRef> own_code;
   base::ScopedCFTypeRef<CFDictionaryRef> own_signing_info;
 
@@ -674,23 +674,6 @@ void ExtensionAppShimHandler::OnExtensionEnabled(
     return;
   }
 
-  // If the connecting shim process doesn't have an acceptable code signature,
-  // reject the connection and recreate the shim.
-  if (!IsAcceptablyCodeSigned(bootstrap->GetAppShimPid())) {
-    // TODO(https://crbug.com/923612): Should only be here for a day or two.
-    LOG(ERROR) << "The attaching app shim's code signature is invalid. This "
-                  "will fail in future builds of Chrome.";
-#if 0
-    if (bootstrap->GetLaunchType() == APP_SHIM_LAUNCH_NORMAL) {
-      constexpr bool recreate_shims = true;
-      delegate_->LaunchShim(profile, extension, recreate_shims,
-                            base::BindOnce([](base::Process) {}));
-    }
-    bootstrap->OnFailedToConnectToHost(APP_SHIM_LAUNCH_FAILED_VALIDATION);
-    return;
-#endif
-  }
-
   AppShimHost* host = delegate_->AllowShimToConnect(profile, extension)
                           ? FindOrCreateHost(profile, extension)
                           : nullptr;
@@ -706,7 +689,16 @@ void ExtensionAppShimHandler::OnExtensionEnabled(
       bootstrap->OnFailedToConnectToHost(APP_SHIM_LAUNCH_DUPLICATE_HOST);
       return;
     }
-    host->OnBootstrapConnected(std::move(bootstrap));
+    if (IsAcceptablyCodeSigned(bootstrap->GetAppShimPid())) {
+      host->OnBootstrapConnected(std::move(bootstrap));
+    } else {
+      // If the connecting shim process doesn't have an acceptable code
+      // signature, reject the connection and re-launch the shim. The internal
+      // re-launch will likely fail, whereupon the shim will be recreated.
+      LOG(ERROR) << "The attaching app shim's code signature is invalid.";
+      bootstrap->OnFailedToConnectToHost(APP_SHIM_LAUNCH_FAILED_VALIDATION);
+      host->LaunchShim();
+    }
   } else {
     // If it's an app that has a shim to launch it but shouldn't use a host
     // (e.g, a hosted app that opens in a tab), terminate the shim, but still
@@ -720,6 +712,10 @@ void ExtensionAppShimHandler::OnExtensionEnabled(
     delegate_->LaunchApp(profile, extension, files);
 }
 
+bool ExtensionAppShimHandler::IsAcceptablyCodeSigned(pid_t pid) const {
+  return IsAcceptablyCodeSignedInternal(pid);
+}
+
 void ExtensionAppShimHandler::OnShimClose(AppShimHost* host) {
   // This might be called when shutting down. Don't try to look up the profile
   // since profile_manager might not be around.

chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h

@@ -174,6 +174,9 @@ class ExtensionAppShimHandler : public AppShimHandler,
   typedef std::set<Browser*> BrowserSet;
   typedef std::map<std::string, BrowserSet> AppBrowserMap;
 
+  // Virtual for tests.
+  virtual bool IsAcceptablyCodeSigned(pid_t pid) const;
+
   // Exposed for testing.
   void set_delegate(Delegate* delegate);
   HostMap& hosts() { return hosts_; }

chrome/browser/apps/app_shim/extension_app_shim_handler_mac_unittest.cc

@@ -82,11 +82,16 @@ class MockDelegate : public ExtensionAppShimHandler::Delegate {
                   ShimTerminatedCallback terminated_callback) override {
     if (launch_shim_callback_capture_)
       *launch_shim_callback_capture_ = std::move(launched_callback);
+    if (terminated_shim_callback_capture_)
+      *terminated_shim_callback_capture_ = std::move(terminated_callback);
     DoLaunchShim(profile, extension, recreate_shim);
   }
   void SetCaptureShimLaunchedCallback(ShimLaunchedCallback* callback) {
     launch_shim_callback_capture_ = callback;
   }
+  void SetCaptureShimTerminatedCallback(ShimTerminatedCallback* callback) {
+    terminated_shim_callback_capture_ = callback;
+  }
 
   MOCK_METHOD0(LaunchUserManager, void());
 
@@ -125,6 +130,7 @@ class MockDelegate : public ExtensionAppShimHandler::Delegate {
 
  private:
   ShimLaunchedCallback* launch_shim_callback_capture_ = nullptr;
+  ShimTerminatedCallback* terminated_shim_callback_capture_ = nullptr;
   std::map<base::FilePath, base::OnceCallback<void(Profile*)>> callbacks_;
   AppShimHost* host_for_create_ = nullptr;
   bool allow_shim_to_connect_ = true;
@@ -153,9 +159,17 @@ class TestingExtensionAppShimHandler : public ExtensionAppShimHandler {
     return it == hosts().end() ? NULL : it->second;
   }
 
+  void SetAcceptablyCodeSigned(bool is_acceptable_code_signed) {
+    is_acceptably_code_signed_ = is_acceptable_code_signed;
+  }
+  bool IsAcceptablyCodeSigned(pid_t pid) const override {
+    return is_acceptably_code_signed_;
+  }
+
   content::NotificationRegistrar& GetRegistrar() { return registrar(); }
 
  private:
+  bool is_acceptably_code_signed_ = true;
   DISALLOW_COPY_AND_ASSIGN(TestingExtensionAppShimHandler);
 };
 
@@ -215,7 +229,7 @@ class TestHost : public AppShimHost {
            TestingExtensionAppShimHandler* handler)
       : AppShimHost(app_id, profile_path, false /* uses_remote_views */),
         handler_(handler),
-        weak_factory_(this) {}
+        test_weak_factory_(this) {}
 
   // Override the GetAppShimHandler for testing.
   apps::AppShimHandler* GetAppShimHandler() const override { return handler_; }
@@ -229,14 +243,16 @@ class TestHost : public AppShimHost {
   }
   bool did_connect_to_host() const { return did_connect_to_host_; }
 
-  base::WeakPtr<TestHost> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }
+  base::WeakPtr<TestHost> GetWeakPtr() {
+    return test_weak_factory_.GetWeakPtr();
+  }
 
  private:
   ~TestHost() override {}
   TestingExtensionAppShimHandler* handler_;
   bool did_connect_to_host_ = false;
 
-  base::WeakPtrFactory<TestHost> weak_factory_;
+  base::WeakPtrFactory<TestHost> test_weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(TestHost);
 };
 
@@ -264,6 +280,11 @@ class ExtensionAppShimHandlerTest : public testing::Test {
                                          &bootstrap_aa_duplicate_result_,
                                          handler_.get()))
             ->GetWeakPtr();
+    bootstrap_aa_thethird_ =
+        (new TestingAppShimHostBootstrap(profile_path_a_, kTestAppIdA,
+                                         &bootstrap_aa_thethird_result_,
+                                         handler_.get()))
+            ->GetWeakPtr();
 
     host_aa_ = (new TestHost(profile_path_a_, kTestAppIdA, handler_.get()))
                    ->GetWeakPtr();
@@ -329,6 +350,7 @@ class ExtensionAppShimHandlerTest : public testing::Test {
     delete bootstrap_ab_.get();
     delete bootstrap_bb_.get();
     delete bootstrap_aa_duplicate_.get();
+    delete bootstrap_aa_thethird_.get();
   }
 
   void DoShimLaunch(base::WeakPtr<TestingAppShimHostBootstrap> bootstrap,
@@ -390,11 +412,13 @@ class ExtensionAppShimHandlerTest : public testing::Test {
   base::WeakPtr<TestingAppShimHostBootstrap> bootstrap_ab_;
   base::WeakPtr<TestingAppShimHostBootstrap> bootstrap_bb_;
   base::WeakPtr<TestingAppShimHostBootstrap> bootstrap_aa_duplicate_;
+  base::WeakPtr<TestingAppShimHostBootstrap> bootstrap_aa_thethird_;
 
   base::Optional<apps::AppShimLaunchResult> bootstrap_aa_result_;
   base::Optional<apps::AppShimLaunchResult> bootstrap_ab_result_;
   base::Optional<apps::AppShimLaunchResult> bootstrap_bb_result_;
   base::Optional<apps::AppShimLaunchResult> bootstrap_aa_duplicate_result_;
+  base::Optional<apps::AppShimLaunchResult> bootstrap_aa_thethird_result_;
 
   base::WeakPtr<TestHost> host_aa_;
   base::WeakPtr<TestHost> host_ab_;
@@ -561,6 +585,80 @@ TEST_F(ExtensionAppShimHandlerTest, FailToLaunch) {
   EXPECT_EQ(nullptr, host_aa_.get());
 }
 
+TEST_F(ExtensionAppShimHandlerTest, FailToConnect) {
+  // When the app activates, it requests a launch.
+  ShimLaunchedCallback launched_callback;
+  delegate_->SetCaptureShimLaunchedCallback(&launched_callback);
+  ShimTerminatedCallback terminated_callback;
+  delegate_->SetCaptureShimTerminatedCallback(&terminated_callback);
+
+  delegate_->SetHostForCreate(host_aa_.get());
+  EXPECT_CALL(*delegate_, DoLaunchShim(&profile_a_, extension_a_.get(), false));
+  handler_->OnAppActivated(&profile_a_, kTestAppIdA);
+  EXPECT_EQ(host_aa_.get(), handler_->FindHost(&profile_a_, kTestAppIdA));
+  EXPECT_TRUE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+
+  // Run the launch callback claiming that the launch succeeded.
+  std::move(launched_callback).Run(base::Process(5));
+  EXPECT_FALSE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+
+  // Report that the process terminated. This should trigger a re-create and
+  // re-launch.
+  EXPECT_CALL(*delegate_, DoLaunchShim(&profile_a_, extension_a_.get(), true));
+  std::move(terminated_callback).Run();
+  EXPECT_TRUE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+
+  // Run the launch callback claiming that the launch succeeded.
+  std::move(launched_callback).Run(base::Process(7));
+  EXPECT_FALSE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+
+  // Report that the process terminated again. This should trigger deletion of
+  // the host.
+  EXPECT_NE(nullptr, host_aa_.get());
+  std::move(terminated_callback).Run();
+  EXPECT_EQ(nullptr, host_aa_.get());
+}
+
+TEST_F(ExtensionAppShimHandlerTest, FailCodeSignature) {
+  handler_->SetAcceptablyCodeSigned(false);
+  ShimLaunchedCallback launched_callback;
+  delegate_->SetCaptureShimLaunchedCallback(&launched_callback);
+  ShimTerminatedCallback terminated_callback;
+  delegate_->SetCaptureShimTerminatedCallback(&terminated_callback);
+
+  // Fail to code-sign. This should result in a host being created, and a launch
+  // having been requested.
+  EXPECT_CALL(*delegate_, DoLaunchShim(&profile_a_, extension_a_.get(), false));
+  NormalLaunch(bootstrap_aa_, host_aa_);
+  EXPECT_EQ(host_aa_.get(), handler_->FindHost(&profile_a_, kTestAppIdA));
+  EXPECT_TRUE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+  EXPECT_FALSE(host_aa_->HasBootstrapConnected());
+
+  // Run the launch callback claiming that the launch succeeded.
+  std::move(launched_callback).Run(base::Process(5));
+  EXPECT_FALSE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+  EXPECT_FALSE(host_aa_->HasBootstrapConnected());
+
+  // Simulate the register call that then fails due to signature failing.
+  RegisterOnlyLaunch(bootstrap_aa_duplicate_, host_aa_);
+  EXPECT_FALSE(host_aa_->HasBootstrapConnected());
+
+  // Simulate the termination after the register failed.
+  handler_->SetAcceptablyCodeSigned(true);
+  EXPECT_CALL(*delegate_, DoLaunchShim(&profile_a_, extension_a_.get(), true));
+  std::move(terminated_callback).Run();
+  EXPECT_TRUE(launched_callback);
+  EXPECT_TRUE(terminated_callback);
+  RegisterOnlyLaunch(bootstrap_aa_thethird_, host_aa_);
+  EXPECT_TRUE(host_aa_->HasBootstrapConnected());
+}
+
 TEST_F(ExtensionAppShimHandlerTest, MaybeTerminate) {
   // Launch shims, adding entries in the map.
   RegisterOnlyLaunch(bootstrap_aa_, host_aa_);

chrome/browser/web_applications/components/web_app_shortcut_mac.mm

@@ -60,6 +60,44 @@
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image_family.h"
 
+// A TerminationObserver observes a NSRunningApplication for when it
+// terminates. On termination, it will run the specified callback, and then
+// release itself.
+@interface TerminationObserver : NSObject {
+  NSRunningApplication* app_;
+  base::OnceClosure callback_;
+}
+- (id)initWithRunningApplication:(NSRunningApplication*)app
+                        callback:(base::OnceClosure)callback;
+@end
+
+@implementation TerminationObserver
+- (id)initWithRunningApplication:(NSRunningApplication*)app
+                        callback:(base::OnceClosure)callback {
+  if (self = [super init]) {
+    app_ = app;
+    callback_ = std::move(callback);
+    [app_ retain];
+    [app_ addObserver:self
+           forKeyPath:@"isTerminated"
+              options:NSKeyValueObservingOptionNew
+              context:nullptr];
+  }
+  return self;
+}
+- (void)observeValueForKeyPath:(NSString*)keyPath
+                      ofObject:(id)object
+                        change:(NSDictionary*)change
+                       context:(void*)context {
+  [app_ removeObserver:self forKeyPath:@"isTerminated" context:nullptr];
+  base::PostTaskWithTraits(FROM_HERE, {content::BrowserThread::UI},
+                           std::move(callback_));
+  [app_ release];
+  app_ = nil;
+  [self release];
+}
+@end
+
 bool g_app_shims_allow_update_and_launch_in_tests = false;
 
 namespace {
@@ -235,13 +273,17 @@ void LaunchShimOnFileThread(web_app::LaunchShimUpdateBehavior update_behavior,
       command_line.AppendSwitch(app_mode::kLaunchedAfterRebuild);
 
     // Launch without activating (NSWorkspaceLaunchWithoutActivation).
-    base::Process process = base::mac::OpenApplicationWithPath(
+    NSRunningApplication* app = base::mac::OpenApplicationWithPath(
         shim_path, command_line,
         NSWorkspaceLaunchDefault | NSWorkspaceLaunchWithoutActivation);
-    if (process.IsValid()) {
+    if (app) {
+      base::Process process([app processIdentifier]);
       base::PostTaskWithTraits(
           FROM_HERE, {content::BrowserThread::UI},
           base::BindOnce(std::move(launched_callback), std::move(process)));
+      [[TerminationObserver alloc]
+          initWithRunningApplication:app
+                            callback:std::move(terminated_callback)];
       return;
     }
   }