Bug 1272614 - Avoid blocking where possible while waiting for the updater to stage; r?spohl draft
authorMatt Howell <mhowell@mozilla.com>
Thu, 04 Aug 2016 12:54:01 -0700
changeset 396852 4e8bfd080e6d78cfeb8e022847722c4f9e9651d5
parent 396804 0ba72e8027cfcbcbf3426770ac264a7ade2af090
child 527314 74dfa74ad4936ca852382517b82d0374e97634c9
push id25135
push usermhowell@mozilla.com
push dateThu, 04 Aug 2016 19:54:27 +0000
reviewersspohl
bugs1272614
milestone51.0a1
Bug 1272614 - Avoid blocking where possible while waiting for the updater to stage; r?spohl MozReview-Commit-ID: 1aAU2wxQvMm
toolkit/xre/nsUpdateDriver.cpp
toolkit/xre/nsUpdateDriver.h
--- a/toolkit/xre/nsUpdateDriver.cpp
+++ b/toolkit/xre/nsUpdateDriver.cpp
@@ -39,44 +39,21 @@
 # include <process.h>
 # include <windows.h>
 # include <shlwapi.h>
 # include "nsWindowsHelpers.h"
 # define getcwd(path, size) _getcwd(path, size)
 # define getpid() GetCurrentProcessId()
 #elif defined(XP_UNIX)
 # include <unistd.h>
+# include <sys/wait.h>
 #endif
 
 using namespace mozilla;
 
-//
-// We use execv to spawn the updater process on all UNIX systems except Mac OSX
-// since it is known to cause problems on the Mac.  Windows has execv, but it
-// is a faked implementation that doesn't really replace the current process.
-// Instead it spawns a new process, so we gain nothing from using execv on
-// Windows.
-//
-// On platforms where we are not calling execv, we may need to make the
-// updater executable wait for the calling process to exit.  Otherwise, the
-// updater may have trouble modifying our executable image (because it might
-// still be in use).  This is accomplished by passing our PID to the updater so
-// that it can wait for us to exit.  This is not perfect as there is a race
-// condition that could bite us.  It's possible that the calling process could
-// exit before the updater waits on the specified PID, and in the meantime a
-// new process with the same PID could be created.  This situation is unlikely,
-// however, given the way most operating systems recycle PIDs.  We'll take our
-// chances ;-)
-//
-// A similar #define lives in updater.cpp and should be kept in sync with this.
-//
-#if defined(XP_UNIX) && !defined(XP_MACOSX)
-#define USE_EXECV
-#endif
-
 static PRLogModuleInfo *
 GetUpdateLog()
 {
   static PRLogModuleInfo *sUpdateLog;
   if (!sUpdateLog)
     sUpdateLog = PR_NewLogModule("updatedriver");
   return sUpdateLog;
 }
