Bug 1658711 - Ensure that the PostUpdate task is called reliably r=mhowell

The PostUpdate task must always be called as the unelevated user, even if we didn't use the service, in order to ensure that we register the WDBA. Additionally, the PostUpdate task should always be run synchronously so that the elevated and unelevated PostUpdate tasks are guaranteed to run in order. This is important since the elevated PostUpdate unregisters the task and the unelevated PostUpdate re-registers it.

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

browser/installer/windows/nsis/shared.nsh

@@ -193,12 +193,12 @@ ${If} $TmpVal == "HKCU"
                     "DidRegisterDefaultBrowserAgent"
   ${If} $0 != 0
   ${OrIf} ${Errors}
-    Exec '"$INSTDIR\default-browser-agent.exe" register-task $AppUserModelID'
+    ExecWait '"$INSTDIR\default-browser-agent.exe" register-task $AppUserModelID'
   ${EndIf}
 ${ElseIf} $TmpVal == "HKLM"
   ; If we're the privileged PostUpdate, make sure that the unprivileged one
   ; will have permission to create a task by clearing out the old one first.
-  Exec '"$INSTDIR\default-browser-agent.exe" unregister-task $AppUserModelID'
+  ExecWait '"$INSTDIR\default-browser-agent.exe" unregister-task $AppUserModelID'
 ${EndIf}
 !endif
 

toolkit/mozapps/update/updater/updater.cpp

@@ -1979,8 +1979,6 @@ bool LaunchWinPostProcess(const WCHAR* installationDir,
 
   WCHAR exefile[MAX_PATH + 1];
   WCHAR exearg[MAX_PATH + 1];
-  WCHAR exeasync[10];
-  bool async = true;
   if (!GetPrivateProfileStringW(L"PostUpdateWin", L"ExeRelPath", nullptr,
                                 exefile, MAX_PATH + 1, inifile)) {
     return false;
@@ -1991,12 +1989,6 @@ bool LaunchWinPostProcess(const WCHAR* installationDir,
     return false;
   }
 
-  if (!GetPrivateProfileStringW(
-          L"PostUpdateWin", L"ExeAsync", L"TRUE", exeasync,
-          sizeof(exeasync) / sizeof(exeasync[0]), inifile)) {
-    return false;
-  }
-
   // The relative path must not contain directory traversals, current directory,
   // or colons.
   if (wcsstr(exefile, L"..") != nullptr || wcsstr(exefile, L"./") != nullptr ||
@@ -2057,11 +2049,6 @@ bool LaunchWinPostProcess(const WCHAR* installationDir,
   wcsncpy(cmdline, dummyArg, len);
   wcscat(cmdline, exearg);
 
-  if (sUsingService || !_wcsnicmp(exeasync, L"false", 6) ||
-      !_wcsnicmp(exeasync, L"0", 2)) {
-    async = false;
-  }
-
   // We want to launch the post update helper app to update the Windows
   // registry even if there is a failure with removing the uninstall.update
   // file or copying the update.log file.
@@ -2080,9 +2067,7 @@ bool LaunchWinPostProcess(const WCHAR* installationDir,
                            workingDirectory, &si, &pi);
   free(cmdline);
   if (ok) {
-    if (!async) {
+    WaitForSingleObject(pi.hProcess, INFINITE);
-      WaitForSingleObject(pi.hProcess, INFINITE);
-    }
     CloseHandle(pi.hProcess);
     CloseHandle(pi.hThread);
   }
@@ -3318,26 +3303,6 @@ int NS_main(int argc, NS_tchar** argv) {
         return 0;
       }
 
-#  ifdef MOZ_MAINTENANCE_SERVICE
-      // If we started the service command, and it finished, check the secure
-      // update status file to make sure that it succeeded, and if it did we
-      // need to launch the PostUpdate process in the unelevated updater which
-      // is running in the current user's session. Note that we don't need to do
-      // this when staging an update since the PostUpdate step runs during the
-      // replace request.
-      if (useService && !sStagedUpdate) {
-        bool updateStatusSucceeded = false;
-        if (IsSecureUpdateStatusSucceeded(updateStatusSucceeded) &&
-            updateStatusSucceeded) {
-          if (!LaunchWinPostProcess(gInstallDirPath, gPatchDirPath)) {
-            fprintf(stderr,
-                    "The post update process which runs as the user"
-                    " for service update could not be launched.");
-          }
-        }
-      }
-#  endif
-
       // If we didn't want to use the service at all, or if an update was
       // already happening, or launching the service command failed, then
       // launch the elevated updater.exe as we do without the service.
@@ -3393,10 +3358,23 @@ int NS_main(int argc, NS_tchar** argv) {
         }
       }
 
-      // Note: The PostUpdate process is launched by the elevated updater which
+      // If we started the elevated updater, and it finished, check the secure
-      // is running in the current user's session when the update is successful
+      // update status file to make sure that it succeeded, and if it did we
-      // and doesn't need to be launched here by the unelevated updater as is
+      // need to launch the PostUpdate process in the unelevated updater which
-      // done when the maintenance service launches the updater code above.
+      // is running in the current user's session. Note that we don't need to do
+      // this when staging an update since the PostUpdate step runs during the
+      // replace request.
+      if (!sStagedUpdate) {
+        bool updateStatusSucceeded = false;
+        if (IsSecureUpdateStatusSucceeded(updateStatusSucceeded) &&
+            updateStatusSucceeded) {
+          if (!LaunchWinPostProcess(gInstallDirPath, gPatchDirPath)) {
+            fprintf(stderr,
+                    "The post update process which runs as the user"
+                    " for service update could not be launched.");
+          }
+        }
+      }
 
       CloseHandle(elevatedFileHandle);