[Mac] Rebuild app shims when they fail to dyload Chrome Framework.

chrome/app/app_mode_loader_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.
 
 // On Mac, shortcuts can't have command-line arguments. Instead, produce small
 // app bundles which locate the Chromium framework and load it, passing the
 // appropriate data. This is the code for such an app bundle. It should be kept
 // minimal and do as little work as possible (with as much work done on
 // framework side as possible).
 
 #include <dlfcn.h>
 
 #include <CoreFoundation/CoreFoundation.h>
 #import <Foundation/Foundation.h>
 
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/launch_services_util.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
+#include "base/process/launch.h"
 #include "base/strings/sys_string_conversions.h"
+#include "chrome/common/chrome_switches.h"
 #import "chrome/common/mac/app_mode_chrome_locator.h"
 #include "chrome/common/mac/app_mode_common.h"
 
 namespace {
 
 typedef int (*StartFun)(const app_mode::ChromeAppModeInfo*);
 
 int LoadFrameworkAndStart(app_mode::ChromeAppModeInfo* info) {
   using base::SysNSStringToUTF8;
   using base::SysNSStringToUTF16;
@@ skipping 26 matching lines @@
   if (!found_bundle) {
     // If no such bundle path exists, try to search by bundle ID.
     if (!app_mode::FindBundleById(cr_bundle_id, &cr_bundle_path)) {
       // TODO(jeremy): Display UI to allow user to manually locate the Chrome
       // bundle.
       LOG(FATAL) << "Failed to locate bundle by identifier";
     }
   }
 
   // ** 2: Read information from the Chrome bundle.
+  base::FilePath executable_path;
   base::string16 raw_version_str;
   base::FilePath version_path;
   base::FilePath framework_shlib_path;
-  if (!app_mode::GetChromeBundleInfo(cr_bundle_path, &raw_version_str,
-          &version_path, &framework_shlib_path)) {
+  if (!app_mode::GetChromeBundleInfo(cr_bundle_path,
+                                     &executable_path,
+                                     &raw_version_str,
+                                     &version_path,
+                                     &framework_shlib_path)) {
     LOG(FATAL) << "Couldn't ready Chrome bundle info";
   }
   base::FilePath app_mode_bundle_path =
       base::mac::NSStringToFilePath([app_bundle bundlePath]);
 
   // ** 3: Fill in ChromeAppModeInfo.
   info->chrome_outer_bundle_path = cr_bundle_path;
   info->chrome_versioned_path = version_path;
   info->app_mode_bundle_path = app_mode_bundle_path;
 
@@ skipping 23 matching lines @@
   void* cr_dylib = dlopen(framework_shlib_path.value().c_str(), RTLD_LAZY);
   if (cr_dylib) {
     // Find the entry point.
     ChromeAppModeStart = (StartFun)dlsym(cr_dylib, "ChromeAppModeStart");
     if (!ChromeAppModeStart)
       LOG(ERROR) << "Couldn't get entry point: " << dlerror();
   } else {
     LOG(ERROR) << "Couldn't load framework: " << dlerror();
   }
 
   if (ChromeAppModeStart) {

[tapted, 2014/05/05 07:02:01]

nit: doesn't need curlies

[jackhou1, 2014/05/06 04:47:58]

Done.

     return ChromeAppModeStart(info);
-  } else {
-    LOG(ERROR) << "Loading Chrome failed, launching with command line.";
-    // Launch Chrome instead and have it update this app_mode_loader bundle.
-    CommandLine command_line(CommandLine::NO_PROGRAM);
+  }
+
+  LOG(ERROR) << "Loading Chrome failed, launching Chrome with command line";
+  CommandLine command_line(executable_path);
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+        app_mode::kLaunchedByChromeProcessId) ||
+      info->app_mode_id == app_mode::kAppListModeId) {
+    // Pass --app-shim-error to have Chrome rebuild this shim.
     command_line.AppendSwitchPath(app_mode::kAppShimError,
                                   app_mode_bundle_path);
-    if (!base::mac::OpenApplicationWithPath(
-            cr_bundle_path, command_line, kLSLaunchDefaults, NULL)) {
-      LOG(ERROR) << "Could not launch Chrome from: " << cr_bundle_path.value();
-      return 1;
-    }
+  } else {
+    // Just launch the app.

[tapted, 2014/05/05 07:02:01]

nit: maybe clarify why this branching exists and that it will ultimately come
back to the above case. E.g. ~"If the shim was launched directly (instead of by
Chrome), first ask Chrome to launch the app. Chrome will launch the shim again
(in parallel). The same error will occur and be handled above. This approach
makes the launch more responsive/robust/othereasons.."

[jackhou1, 2014/05/06 04:47:58]

Done.

+    command_line.AppendSwitchPath(switches::kProfileDirectory,
+                                  info->profile_dir);
+    command_line.AppendSwitchASCII(switches::kAppId, info->app_mode_id);
+  }
+  if (!base::LaunchProcess(command_line, base::LaunchOptions(), NULL)) {

[tapted, 2014/05/05 07:02:01]

why was this change needed? (i.e. changing from
base::mac::OpenApplicationWithPath)

[jackhou1, 2014/05/06 04:47:58]

Updated the comment. LSOpenApplication doesn't pass the command line properly.

+    LOG(ERROR) << "Could not launch Chrome: "
+               << command_line.GetCommandLineString();
+    return 1;
+  }
 
-    return 0;
-  }
+  return 0;

[tapted, 2014/05/05 07:02:01]

Maybe really unlikely.. but is there a potential race condition here if this
process stays alive? i.e. Could the shim re-launch be blocked due to the way
LSOpenApplication works, e.g. by putting a big sleep before this return?

[jackhou1, 2014/05/06 04:47:58]

Yeah there is, but I don't think it's worth trying to handle. Chrome would have
to start up, connect to the process singleton, and launch the app before this
shuts down.

 }
 
 } // namespace
 
 __attribute__((visibility("default")))
 int main(int argc, char** argv) {
+  CommandLine::Init(argc, argv);

[tapted, 2014/05/05 07:02:01]

Hm - this will be called again at ChromeAppModeStart and it will "fail" (return
false). But of course we can't remove that second call or it will break working
shims. Just pondering whether there are any downsides to this, but I think it's
fine.

   app_mode::ChromeAppModeInfo info;
 
   // Hard coded info parameters.
   info.major_version = app_mode::kCurrentChromeAppModeInfoMajorVersion;
   info.minor_version = app_mode::kCurrentChromeAppModeInfoMinorVersion;
   info.argc = argc;
   info.argv = argv;
 
   // Exit instead of returning to avoid the the removal of |main()| from stack
   // backtraces under tail call optimization.
   exit(LoadFrameworkAndStart(&info));
 }