711692 - Various intermittent xpcshell test failures for maintenance service updates

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

Get rid of the NS_ERROR_FILE_IS_LOCKED errors that happen sometimes from within getAppConsoleLogPath by making the path unique per test.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Not positive this will get rid of them all but it at least should get rid of some of these errors

Comment 2

[Brian R. Bondy [:bbondy]]

Comment on attachment 582483 [details] [diff] [review]
patch rev1

Review of attachment 582483 [details] [diff] [review]:
-----------------------------------------------------------------

Splendid!

Comment 3

[Robert Strong (they/them - no direct email)]

TEST_ID came up as undefined on one of my runs of test_0200_app_launch_apply_update_svc.js so I put that back the way it was so if things are good we will have some green runs by the morning.

https://hg.mozilla.org/projects/elm/rev/304f3f6c6a97

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: Supplemental - Use TEST_ID in getAppConsoleLogPath

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch for not causing failures on remove() calls. rev2

Re-uploading your base patch because the one on elm was correct and had a newer rev.  I.e. on elm it did not have TEST_ID in this patch.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Intermittent callback app log fix

I know you are not a fan of running this sync but I don't think there is any harm in doing so and gets rid of an intermittent failure.  If you want though I can look into this more and find a better way to fix.

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 583263 [details] [diff] [review]
Intermittent callback app log fix

pinged bbondy on irc to also fix the comment

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Intermittent callback app log fix v2.

v2 now with added comment goodness

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 583270 [details] [diff] [review]
Intermittent callback app log fix v2.

I'm fine with this but keep this in mind if there are other weird errors that start happening

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Loop on app bin copy to ensure it can be removed. v1.

Comment 11

[Brian R. Bondy [:bbondy]]

https://tbpl.mozilla.org/?tree=Elm&rev=82a5d376a1fd  Please see the comment in the xpcshell test also.

Comment 12

[Brian R. Bondy [:bbondy]]

Attachment: Wait for app bin callback to finish, loop. v1.

This last patch "Loop on app bin copy to ensure it can be removed. v1." didn't really do anything since do_timeout will run async and the copyTo after the remove was executed right away.

The new patch waits for the bootstrap app bin to finish before test_0110 runs which I think is the real problem here.

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 583478 [details] [diff] [review]
Wait for app bin callback to finish, loop. v1.

># HG changeset patch
># Parent fb10fb5c8d4eba5c07e57954bfb82095766cefb4
># User Brian R. Bondy <[e-mail]>
>Bug 711692 - Loop on app bin copy to ensure it can be removed (intermittent test failure)
>
>diff --git a/toolkit/mozapps/update/test_svc/unit/test_0000_bootstrap_svc.js b/toolkit/mozapps/update/test_svc/unit/test_0000_bootstrap_svc.js
>--- a/toolkit/mozapps/update/test_svc/unit/test_0000_bootstrap_svc.js
>+++ b/toolkit/mozapps/update/test_svc/unit/test_0000_bootstrap_svc.js
>@@ -35,9 +35,14 @@ function run_test() {
> 
>   // apply the complete mar
>   runUpdateUsingService(STATE_PENDING_SVC, STATE_SUCCEEDED, checkUpdateApplied, null, false);
> }
> 
> function checkUpdateApplied() {
>   checkFilesAfterUpdateSuccess();
>   do_test_finished();
>+
>+  // We need to check the service log even know this is a bootstrap
I think you mean though when you wrote know.

>+  // because the app bin could be in use by this test by the time the next
>+  // test runs.
>+  checkCallbackServiceLog();
> }

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Wait for app bin callback to finish, loop. v2.

Fixed grammar. Added a couple of logging statements for stopping the service too which should show up in the purple we're getting.  Carrying forward r+.

Comment 15

[Robert Strong (they/them - no direct email)]

Hey Brian,

I just noticed that the following can be removed

