Bug 1772908 - [6/6] Replace polling in UpdateDriver with use of HandleWatcher...

Bug 1772908 - [6/6] Replace polling in UpdateDriver with use of HandleWatcher on Windows r=application-update-reviewers,bytesized

On Windows and on most flavors of Unix (including Linux, but explicitly
excluding macOS), when we wait for an updater-process to terminate, we
poll for its completion rather than reacting asynchronously to some
OS-provided event. The polling loop is _supposed_ to be async-friendly
and interruptible, in that it enqueues its continuation onto its
thread's event queue, rather than directly and synchronously looping.

Unfortunately, the polling loop doesn't idle between poll attempts.
Instead, it calls `sleep()` (or the moral equivalent) directly within
the posted task, and then enqueues its next continuation at the default
priority before concluding. Since shutdown-time teardown is effectively
an idle-priority task, it will never get to run unless the updater
finishes — so an active updater process will always induce a shutdown
hang.

The best fix for this is almost certainly to just _not_ poll, and
instead set up some listener for whatever asynchronous mechanism each OS
provides for this sort of thing.

For Windows, use `HandleWatcher` to do exactly that. (For other OSes,
kick the can down the road.)

Differential Revision: https://phabricator.services.mozilla.com/D159563

toolkit/xre/nsUpdateDriver.cpp

@@ -644,20 +644,17 @@ if (isStaged) {
   }
 }
 
+#if !defined(XP_WIN)
 /**
  * Wait briefly to see if a process terminates, then return true if it has.
+ *
+ * (Not implemented on Windows, where HandleWatcher is used instead.)
  */
 static bool ProcessHasTerminated(ProcessType pt) {
-#if defined(XP_WIN)
-  if (WaitForSingleObject(pt, 1000)) {
-    return false;
-  }
-  CloseHandle(pt);
-  return true;
-#elif defined(XP_MACOSX)
+#  if defined(XP_MACOSX)
   // We're waiting for the process to terminate in LaunchChildMac.
   return true;
-#elif defined(XP_UNIX)
+#  elif defined(XP_UNIX)
   int exitStatus;
   pid_t exited = waitpid(pt, &exitStatus, WNOHANG);
   if (exited == 0) {
@@ -676,7 +673,7 @@ static bool ProcessHasTerminated(ProcessType pt) {
     LOG(("Error while running the updater process, check update.log"));
   }
   return true;
-#else
+#  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;
@@ -685,8 +682,9 @@ static bool ProcessHasTerminated(ProcessType pt) {
     LOG(("Error while running the updater process, check update.log"));
   }
   return true;
-#endif
+#  endif
 }
+#endif
 
 nsresult ProcessUpdates(nsIFile* greDir, nsIFile* appDir, nsIFile* updRootDir,
                         int argc, char** argv, const char* appVersion,
@@ -748,7 +746,11 @@ NS_IMPL_ISUPPORTS(nsUpdateProcessor, nsIUpdateProcessor)
 
 nsUpdateProcessor::nsUpdateProcessor() : mUpdaterPID(0) {}
 
+#ifdef XP_WIN
+nsUpdateProcessor::~nsUpdateProcessor() { mProcessWatcher.Stop(); }
+#else
 nsUpdateProcessor::~nsUpdateProcessor() = default;
+#endif
 
 NS_IMETHODIMP
 nsUpdateProcessor::ProcessUpdate() {
@@ -838,6 +840,17 @@ void nsUpdateProcessor::StartStagedUpdate() {
     return;
   }
 
+#ifdef WIN32
+  // Set up a HandleWatcher to report to the main thread when we're done.
+  RefPtr<nsIThread> mainThread;
+  NS_GetMainThread(getter_AddRefs(mainThread));
+  mProcessWatcher.Watch(mUpdaterPID, mainThread,
+                        NewRunnableMethod("nsUpdateProcessor::UpdateDone", this,
+                                          &nsUpdateProcessor::UpdateDone));
+
+// On Windows, that's all we need the worker thread for. Let
+// `onExitStopThread` shut us down.
+#else
   // Monitor the state of the updater process while it is staging an update.
   rv = NS_DispatchToCurrentThread(
       NewRunnableMethod("nsUpdateProcessor::WaitForProcess", this,
@@ -852,6 +865,7 @@ void nsUpdateProcessor::StartStagedUpdate() {
   // Leave the worker thread alive to run WaitForProcess. Either it or its
   // successors will be responsible for shutting down the worker thread.
   onExitStopThread.release();
+#endif
 }
 
 void nsUpdateProcessor::ShutdownWorkerThread() {
@@ -860,6 +874,7 @@ void nsUpdateProcessor::ShutdownWorkerThread() {
   mWorkerThread = nullptr;
 }
 
+#ifndef WIN32
 void nsUpdateProcessor::WaitForProcess() {
   MOZ_ASSERT(!NS_IsMainThread(), "main thread");
   if (ProcessHasTerminated(mUpdaterPID)) {
@@ -871,6 +886,7 @@ void nsUpdateProcessor::WaitForProcess() {
                           &nsUpdateProcessor::WaitForProcess));
   }
 }
+#endif
 
 void nsUpdateProcessor::UpdateDone() {
   MOZ_ASSERT(NS_IsMainThread(), "not main thread");
@@ -884,7 +900,11 @@ void nsUpdateProcessor::UpdateDone() {
     um->RefreshUpdateStatus(getter_AddRefs(outPromise));
   }
 
+// On Windows, shutting down the worker thread is taken care of by another task.
+// (Which may not have run yet, so we can't assert.)
+#ifndef XP_WIN
   ShutdownWorkerThread();
+#endif
 }
 
 NS_IMETHODIMP

toolkit/xre/nsUpdateDriver.h

@@ -18,6 +18,7 @@ class nsIFile;
 
 #if defined(XP_WIN)
 #  include <windows.h>
+#  include "mozilla/WinHandleWatcher.h"
 typedef HANDLE ProcessType;
 #elif defined(XP_UNIX)
 typedef pid_t ProcessType;
@@ -93,13 +94,19 @@ class nsUpdateProcessor final : public nsIUpdateProcessor {
 
  private:
   void StartStagedUpdate();
-  void WaitForProcess();
   void UpdateDone();
   void ShutdownWorkerThread();
 
+#ifndef XP_WIN
+  void WaitForProcess();
+#endif
+
  private:
   ProcessType mUpdaterPID;
   nsCOMPtr<nsIThread> mWorkerThread;
+#ifdef XP_WIN
+  mozilla::HandleWatcher mProcessWatcher;
+#endif
   StagedUpdateInfo mInfo;
 };
 #endif  // nsUpdateDriver_h__