Bug 1288321 - Avoid blocking where possible while waiting for the updater to stage draft
authorMatt Howell <mhowell@mozilla.com>
Tue, 26 Jul 2016 08:39:32 -0700
changeset 393046 2ee24c42ed82e7a76a678922869edd2212f53ce2
parent 392950 ceb63dec9267e9bb62f5e5e1f4c9d32d3ac1fbac
child 526475 e1bb067c22bb5deba78fae8fe91480db68f671bc
push id24194
push usermhowell@mozilla.com
push dateTue, 26 Jul 2016 21:57:38 +0000
bugs1288321
milestone50.0a1
Bug 1288321 - Avoid blocking where possible while waiting for the updater to stage MozReview-Commit-ID: 62FXpuSnwMD
toolkit/xre/nsUpdateDriver.cpp
toolkit/xre/nsUpdateDriver.h
--- a/toolkit/xre/nsUpdateDriver.cpp
+++ b/toolkit/xre/nsUpdateDriver.cpp
@@ -41,41 +41,21 @@
 # include <shlwapi.h>
 # include "nsWindowsHelpers.h"
 # define getcwd(path, size) _getcwd(path, size)
 # define getpid() GetCurrentProcessId()
 #elif defined(XP_UNIX)
 # include <unistd.h>
 #endif
 
-using namespace mozilla;
+#ifdef USE_EXECV
+# include <sys/wait.h>
+#endif
 
-//
-// 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
+using namespace mozilla;
 
 static PRLogModuleInfo *
 GetUpdateLog()
 {
   static PRLogModuleInfo *sUpdateLog;
   if (!sUpdateLog)
     sUpdateLog = PR_NewLogModule("updatedriver");
   return sUpdateLog;
@@ -199,17 +179,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 +376,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
  */
@@ -784,17 +764,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;
@@ -941,18 +921,22 @@ ApplyUpdate(nsIFile *greDir, nsIFile *up
 #endif
 
   LOG(("spawning updater process [%s]\n", updaterPath.get()));
 
 #if defined(USE_EXECV)
   // Don't use execv when staging updates.
   if (restart) {
     execv(updaterPath.get(), argv);
-  } else {
-    *outpid = PR_CreateProcess(updaterPath.get(), argv, nullptr, nullptr);
+  }
+  *outpid = fork();
+  if (*outpid == -1) {
+    return;
+  } else if (*outpid == 0) {
+    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 +964,47 @@ 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(USE_EXECV) || defined(XP_MACOSX)
+  int exitStatus;
+  bool exited = waitpid(pt, &exitStatus, WNOHANG) > 0;
+  if (!exited) {
+    sleep(1);
+    exited = waitpid(pt, 0, WNOHANG) > 0;
+  }
+  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 +1018,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 +1264,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
@@ -11,34 +11,58 @@
 #include "nsIUpdateService.h"
 #include "nsIThread.h"
 #include "nsCOMPtr.h"
 #include "nsString.h"
 #include "mozilla/Attributes.h"
 
 class nsIFile;
 
+//
+// 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
+
 #if defined(XP_WIN)
 #include <windows.h>
   typedef HANDLE     ProcessType;
-#elif defined(XP_MACOSX)
+#elif defined(USE_EXECV) || defined(XP_MACOSX)
   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