Add UMA for --app-shim-error.

chrome/browser/web_applications/web_app_mac.mm

Index: chrome/browser/web_applications/web_app_mac.mm
diff --git a/chrome/browser/web_applications/web_app_mac.mm b/chrome/browser/web_applications/web_app_mac.mm
index 02f3f0dc7c14ead4c29c6e2e6b3ff23c5122760f..af41cdfe63f09516d1bafba606237afefd748dfb 100644
--- a/chrome/browser/web_applications/web_app_mac.mm
+++ b/chrome/browser/web_applications/web_app_mac.mm
@@ -16,6 +16,7 @@
 #include "base/mac/mac_util.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/mac/scoped_nsobject.h"
+#include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #include "base/process/process_handle.h"
 #include "base/strings/string16.h"
@@ -57,6 +58,19 @@ namespace {
 // Launch Services Key to run as an agent app, which doesn't launch in the dock.
 NSString* const kLSUIElement = @"LSUIElement";
 
+// When a --app-shim-error is triggered, this is used to record the relative
+// bitness of the shim and Chrome. If they have the same bitness, then the shim
+// failed for another reason.
+// Order must be maintained as this is used in UMA.
+enum AppShimError {
+  APP_SHIM_ERROR_UNKNOWN,                    // No details.
+  APP_SHIM_ERROR_BITNESS_SHIM_32_CHROME_64,  // Common case.
+  APP_SHIM_ERROR_BITNESS_SHIM_64_CHROME_32,  // Reverse case.
+  APP_SHIM_ERROR_BITNESS_BOTH_32,            // Error case.
+  APP_SHIM_ERROR_BITNESS_BOTH_64,            // Error case.
+  APP_SHIM_ERROR_COUNT,
+};
+
 class ScopedCarbonHandle {
  public:
   ScopedCarbonHandle(size_t initial_size) : handle_(NewHandle(initial_size)) {
@@ -496,6 +510,35 @@ web_app::ShortcutInfo BuildShortcutInfoFromBundle(
   return shortcut_info;
 }
 
+void RecordAppShimErrorAndRebuild(const CommandLine& command_line) {

[tapted, 2014/06/26 06:47:05]

maybe rename this to make clear the thread it runs on
RecordAppShimErrorAndRebuild -> RecordAppShimErrorAndRebuildOnBlockingPool

[jackhou1, 2014/06/27 04:55:56]

Moved this to the UI thread.

+  std::string error_detail =
+      command_line.GetSwitchValueASCII(app_mode::kAppShimErrorDetail);
+  int error_enum = APP_SHIM_ERROR_UNKNOWN;
+
+#if defined(ARCH_CPU_32_BITS)
+  if (error_detail == app_mode::kAppShimErrorDetail32Bit)
+    error_enum = APP_SHIM_ERROR_BITNESS_BOTH_32;

[tapted, 2014/06/26 06:47:05]

So.... I think we can determine shim bitness from the plist.

If we read CFBundleShortVersionString from the shim's plist before upgrading it,
and grab Chrome's major version number, that's enough to determine the bitness
for everything except canary/dev: beta and stable should never change bitness
during the major version.

And since we are adding this logic in the shim "now", canary/dev will likely
only ever be able to report 64-bit anyway.

WDYT?

[jackhou1, 2014/06/27 04:55:57]

I think the main benefit would be not having to update the shim binary again. Is
there already a mapping from version to bitness? I wouldn't want to have to
maintain that, unless we can just record the versions into the histogram.

One benefit of passing a command line flag is that we can pass more error types
in the future. Though I'm not sure how likely that is, since it only covers
errors where it can start Chrome but can't dyload the framework.

Ideally we'd inspect the shim binary from Chrome, but I think the only way to do
that is to call "file" in a subprocess.

[tapted, 2014/06/27 05:10:48]

The mapping should be pretty simple:

<38: no
38: maybe
recording those 3 values for the shim version in the histogram is probably
enough.
Yeah - let's get the UMA in first. If we see any "bitness matches but dyload
failed" in m38 beta then we will need to investigate more.
Yeah realised the same, and figured the plist might have this info as well in
some form (i.e. bitness), but Chrome version number was the closest it gets. It
*might* be as simple as reading the first 2 bytes of app_mode_loader but raises
a lot more questions than just reading the plist.

[jackhou1, 2014/07/01 04:30:39]

Okay, changed this to just read the plist. I also forgot previously that the UMA
include Chrome's version, so we only need to track the shim version, which is
pretty simple.

+  else if (error_detail == app_mode::kAppShimErrorDetail64Bit)
+    error_enum = APP_SHIM_ERROR_BITNESS_SHIM_64_CHROME_32;
+#elif defined(ARCH_CPU_64_BITS)
+  if (error_detail == app_mode::kAppShimErrorDetail32Bit)
+    error_enum = APP_SHIM_ERROR_BITNESS_SHIM_32_CHROME_64;
+  else if (error_detail == app_mode::kAppShimErrorDetail64Bit)
+    error_enum = APP_SHIM_ERROR_BITNESS_BOTH_64;
+#endif
+
+  UMA_HISTOGRAM_ENUMERATION("Apps.AppShimError",
+                            error_enum,
+                            APP_SHIM_ERROR_COUNT);
+
+  web_app::ShortcutInfo info = BuildShortcutInfoFromBundle(
+      command_line.GetSwitchValuePath(app_mode::kAppShimError));
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&RebuildAppAndLaunch, info));
+}
+
 void UpdateFileTypes(NSMutableDictionary* plist,
                      const extensions::FileHandlersInfo& file_handlers_info) {
   NSMutableArray* document_types =
@@ -936,12 +979,9 @@ bool MaybeRebuildShortcut(const CommandLine& command_line) {
   if (!command_line.HasSwitch(app_mode::kAppShimError))
     return false;
 
-  base::PostTaskAndReplyWithResult(
-      content::BrowserThread::GetBlockingPool(),
+  content::BrowserThread::GetBlockingPool()->PostTask(
       FROM_HERE,
-      base::Bind(&BuildShortcutInfoFromBundle,
-                 command_line.GetSwitchValuePath(app_mode::kAppShimError)),
-      base::Bind(&RebuildAppAndLaunch));
+      base::Bind(&RecordAppShimErrorAndRebuild, command_line));

[tapted, 2014/06/26 06:47:05]

this will do a deep-copy of the command line object into the closure, which
isn't cheap -- probably cheaper to just call RecordAppShimError(command_line) on
the UI thread before posting the task

[jackhou1, 2014/06/27 04:55:56]

Done.

   return true;
 }