Add metrics to base::mac::LaunchApplication.

In particular we want to know how frequently kSuccessDespiteError occurs
in the wild.

Bug: 938661
Change-Id: I0cec92a614c7974efaac93270e88798ddbf61e4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4743481
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1181724}

base/mac/launch_application.mm

@@ -11,6 +11,7 @@
 #include "base/mac/foundation_util.h"
 #include "base/mac/launch_services_spi.h"
 #include "base/mac/mac_util.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/types/expected.h"
 
@@ -18,6 +19,19 @@ namespace base::mac {
 
 namespace {
 
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+enum class LaunchResult {
+  kSuccess = 0,
+  kSuccessDespiteError = 1,
+  kFailure = 2,
+  kMaxValue = kFailure,
+};
+
+void LogLaunchResult(LaunchResult result) {
+  UmaHistogramEnumeration("Mac.LaunchApplicationResult", result);
+}
+
 NSArray* CommandLineArgsToArgsArray(const CommandLineArgs& command_line_args) {
   if (const CommandLine* command_line =
           absl::get_if<CommandLine>(&command_line_args)) {
@@ -99,6 +113,45 @@ bool ShouldScanRunningAppsForError(NSError* error) {
   return false;
 }
 
+void LogResultAndInvokeCallback(const base::FilePath& app_bundle_path,
+                                bool create_new_instance,
+                                LaunchApplicationCallback callback,
+                                NSRunningApplication* app,
+                                NSError* error) {
+  // Sometimes macOS 11 and 12 report an error launching even though the
+  // launch succeeded anyway. To work around such cases, check if we can
+  // find a running application matching the app we were trying to launch.
+  // Only do this if `options.create_new_instance` is false though, as
+  // otherwise we wouldn't know which instance to return.
+  if (IsAtLeastOS11() && IsAtMostOS12() && !create_new_instance && !app &&
+      ShouldScanRunningAppsForError(error)) {
+    NSArray<NSRunningApplication*>* all_apps =
+        NSWorkspace.sharedWorkspace.runningApplications;
+    for (NSRunningApplication* running_app in all_apps) {
+      if (NSURLToFilePath(running_app.bundleURL) == app_bundle_path) {
+        LOG(ERROR) << "Launch succeeded despite error: "
+                   << base::SysNSStringToUTF8(error.localizedDescription);
+        app = running_app;
+        break;
+      }
+    }
+    if (app) {
+      error = nil;
+    }
+    LogLaunchResult(app ? LaunchResult::kSuccessDespiteError
+                        : LaunchResult::kFailure);
+  } else {
+    LogLaunchResult(app ? LaunchResult::kSuccess : LaunchResult::kFailure);
+  }
+
+  if (error) {
+    LOG(ERROR) << base::SysNSStringToUTF8(error.localizedDescription);
+    std::move(callback).Run(nil, error);
+  } else {
+    std::move(callback).Run(app, nil);
+  }
+}
+
 }  // namespace
 
 void LaunchApplication(const base::FilePath& app_bundle_path,
@@ -106,35 +159,9 @@ void LaunchApplication(const base::FilePath& app_bundle_path,
                        const std::vector<std::string>& url_specs,
                        LaunchApplicationOptions options,
                        LaunchApplicationCallback callback) {
-  __block LaunchApplicationCallback callback_block_access = std::move(callback);
-  if (IsAtLeastOS11() && IsAtMostOS12() && !options.create_new_instance) {
-    // Sometimes macOS 11 and 12 report an error launching even though the
-    // launch succeeded anyway. To work around such cases, check if we can
-    // find a running application matching the app we were trying to launch.
-    // Only do this if `options.create_new_instance` is false though, as
-    // otherwise we wouldn't know which instance to return.
-    callback_block_access = base::BindOnce(
-        [](const base::FilePath& app_bundle_path,
-           LaunchApplicationCallback callback, NSRunningApplication* app,
-           NSError* error) {
-          if (!app && ShouldScanRunningAppsForError(error)) {
-            NSArray<NSRunningApplication*>* all_apps =
-                NSWorkspace.sharedWorkspace.runningApplications;
-            for (NSRunningApplication* running_app in all_apps) {
-              if (NSURLToFilePath(running_app.bundleURL) == app_bundle_path) {
-                LOG(ERROR) << "Launch succeeded despite error";
-                app = running_app;
-                break;
-              }
-            }
-            if (app) {
-              error = nil;
-            }
-          }
-          std::move(callback).Run(app, error);
-        },
-        app_bundle_path, std::move(callback_block_access));
-  }
+  __block LaunchApplicationCallback callback_block_access =
+      base::BindOnce(&LogResultAndInvokeCallback, app_bundle_path,
+                     options.create_new_instance, std::move(callback));
 
   NSURL* bundle_url = FilePathToNSURL(app_bundle_path);
   if (!bundle_url) {
@@ -166,12 +193,7 @@ void LaunchApplication(const base::FilePath& app_bundle_path,
           }
           NSError* error = base::apple::CFToNSPtrCast(cf_error);
           dispatch_async(dispatch_get_main_queue(), ^{
-            if (error) {
-              LOG(ERROR) << base::SysNSStringToUTF8(error.localizedDescription);
-              std::move(callback_block_access).Run(nil, error);
-            } else {
-              std::move(callback_block_access).Run(app, nil);
-            }
+            std::move(callback_block_access).Run(app, error);
           });
         };
 
@@ -186,12 +208,7 @@ void LaunchApplication(const base::FilePath& app_bundle_path,
   void (^action_block)(NSRunningApplication*, NSError*) =
       ^void(NSRunningApplication* app, NSError* error) {
         dispatch_async(dispatch_get_main_queue(), ^{
-          if (error) {
-            LOG(ERROR) << base::SysNSStringToUTF8(error.localizedDescription);
-            std::move(callback_block_access).Run(nil, error);
-          } else {
-            std::move(callback_block_access).Run(app, nil);
-          }
+          std::move(callback_block_access).Run(app, error);
         });
       };
 

tools/metrics/histograms/enums.xml

@@ -69063,6 +69063,14 @@ from previous Chrome versions.
   </int>
 </enum>
 
+<enum name="MacLaunchApplicationResult">
+  <int value="0" label="Success"/>
+  <int value="1" label="Success despite error">
+    The OS returned an error, but the launch appears to have succeeded anyway.
+  </int>
+  <int value="2" label="Failure"/>
+</enum>
+
 <enum name="MacThermalState">
   <int value="0" label="0-Nominal"/>
   <int value="1" label="1-Fair"/>

tools/metrics/histograms/metadata/others/histograms.xml

@@ -7875,6 +7875,16 @@ chromium-metrics-reviews@google.com.
   </summary>
 </histogram>
 
+<histogram name="Mac.LaunchApplicationResult" enum="MacLaunchApplicationResult"
+    expires_after="2023-12-31">
+  <owner>mek@chromium.org</owner>
+  <owner>src/base/mac/OWNERS</owner>
+  <summary>
+    The result of calls to base::mac::LaunchApplication, logged once for every
+    invocation of that method.
+  </summary>
+</histogram>
+
 <histogram name="Mac.UniversalLink.APIDuration" units="ms"
     expires_after="2022-07-01">
   <obsolete>