@@ -199,17 +176,17 @@ GetXULRunnerStubPath(const char* argv0, 
   return NS_OK;
 }
 #endif /* XP_MACOSX */
 
 static bool
 GetFile(nsIFile *dir, const nsCSubstring &name, nsCOMPtr<nsIFile> &result)
 {
   nsresult rv;
-  
+
   nsCOMPtr<nsIFile> file;
   rv = dir->Clone(getter_AddRefs(file));
   if (NS_FAILED(rv))
     return false;
 
   rv = file->AppendNative(name);
   if (NS_FAILED(rv))
     return false;
@@ -396,17 +373,17 @@ CopyUpdaterIntoUpdateDir(nsIFile *greDir
   if (NS_FAILED(tmp)) {
     rv = tmp;
   }
   tmp = updater->AppendNative(NS_LITERAL_CSTRING("MacOS"));
   if (NS_FAILED(tmp) || NS_FAILED(rv))
     return false;
 #endif
   rv = updater->AppendNative(NS_LITERAL_CSTRING(UPDATER_BIN));
-  return NS_SUCCEEDED(rv); 
+  return NS_SUCCEEDED(rv);
 }
 
 /**
  * Appends the specified path to the library path.
  * This is used so that updater can find libmozsqlite3.so and other shared libs.
  *
  * @param pathToAppend A new library path to prepend to LD_LIBRARY_PATH
  */
@@ -588,19 +565,20 @@ SwitchToUpdatedApp(nsIFile *greDir, nsIF
     return;
 
   // Get the current working directory.
   char workingDirPath[MAXPATHLEN];
   rv = GetCurrentWorkingDir(workingDirPath, sizeof(workingDirPath));
   if (NS_FAILED(rv))
     return;
 
-  // Construct the PID argument for this process.  If we are using execv, then
-  // we pass "0" which is then ignored by the updater.
-#if defined(USE_EXECV)
+  // Construct the PID argument for this process. We start the updater using
+  // execv on all Unix platforms except Mac, so on those platforms we pass 0
+  // instead of a good PID to signal the updater not to try and wait for us.
+#if defined(XP_UNIX) & !defined(XP_MACOSX)
   nsAutoCString pid("0");
 #else
   nsAutoCString pid;
   pid.AppendInt((int32_t) getpid());
 #endif
 
   // Append a special token to the PID in order to let the updater know that it
   // just needs to replace the update directory.
@@ -637,24 +615,24 @@ SwitchToUpdatedApp(nsIFile *greDir, nsIF
   }
 #if defined(MOZ_VERIFY_MAR_SIGNATURE) && !defined(XP_WIN) && \
     !defined(XP_MACOSX) && !defined(MOZ_WIDGET_GONK)
   AppendToLibPath(installDirPath.get());
 #endif
 
   LOG(("spawning updater process for replacing [%s]\n", updaterPath.get()));
 
-#if defined(USE_EXECV)
+#if defined(XP_UNIX) & !defined(XP_MACOSX)
 # if defined(MOZ_WIDGET_GONK)
   // In Gonk, we preload libmozglue, which the updater process doesn't need.
   // Since the updater will move and delete libmozglue.so, this can actually
   // stop the /system mount from correctly being remounted as read-only.
   unsetenv("LD_PRELOAD");
 # endif
-  execv(updaterPath.get(), argv);
+  exit(execv(updaterPath.get(), argv));
 #elif defined(XP_WIN)
   // Switch the application using updater.exe
   if (!WinLaunchChild(updaterPathW.get(), argc, argv)) {
     return;
   }
   _exit(0);
 #elif defined(XP_MACOSX)
   CommandLineServiceMac::SetupMacCommandLine(argc, argv, true);
@@ -784,17 +762,17 @@ ApplyUpdate(nsIFile *greDir, nsIFile *up
     return;
   }
   NS_ConvertUTF16toUTF8 updaterPath(updaterPathW);
 #else
   nsAutoCString appFilePath;
   rv = appFile->GetNativePath(appFilePath);
   if (NS_FAILED(rv))
     return;
-  
+
   nsAutoCString updaterPath;
   rv = updater->GetNativePath(updaterPath);
   if (NS_FAILED(rv))
     return;
 
 #endif
 
   nsAutoCString installDirPath;
@@ -863,24 +841,34 @@ ApplyUpdate(nsIFile *greDir, nsIFile *up
     return;
 
   // We used to write out "Applying" to the update.status file here.
   // Instead we do this from within the updater application now.
   // This is so that we don't overwrite the status of pending-service
   // in the Windows case.  This change was made for all platforms so
   // that it stays consistent across all OS.
 
+  // On platforms where we are not calling execv, we may need to make the
+  // updater executable wait for the calling process to exit.  Otherwise, the
+  // updater may have trouble modifying our executable image (because it might
+  // still be in use).  This is accomplished by passing our PID to the updater so
+  // that it can wait for us to exit.  This is not perfect as there is a race
+  // condition that could bite us.  It's possible that the calling process could
+  // exit before the updater waits on the specified PID, and in the meantime a
+  // new process with the same PID could be created.  This situation is unlikely,
+  // however, given the way most operating systems recycle PIDs.  We'll take our
+  // chances ;-)
   // Construct the PID argument for this process.  If we are using execv, then
   // we pass "0" which is then ignored by the updater.
   nsAutoCString pid;
   if (!restart) {
     // Signal the updater application that it should stage the update.
     pid.AssignASCII("-1");
   } else {
-#if defined(USE_EXECV)
+#if defined(XP_UNIX) & !defined(XP_MACOSX)
     pid.AssignASCII("0");
 #else
     pid.AppendInt((int32_t) getpid());
 #endif
   }
 
   int immersiveArgc = 0;
   int argc = appArgc + 6 + immersiveArgc;
@@ -937,22 +925,30 @@ ApplyUpdate(nsIFile *greDir, nsIFile *up
   // Note: we allocate a new string on heap and pass that to PR_SetEnv, since
   // the string can be used after this function returns.  This means that we
   // will intentionally leak this buffer.
   PR_SetEnv(ToNewCString(prioEnv));
 #endif
 
   LOG(("spawning updater process [%s]\n", updaterPath.get()));
 
-#if defined(USE_EXECV)
-  // Don't use execv when staging updates.
+#if defined(XP_UNIX) && !defined(XP_MACOSX)
+  // We use execv to spawn the updater process on all UNIX systems except Mac OSX
+  // since it is known to cause problems on the Mac.  Windows has execv, but it
+  // is a faked implementation that doesn't really replace the current process.
+  // Instead it spawns a new process, so we gain nothing from using execv on
+  // Windows.
   if (restart) {
-    execv(updaterPath.get(), argv);
-  } else {
-    *outpid = PR_CreateProcess(updaterPath.get(), argv, nullptr, nullptr);
+    exit(execv(updaterPath.get(), argv));
+  }
+  *outpid = fork();
+  if (*outpid == -1) {
+    return;
+  } else if (*outpid == 0) {
+    exit(execv(updaterPath.get(), argv));
   }
 #elif defined(XP_WIN)
   // Launch the update using updater.exe
   if (!WinLaunchChild(updaterPathW.get(), argc, argv, nullptr, outpid)) {
     return;
   }
 
   if (restart) {
@@ -980,32 +976,45 @@ ApplyUpdate(nsIFile *greDir, nsIFile *up
   *outpid = PR_CreateProcess(updaterPath.get(), argv, nullptr, nullptr);
   if (restart) {
     exit(0);
   }
 #endif
 }
 
 /**
- * Wait for a process until it terminates.  This call is blocking.
+ * Wait briefly to see if a process terminates, then return true if it has.
  */
-static void
-WaitForProcess(ProcessType pt)
+static bool
+ProcessHasTerminated(ProcessType pt)
 {
 #if defined(XP_WIN)
-  WaitForSingleObject(pt, INFINITE);
+  if (WaitForSingleObject(pt, 1000)) {
+    return false;
+  }
   CloseHandle(pt);
-#elif defined(XP_MACOSX)
-  waitpid(pt, 0, 0);
+  return true;
+#elif defined(XP_UNIX)
+  int exitStatus;
+  bool exited = waitpid(pt, &exitStatus, WNOHANG) > 0;
+  if (!exited) {
+    sleep(1);
+  } else if (WIFEXITED(exitStatus) && (WEXITSTATUS(exitStatus) != 0)) {
+    LOG(("Error while running the updater process, check update.log"));
+  }
+  return exited;
 #else
+  // No way to have a non-blocking implementation on these platforms,
+  // because we're using NSPR and it only provides a blocking wait.
   int32_t exitCode;
   PR_WaitProcess(pt, &exitCode);
   if (exitCode != 0) {
     LOG(("Error while running the updater process, check update.log"));
   }
+  return true;
 #endif
 }
 
 nsresult
 ProcessUpdates(nsIFile *greDir, nsIFile *appDir, nsIFile *updRootDir,
                int argc, char **argv, const char *appVersion,
                bool restart, bool isOSUpdate, nsIFile *osApplyToDir,
                ProcessType *pid)
@@ -1019,17 +1028,17 @@ ProcessUpdates(nsIFile *greDir, nsIFile 
 
   rv = updatesDir->AppendNative(NS_LITERAL_CSTRING("updates"));
   if (NS_FAILED(rv))
     return rv;
 
   rv = updatesDir->AppendNative(NS_LITERAL_CSTRING("0"));
   if (NS_FAILED(rv))
     return rv;
- 
+
   nsCOMPtr<nsIFile> statusFile;
   UpdateStatus status = GetUpdateStatus(updatesDir, statusFile);
   switch (status) {
   case ePendingElevate: {
     if (NS_IsMainThread()) {
       // Only do this if we're called from the main thread.
       nsCOMPtr<nsIUpdatePrompt> up =
         do_GetService("@mozilla.org/updates/update-prompt;1");
@@ -1265,26 +1274,28 @@ nsUpdateProcessor::ShutdownWatcherThread
   mProcessWatcher->Shutdown();
   mProcessWatcher = nullptr;
 }
 
 void
 nsUpdateProcessor::WaitForProcess()
 {
   MOZ_ASSERT(!NS_IsMainThread(), "main thread");
-  ::WaitForProcess(mUpdaterPID);
-  NS_DispatchToMainThread(NewRunnableMethod(this, &nsUpdateProcessor::UpdateDone));
+  if (ProcessHasTerminated(mUpdaterPID)) {
+    NS_DispatchToMainThread(NewRunnableMethod(this, &nsUpdateProcessor::UpdateDone));
+  } else {
+    NS_DispatchToCurrentThread(NewRunnableMethod(this, &nsUpdateProcessor::WaitForProcess));
+  }
 }
 
 void
 nsUpdateProcessor::UpdateDone()
 {
   MOZ_ASSERT(NS_IsMainThread(), "not main thread");
 
   nsCOMPtr<nsIUpdateManager> um =
     do_GetService("@mozilla.org/updates/update-manager;1");
   if (um) {
     um->RefreshUpdateStatus();
   }
 
   ShutdownWatcherThread();
 }
-
--- a/toolkit/xre/nsUpdateDriver.h
+++ b/toolkit/xre/nsUpdateDriver.h
@@ -14,31 +14,31 @@
 #include "nsString.h"
 #include "mozilla/Attributes.h"
 
 class nsIFile;
 
 #if defined(XP_WIN)
 #include <windows.h>
   typedef HANDLE     ProcessType;
-#elif defined(XP_MACOSX)
+#elif defined(XP_UNIX)
   typedef pid_t      ProcessType;
 #else
 #include "prproces.h"
   typedef PRProcess* ProcessType;
 #endif
 
 /**
  * This function processes any available updates.  As part of that process, it
  * may exit the current process and relaunch it at a later time.
  *
  * Two directories are passed to this function: greDir (where the actual
  * binary resides) and appDir (which contains application.ini for XULRunner
  * apps). If this is not a XULRunner app then appDir is identical to greDir.
- * 
+ *
  * The argc and argv passed to this function should be what is needed to
  * relaunch the current process.
  *
  * The appVersion param passed to this function is the current application's
  * version and is used to determine if an update's version is older than the
  * current application version.
  *
  * If you want the update to be processed without restarting, set the restart