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

chrome/app/app_mode_loader_mac.mm

Index: chrome/app/app_mode_loader_mac.mm
diff --git a/chrome/app/app_mode_loader_mac.mm b/chrome/app/app_mode_loader_mac.mm
index 402aea80391101a2660b19e36bedbf6aafcb30c8..5fc24dfbfd0947da53010916c183c4d7a68ea1d0 100644
--- a/chrome/app/app_mode_loader_mac.mm
+++ b/chrome/app/app_mode_loader_mac.mm
@@ -20,7 +20,9 @@
 #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"
 
@@ -67,11 +69,15 @@ int LoadFrameworkAndStart(app_mode::ChromeAppModeInfo* info) {
   }
 
   // ** 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 =
@@ -117,26 +123,36 @@ int LoadFrameworkAndStart(app_mode::ChromeAppModeInfo* info) {
 
   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;
-    }
-
-    return 0;
+  } 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;

[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.