diff --git a/toolkit/mozapps/update/test_svc/unit/test_0000_bootstrap_svc.js b/toolkit/mozapps/update/test_svc/unit/test_0000_bootstrap_svc.js
--- a/toolkit/mozapps/update/test_svc/unit/test_0000_bootstrap_svc.js
+++ b/toolkit/mozapps/update/test_svc/unit/test_0000_bootstrap_svc.js
@@ -25,19 +25,16 @@ function run_test() {
     return;
   }
 
   do_test_pending();
   do_register_cleanup(cleanupUpdaterTest);
 
   setupUpdaterTest(MAR_COMPLETE_FILE);
 
-  let updatesDir = do_get_file(TEST_ID + UPDATES_DIR_SUFFIX);
-  let applyToDir = getApplyDirFile();
-
   // apply the complete mar
   runUpdateUsingService(STATE_PENDING_SVC, STATE_SUCCEEDED, checkUpdateApplied, null, false);
 }
 
 function checkUpdateApplied() {
   checkFilesAfterUpdateSuccess();
   do_test_finished();
 
diff --git a/toolkit/mozapps/update/test_svc/unit/test_0110_general_svc.js b/toolkit/mozapps/update/test_svc/unit/test_0110_general_svc.js
--- a/toolkit/mozapps/update/test_svc/unit/test_0110_general_svc.js
+++ b/toolkit/mozapps/update/test_svc/unit/test_0110_general_svc.js
@@ -273,18 +273,16 @@ function run_test() {
     return;
   }
 
   do_test_pending();
   do_register_cleanup(cleanupUpdaterTest);
 
   setupUpdaterTest(MAR_COMPLETE_FILE);
 
-  let updatesDir = do_get_file(TEST_ID + UPDATES_DIR_SUFFIX);
-
   // apply the complete mar
   runUpdateUsingService(STATE_PENDING_SVC, STATE_SUCCEEDED, checkUpdateApplied);
 }
 
 function checkUpdateApplied() {
   logTestInfo("testing update.status should be " + STATE_SUCCEEDED);
   let updatesDir = do_get_file(TEST_ID + UPDATES_DIR_SUFFIX);
   do_check_eq(readStatusFile(updatesDir), STATE_SUCCEEDED);

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Cleanup tests. Patch v1.

Comment 17

[Robert Strong (they/them - no direct email)]

Comment on attachment 583573 [details] [diff] [review]
Cleanup tests. Patch v1.

I thought this would be a good ride along for a future patch but this is fine

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: fail if service is started and doesn't stop at test art

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: No event synchronization, no uninstall service just replace bin. Patch v1.

Comment 20

[Brian R. Bondy [:bbondy]]

Comment on attachment 583677 [details] [diff] [review]
fail if service is started and doesn't stop at test art

Review of attachment 583677 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  
Good idea with failing the test at the start and not at the end.

Comment 21

[Brian R. Bondy [:bbondy]]

Attachment: No event synchronization, no uninstall service just replace bin. Patch v2.

Was always returning false from service stop function which caused upgrades of service to not work. Fixed in this v2 patch.

Comment 22

[Brian R. Bondy [:bbondy]]

Attachment: Wait for app bin callback to finish, loop. v3.

Carrying forward r+.

The original fix was for the bootstrap waiting for the callback app log.
But it wasn't waiting because do_test_finished() was called before checkCallbackServiceLog() from test_0000.

Also checkCallbackServiceLog() itself calls do_test_finished so it was being called twice.

I think this will fix the  service running intermittent errors.

Comment 23

[Brian R. Bondy [:bbondy]]

Attachment: SCM freeze race condition and gAppBin prefix

There was a race condition where the SCM would become locked and

Callstack would be stuck in maintenanceservice.cpp in first thread at:
> WaitForSingleObject(ghSvcStopEvent, INFINITE); 

And on second thread at:
> gSvcStatusHandle = RegisterServiceCtrlHandlerW(SVC_NAME, SvcCtrlHandler);



To fix this I no longer open the SCM at all to close the service from the service, instead I just set the event ghSvcStopEvent.

Also attempted to fix the gAppBin by always naming it with a prefix of the TEST_ID.

Comment 24

[Brian R. Bondy [:bbondy]]

Attachment: Purple fixer

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 583716 [details] [diff] [review]
No event synchronization, no uninstall service just replace bin. Patch v2.

> #define LOGS_TO_KEEP 5
>diff --git a/toolkit/components/maintenanceservice/serviceinstall.cpp b/toolkit/components/maintenanceservice/serviceinstall.cpp
>--- a/toolkit/components/maintenanceservice/serviceinstall.cpp
>+++ b/toolkit/components/maintenanceservice/serviceinstall.cpp
>...
>+  if (!serviceAlreadyExists) {
>+    // Create the service as on demand
>+    schService.own(CreateServiceW(schSCManager, SVC_NAME, SVC_DISPLAY_NAME,
>+                                  SERVICE_ALL_ACCESS, SERVICE_WIN32_OWN_PROCESS,
>+                                  SERVICE_DEMAND_START, SERVICE_ERROR_NORMAL,
>+                                  newServiceBinaryPath, NULL, NULL, NULL, 
>+                                  NULL, NULL));
>+    if (!schService) {
>+      LOG(("Could not create Windows service. "
>+           "This error should never happen since a service install "
>+           "should only be called when elevated. (%d)\n", GetLastError()));
>+      return FALSE;
>+    } 
> 
>-  if (!SetUserAccessServiceDACL(schService)) {
>-    LOG(("Could not set security ACE on service handle, the service will not be "
>-         "able to be started from unelevated processes. "
>-         "This error should never happen.  (%d)\n", 
>-         GetLastError()));
>+    if (!SetUserAccessServiceDACL(schService)) {
>+      LOG(("Could not set security ACE on service handle, the service will not be "
>+           "able to be started from unelevated processes. "
>+           "This error should never happen.  (%d)\n", 
>+           GetLastError()));
>+    }
>   }
I'm not convinced one way or the other whether we should always set the DACL. On the one hand, if the permissions are changed this would fix them. On the other hand, the admin might want to change the permissions. I think this is the right way to go.

>@@ -266,18 +272,17 @@ StopService()
>   if (!schService) {
>     LOG(("Could not open service.  (%d)\n", GetLastError()));
>     return FALSE;
>   } 
> 
>   // Stop logging before stopping the service.
>   LogFinish();
> 
Please double check that the schService handle is closed;

>diff --git a/toolkit/components/maintenanceservice/workmonitor.cpp b/toolkit/components/maintenanceservice/workmonitor.cpp
>--- a/toolkit/components/maintenanceservice/workmonitor.cpp
>+++ b/toolkit/components/maintenanceservice/workmonitor.cpp
>@@ -390,20 +385,22 @@ ExecuteServiceCommand(int argc, LPWSTR *
>   }
>   LOG(("Executing service command %ls, ID: %ls\n", 
>        argv[2], reinterpret_cast<LPCWSTR>(guidString)));
>   RpcStringFree(&guidString);
> 
>   BOOL result = FALSE;
>   if (!lstrcmpi(argv[2], L"software-update")) {
>     result = ProessSoftwareUpdateCommand(argc - 3, argv + 3);
>+    // We may not reach here if the service install succeeded
s/We may/We might/

>+    // because the service self updates itself and the service
>+    // installer will stop the service.
How is a software-update performed in this scenario?

>     LOG(("Service command %ls complete.\n", argv[2]));

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: No event synchronization, no uninstall service just replace bin. Patch v3.

Carrying forward, fixed 2 nits.

>+    // because the service self updates itself and the service
>+    // installer will stop the service.
> How is a software-update performed in this scenario?

In this scenario the service gets stopped from the maintenance service installer.
So the maintenance service executes the maintenance service installer after the post-update process.

Comment 27

[Brian R. Bondy [:bbondy]]

Attachment: Various fixes for intermittent failures

Lots of robustness changes and some other fixes.
https://tbpl.mozilla.org/?tree=Elm&rev=681dee87aa55

Comment 28

[Brian R. Bondy [:bbondy]]

Attachment: Various fixes for intermittent failures

Added the 2 new maintenance service error codes to nsUpdateService.js

Comment 29

[Robert Strong (they/them - no direct email)]

Comment on attachment 585414 [details] [diff] [review]
Various fixes for intermittent failures

>diff --git a/browser/installer/windows/nsis/maintenanceservice_installer.nsi b/browser/installer/windows/nsis/maintenanceservice_installer.nsi
>--- a/browser/installer/windows/nsis/maintenanceservice_installer.nsi
>+++ b/browser/installer/windows/nsis/maintenanceservice_installer.nsi
>...
>@@ -268,11 +259,10 @@ Section "Uninstall"
>   Push "$INSTDIR\Uninstall.exe"
>   Call un.RenameDelete
>   RMDir /REBOOTOK "$INSTDIR"
> 
>   DeleteRegKey HKLM "${MaintUninstallKey}"
> 
>   SetRegView 64
>   DeleteRegValue HKLM "Software\Mozilla\MaintenanceService" "Installed"
>-  DeleteRegKey HKLM "${FallbackKey}\"
Why does this need to be removed?

>   SetRegView lastused
> SectionEnd
>diff --git a/toolkit/components/maintenanceservice/maintenanceservice.cpp b/toolkit/components/maintenanceservice/maintenanceservice.cpp
>--- a/toolkit/components/maintenanceservice/maintenanceservice.cpp
>+++ b/toolkit/components/maintenanceservice/maintenanceservice.cpp
>@@ -226,88 +213,116 @@ BackupOldLogs(LPCWSTR basePath, int numL
>     if (!GetBackupLogPath(oldPath, basePath, i -1)) {
>       continue;
>     }
> 
>     if (!GetBackupLogPath(newPath, basePath, i)) {
>       continue;
>     }
> 
>-    if (!MoveFileEx(oldPath, newPath, MOVEFILE_REPLACE_EXISTING)) {
>+    if (!MoveFileExW(oldPath, newPath, MOVEFILE_REPLACE_EXISTING)) {
>       continue;
>     }
>   }
> }
> 
> /**
>+ * Ensures the service is shutdown once all work is complete.
>+ * There is an issue on XP SP2 and below where the service can hang
>+ * in a stop pending state even know the SCM is notified of a stopped
s/know/though/

>+ * state.  On a stopped state, control *should* be returned from the call to
>+ * StartServiceCtrlDispatcher in the wmain thread.  Sometimes this is not the
>+ * case though. This thread will terminate the process if it has been 5
nit: this could be better written though the following isn't ideal either.
Control should be returned from the call to StartServiceCtrlDispatcher in the wmain thread When the state is stopped but intermittently it does not return control.

>+ * seconds after all work is done and the process is still not terminated.
>+ * This thread is only started once a stopped state was sent to the SCM.
>+ * The stop pending hang can be reproduced intermittently even if you set a
>+ * stopped state dirctly and never set a stop pending state.
>+ * It is safe to forcefully terminate the process ourselves since all work 
>+ * is done once we start this thread.
>+*/
>+DWORD WINAPI
>+EnsureProcessTerminatedThread(LPVOID) 
>+{
>+  Sleep(5000);
>+  exit(0);
>+  return 0;
>+}
>+

>diff --git a/toolkit/mozapps/update/common/updatehelper.cpp b/toolkit/mozapps/update/common/updatehelper.cpp
>--- a/toolkit/mozapps/update/common/updatehelper.cpp
>+++ b/toolkit/mozapps/update/common/updatehelper.cpp
>...
> /**
>  * Executes a maintenance service command
>  * 
>  * @param  argc    The total number of arguments in argv
>  * @param  argv    An array of null terminated strings to pass to the service, 
>- * @return TRUE if the service command was started.
>+ * @return ERROR_SUCCESS if the service command was started.
There are several possible error values returned that should be commented on here.

> */
>-BOOL 
>+DWORD
> StartServiceCommand(int argc, LPCWSTR* argv) 
> {
>+  DWORD lastState = WaitForServiceStop(SVC_NAME, 5);
>+  if (lastState != SERVICE_STOPPED) {
>+    return 20000 + lastState;
>+  }
>+
>   // Get a handle to the SCM database.
>   SC_HANDLE serviceManager = OpenSCManager(NULL, NULL, 
>                                            SC_MANAGER_CONNECT | 
>                                            SC_MANAGER_ENUMERATE_SERVICE);
>   if (!serviceManager)  {
>-    return FALSE;
>+    return 17001;
>   }
> 
>   // Get a handle to the service.
>   SC_HANDLE service = OpenServiceW(serviceManager, 
>                                    SVC_NAME, 
>-                                   SERVICE_QUERY_STATUS | SERVICE_START);
>+                                   SERVICE_START);
>   if (!service) {
>     CloseServiceHandle(serviceManager);
>-    return FALSE;
>+    return 17002;
>   }
> 
>-  // Make sure the service is not stopped.
>-  SERVICE_STATUS_PROCESS ssp;
>-  DWORD bytesNeeded;
>-  if (!QueryServiceStatusEx(service, SC_STATUS_PROCESS_INFO, (LPBYTE)&ssp,
>-                            sizeof(SERVICE_STATUS_PROCESS), &bytesNeeded)) {
>-    CloseServiceHandle(service);
>-    CloseServiceHandle(serviceManager);
>-    return FALSE;
>+  // Wait at most 5 seconds trying to start the service in case of errors
>+  // like ERROR_SERVICE_DATABASE_LOCKED or ERROR_SERVICE_REQUEST_TIMEOUT.
>+  const DWORD maxWaitMS = 5000;
>+  DWORD currentWaitMS = 0;
>+  DWORD lastError = ERROR_SUCCESS;
>+  while (currentWaitMS < maxWaitMS) {
>+    BOOL result = StartServiceW(service, argc, argv);
>+    if (result) {
>+      lastError = ERROR_SUCCESS;
>+      break;
>+    } else {
>+      lastError = 18000 + GetLastError();
>+    }
>+    Sleep(100);
>+    currentWaitMS += 100;
>   }
>-
>-  // The service is already in use.
>-  if (ssp.dwCurrentState != SERVICE_STOPPED) {
>-    CloseServiceHandle(service);
>-    CloseServiceHandle(serviceManager);
>-    return FALSE;
>-  }
>-
>-  return StartServiceW(service, argc, argv);
>+  CloseServiceHandle(service);
>+  CloseServiceHandle(serviceManager);
>+  return lastError;
> }
> 
>@@ -429,57 +432,199 @@ WriteStatusFailure(LPCWSTR updateDirPath
>...
>-BOOL
>-WaitForServiceStop(LPCWSTR serviceName, DWORD maxWaitSeconds) 
>+DWORD WaitForServiceStop(LPCWSTR serviceName, DWORD maxWaitSeconds) 
nit: here and else where
DWORD
WaitForServiceStop

> {
>+  // 0x000000CF is defined above to be not set
>+  DWORD lastServiceState = 0x000000CF;
>+
>   // Get a handle to the SCM database.
>   SC_HANDLE serviceManager = OpenSCManager(NULL, NULL, 
>                                            SC_MANAGER_CONNECT | 
>                                            SC_MANAGER_ENUMERATE_SERVICE);
>   if (!serviceManager)  {
>-    return FALSE;
>+    DWORD lastError = GetLastError();
>+    switch(lastError) {
>+    case ERROR_ACCESS_DENIED:
>+      return 0x000000FD;
>+    case ERROR_DATABASE_DOES_NOT_EXIST:
>+      return 0x000000FE;
>+    default:
>+      return 0x000000FF;
>+    }
>   }
> 
>   // Get a handle to the service.
>   SC_HANDLE service = OpenServiceW(serviceManager, 
>                                    serviceName, 
>                                    SERVICE_QUERY_STATUS);
>   if (!service) {
>+    DWORD lastError = GetLastError();
>     CloseServiceHandle(serviceManager);
>-    return FALSE;
>+    switch(lastError) {
>+    case ERROR_ACCESS_DENIED:
>+      return 0x000000EB;
>+    case ERROR_INVALID_HANDLE:
>+      return 0x000000EC;
>+    case ERROR_INVALID_NAME:
>+      return 0x000000ED;
>+    case ERROR_SERVICE_DOES_NOT_EXIST:
>+      return 0x000000EE;
>+    default:
>+      return 0x000000EF;
>+    } 
>   }
> 
>-  BOOL gotStop = FALSE;
>   DWORD currentWaitMS = 0;
>+  SERVICE_STATUS_PROCESS ssp;
>+  ssp.dwCurrentState = lastServiceState;
>   while (currentWaitMS < maxWaitSeconds * 1000) {
>-    // Make sure the service is not stopped.
>-    SERVICE_STATUS_PROCESS ssp;
>     DWORD bytesNeeded;
>     if (!QueryServiceStatusEx(service, SC_STATUS_PROCESS_INFO, (LPBYTE)&ssp,
>                               sizeof(SERVICE_STATUS_PROCESS), &bytesNeeded)) {
>+      DWORD lastError = GetLastError();
>+      switch (lastError) {
>+      case ERROR_INVALID_HANDLE:
>+        ssp.dwCurrentState = 0x000000D9;
>+        break;
>+      case ERROR_ACCESS_DENIED:
>+        ssp.dwCurrentState = 0x000000DA;
>+        break;
>+      case ERROR_INSUFFICIENT_BUFFER:
>+        ssp.dwCurrentState = 0x000000DB;
>+        break;
>+      case ERROR_INVALID_PARAMETER:
>+        ssp.dwCurrentState = 0x000000DC;
>+        break;
>+      case ERROR_INVALID_LEVEL:
>+        ssp.dwCurrentState = 0x000000DD;
>+        break;
>+      case ERROR_SHUTDOWN_IN_PROGRESS:
>+        ssp.dwCurrentState = 0x000000DE;
>+        break;
>+      // These 3 errors can occur when the service is not yet stopped but
>+      // it is stopping.
>+      case ERROR_INVALID_SERVICE_CONTROL:
>+      case ERROR_SERVICE_CANNOT_ACCEPT_CTRL:
>+      case ERROR_SERVICE_NOT_ACTIVE:
>+        currentWaitMS += 50;
>+        Sleep(50);
>+        continue;
>+      default:
>+        ssp.dwCurrentState = 0x000000DF;
>+      }
>+
>+      // We couldn't query the status so just break out
>       break;
>     }
> 
>     // The service is already in use.
>     if (ssp.dwCurrentState == SERVICE_STOPPED) {
>-      gotStop = TRUE;
>       break;
>     }
>     currentWaitMS += 50;
>     Sleep(50);
>   }
> 
>+  lastServiceState = ssp.dwCurrentState;
>   CloseServiceHandle(service);
>   CloseServiceHandle(serviceManager);
>-  return gotStop;
>+  return lastServiceState;
> }
>+
>+
nit: extra blank line

>+/**
>+ * Determines if there is at least one process running for the specified
>+ * application. A match will be found across any session for any user.
>+ *
>+ * @param process The process to check for existance
>+ * @return ERROR_NOT_FOUND if the process was not found
>+ * @       ERROR_SUCCESS if the process was found and there were no errors
>+ * @       Other Win32 system error code for other errors
>+**/
>+DWORD
>+IsApplicationRunning(LPCWSTR filename)
How about IsProcessRunning?

>+/**
>+ * Waits for the specified applicaiton to exit.
>+ *
>+ * @param filename   The application to wait for.
>+ * @param maxSeconds The maximum amount of seconds to wait for all
>+ *                   instances of the application to exit.
>+ * @return  ERROR_SUCCESS if no instances of the application exist
>+ *          WAIT_TIMEOUT if the process is still running after maxSeconds.
>+ *          Any other Win32 system error code.
>+*/
>+DWORD
>+WaitForApplicationExit(LPCWSTR filename, DWORD maxSeconds) 
How about WaitForProcessExit?

nit: I see you removed some trailing spaces and added a new one here. Might as well search the files and clean them up.

>+{
>+  DWORD applicationRunningError = WAIT_TIMEOUT;
>+  for(DWORD i = 0; i < maxSeconds; i++) {
>+    DWORD applicationRunningError = IsApplicationRunning(filename);
>+    if (ERROR_NOT_FOUND == applicationRunningError) {
>+      return ERROR_SUCCESS;
>+    }
>+    Sleep(1000);
>+  }
>+
>+  if (ERROR_SUCCESS == applicationRunningError) {
>+    return WAIT_TIMEOUT;
>+  }
>+
>+  return applicationRunningError;
>+}
>diff --git a/toolkit/mozapps/update/test/TestAUSHelper.cpp b/toolkit/mozapps/update/test/TestAUSHelper.cpp
>--- a/toolkit/mozapps/update/test/TestAUSHelper.cpp
>+++ b/toolkit/mozapps/update/test/TestAUSHelper.cpp
>...
>-bool WaitForServiceStop(LPCWSTR serviceName, DWORD maxWaitSeconds) 
>+DWORD WaitForServiceStop(LPCWSTR serviceName, DWORD maxWaitSeconds) 
nit: please change the format so it is consistent
DWORD
WaitForServiceStop(LPCWSTR serviceName, DWORD maxWaitSeconds) 

>diff --git a/toolkit/mozapps/update/test/unit/head_update.js.in b/toolkit/mozapps/update/test/unit/head_update.js.in
>--- a/toolkit/mozapps/update/test/unit/head_update.js.in
>+++ b/toolkit/mozapps/update/test/unit/head_update.js.in
>@@ -522,16 +548,44 @@ function copyBinToApplyToDir(filename) {
>   let applyToUpdater = getApplyDirFile(null, true);
>   if (applyToUpdater.path != binDir.path) {
>     do_print("copying " + fileToCopy.path + " to: " + applyToUpdater.path);
>     fileToCopy.copyTo(applyToUpdater, filename);
>   }
> }
> 
> /**
>+ * Attempts to upgrade the maintenance service if permissions are allowed.
>+ * This is useful for XP where we have permission to upgrade in case an
>+ * older service installer exists.  Also if the user manually installed into
>+ * a unprivileged location.
This comment states, "This is useful for XP where we have permission to upgrade in case an older service installer exists." yet it has an early return for XP and below.

How are new versions of the service going to be tested for this case? We might just make the service directory writable on the build systems.

r=me with the above nits fixed. btw: excellent job on this patch!

Comment 30

[Brian R. Bondy [:bbondy]]

> This comment states, "This is useful for XP where we have permission to upgrade 
> in case an older service installer exists." yet it has an early return for XP 
> and below.

We have an early return for NOT XP or lower.  I'll change that to say isVistaOrHigher because it reads cleaner.

Old code there:

> var isXPOrLower = (parseFloat(version) < 6.0);
>   if (!isXPOrLower) {
>     return;
>   }


> How are new versions of the service going to be tested for this case? 
> We might just make the service directory writable on the build systems.

Before this change we just relied on the bootstrap upgrading, but that doesn't help with single test runs if you are using an old service.  Once this is all landed I'd like to propose a change as you mentioned for making it writable and we can get rid of the bootstrap test case all together.

Comment 31

[Brian R. Bondy [:bbondy]]

Attachment: Various fixes for intermittent failures. Patch v2.

Implemented review comments.

You can see just the differences from the last patch here:
https://hg.mozilla.org/projects/elm/rev/218f0eee530a

Comment 32

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/10894668e37f
https://hg.mozilla.org/mozilla-central/rev/e5481fffac9e
https://hg.mozilla.org/mozilla-central/rev/bda3dc1ca333
https://hg.mozilla.org/mozilla-central/rev/780d40dcf3c0
https://hg.mozilla.org/mozilla-central/rev/5c67bc43f94d
https://hg.mozilla.org/mozilla-central/rev/44ba65ab3eb5
https://hg.mozilla.org/mozilla-central/rev/a3578e0c0a51
https://hg.mozilla.org/mozilla-central/rev/a788eb31a0aa
https://hg.mozilla.org/mozilla-central/rev/9d47544d1aba
https://hg.mozilla.org/mozilla-central/rev/e805d90954b7

Comment 33

[[e-mail]]

Comment on attachment 585742 [details] [diff] [review]
Various fixes for intermittent failures. Patch v2.

>+  DWORD creationFlags = 0;
>+#ifdef DEBUG
>+  creationFlags |= CREATE_NEW_CONSOLE;
>+#endif
Why this change?

Comment 34

[Brian R. Bondy [:bbondy]]

I don't recall exactly but I suspected it was the cause of some failure throughout the tests.  It'll be reverted in the context of the bug khuey is posting regarding not using the same console.