Add UMA for --app-shim-error.

chrome/browser/web_applications/web_app_mac.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/web_applications/web_app_mac.h"
 
 #import <Carbon/Carbon.h>
 #import <Cocoa/Cocoa.h>
 
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/launch_services_util.h"
 #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"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #import "chrome/browser/mac/dock.h"
@@ skipping 21 matching lines @@
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image_family.h"
 
 bool g_app_shims_allow_update_and_launch_in_tests = false;
 
 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)) {
     DCHECK(handle_);
     DCHECK_EQ(noErr, MemError());
   }
   ~ScopedCarbonHandle() { DisposeHandle(handle_); }
 
   Handle Get() { return handle_; }
   char* Data() { return *handle_; }
@@ skipping 419 matching lines @@
   base::FilePath profile_base_name = base::mac::NSStringToFilePath(
       [plist valueForKey:app_mode::kCrAppModeProfileDirKey]);
   if (user_data_dir.DirName().DirName().BaseName() == profile_base_name)
     shortcut_info.profile_path = user_data_dir.DirName().DirName();
   else
     shortcut_info.profile_path = user_data_dir.Append(profile_base_name);
 
   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 =
       [NSMutableArray arrayWithCapacity:file_handlers_info.size()];
 
   for (extensions::FileHandlersInfo::const_iterator info_it =
            file_handlers_info.begin();
        info_it != file_handlers_info.end();
        ++info_it) {
     const extensions::FileHandlerInfo& info = *info_it;
@@ skipping 420 matching lines @@
   content::BrowserThread::PostTask(
       content::BrowserThread::FILE,
       FROM_HERE,
       base::Bind(&LaunchShimOnFileThread, shortcut_info, false));
 }
 
 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;
 }
 
 // Called when the app's ShortcutInfo (with icon) is loaded when creating app
 // shortcuts.
 void CreateAppShortcutInfoLoaded(
     Profile* profile,
     const extensions::Extension* app,
     const base::Callback<void(bool)>& close_callback,
     const ShortcutInfo& shortcut_info) {
@@ skipping 134 matching lines @@
   web_app::UpdateShortcutInfoAndIconForApp(
       app,
       profile,
       base::Bind(&web_app::CreateAppShortcutInfoLoaded,
                  profile,
                  app,
                  close_callback));
 }
 
 }  // namespace chrome