Bug 1696772 - Don't use FILE_FLAG_DELETE_ON_CLOSE for multi-instance locks. r=nalexander,application-update-reviewers.

FILE_FLAG_DELETE_ON_CLOSE had the wrong semantics, rendering the lock
file unusable after it had been closed once.

Delete the lock file in the uninstaller as a simple alternative (given that
the lock file is not in a temporary location on Windows).

For a test I returned to the older form of
test_backgroundtask_update_sync_manager which initially exposed the issue:
It expects the background task to be able to detect the xpcshell instance
after running resetLock, which failed before this fix.
I also extended the original updateSyncManager test to run the second
copy twice, which also catches the issue.

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

Author: agashlin

browser/installer/windows/nsis/uninstaller.nsi

@@ -327,6 +327,20 @@ Function un.OpenRefreshHelpURL
   ExecShell "open" "${URLProfileRefreshHelp}"
 FunctionEnd
 
+; Returns the common directory (typically "C:\ProgramData\Mozilla") on the stack.
+Function un.GetCommonDirectory
+  Push $0   ; Save $0
+
+  ; This gets C:\ProgramData or the equivalent.
+  ; 0x23 is CSIDL_COMMON_APPDATA, see CreateUpdateDir in common.nsh.
+  System::Call "Shell32::SHGetSpecialFolderPathW(p 0, t.r0, i 0x23, i 0)"
+  ; Add our subdirectory, this is hardcoded as grandparent of the update directory in
+  ; several other places.
+  StrCpy $0 "$0\Mozilla"
+
+  Exch $0   ; Restore original $0 and put our $0 on the stack.
+FunctionEnd
+
 Function un.SendUninstallPing
   ${If} $AppUserModelID == ""
     Return
@@ -340,12 +354,8 @@ Function un.SendUninstallPing
   Push $5   ; $5 = URL, POST result
   Push $6   ; $6 = Full path to the ping file
 
-  ; This gets C:\ProgramData or the equivalent.
-  ; 0x23 is CSIDL_COMMON_APPDATA, see CreateUpdateDir in common.nsh.
-  System::Call "Shell32::SHGetSpecialFolderPathW(p 0, t.r2, i 0x23, i 0)"
-  ; Add our subdirectory, this is hardcoded as grandparent of the update directory in
-  ; several other places.
-  StrCpy $2 "$2\Mozilla"
+  Call un.GetCommonDirectory
+  Pop $2
 
   ; The ping ID is in the file name, so that we can get it for the submission URL
   ; without having to parse the ping. Since we don't know the exact name, use FindFirst
@@ -441,6 +451,13 @@ Section "Uninstall"
     ApplicationID::UninstallJumpLists "$AppUserModelID"
   ${EndIf}
 
+  ; Remove the update sync manager's multi-instance lock file
+  ${If} "$AppUserModelID" != ""
+    Call un.GetCommonDirectory
+    Pop $0
+    Delete /REBOOTOK "$0\UpdateLock-$AppUserModelID"
+  ${EndIf}
+
   ; Remove the updates directory
   ${un.CleanUpdateDirectories} "Mozilla\Firefox" "Mozilla\updates"
 

toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_update_sync_manager.js

@@ -14,26 +14,15 @@ add_task(async function test_backgroundtask_update_sync_manager() {
   // This can also be achieved by overriding directory providers, but
   // that's not particularly robust in the face of parallel testing.
   // Doing it this way exercises `resetLock` with a path.
-  let syncManager = Cc["@mozilla.org/updates/update-sync-manager;1"].getService(
-    Ci.nsIUpdateSyncManager
-  );
-
-  let file = do_get_profile();
-  file.append("customExePath1");
-  file.append("customExe");
-
-  // The background task will see our process.
-  syncManager.resetLock(file);
-
   let exitCode = await do_backgroundtask("update_sync_manager", {
-    extraArgs: [file.path],
+    extraArgs: [Services.dirsvc.get("XREExeF", Ci.nsIFile).path],
   });
   Assert.equal(80, exitCode, "Another instance is running");
 
   // If we tell the backgroundtask to use a unique appPath, the
   // background task won't see any other instances running.
-  file = do_get_profile();
-  file.append("customExePath2");
+  let file = do_get_profile();
+  file.append("customExePath");
   file.append("customExe");
 
   exitCode = await do_backgroundtask("update_sync_manager", {

toolkit/mozapps/update/tests/unit_aus_update/updateSyncManager.js

@@ -61,39 +61,45 @@ add_task(async function() {
     getTestDirFile("syncManagerTestChild.js").path,
   ];
 
-  // Now we can actually invoke the process.
-  debugDump(`launching child process at ${thisBinary.path} with args ${args}`);
-  Subprocess.call({
-    command: thisBinary.path,
-    arguments: args,
-    stderr: "stdout",
-  });
-
-  // It will take the new xpcshell a little time to start up, but we should see
-  // the effect on the lock within at most a few seconds.
-  await TestUtils.waitForCondition(
-    () => syncManager.isOtherInstanceRunning(),
-    "waiting for child process to take the lock"
-  ).catch(e => {
-    // Rather than throwing out of waitForCondition(), catch and log the failure
-    // manually so that we get output that's a bit more readable.
-    Assert.ok(
-      syncManager.isOtherInstanceRunning(),
-      "child process has the lock"
+  // Run the second copy two times, to show the lock is usable after having
+  // been closed.
+  for (let runs = 0; runs < 2; runs++) {
+    // Now we can actually invoke the process.
+    debugDump(
+      `launching child process at ${thisBinary.path} with args ${args}`
     );
-  });
+    Subprocess.call({
+      command: thisBinary.path,
+      arguments: args,
+      stderr: "stdout",
+    });
 
-  // The lock should have been closed when the process exited, but we'll allow
-  // a little time for the OS to clean up the handle.
-  await TestUtils.waitForCondition(
-    () => !syncManager.isOtherInstanceRunning(),
-    "waiting for child process to release the lock"
-  ).catch(e => {
-    Assert.ok(
-      !syncManager.isOtherInstanceRunning(),
-      "child process has released the lock"
-    );
-  });
+    // It will take the new xpcshell a little time to start up, but we should see
+    // the effect on the lock within at most a few seconds.
+    await TestUtils.waitForCondition(
+      () => syncManager.isOtherInstanceRunning(),
+      "waiting for child process to take the lock"
+    ).catch(e => {
+      // Rather than throwing out of waitForCondition(), catch and log the failure
+      // manually so that we get output that's a bit more readable.
+      Assert.ok(
+        syncManager.isOtherInstanceRunning(),
+        "child process has the lock"
+      );
+    });
+
+    // The lock should have been closed when the process exited, but we'll allow
+    // a little time for the OS to clean up the handle.
+    await TestUtils.waitForCondition(
+      () => !syncManager.isOtherInstanceRunning(),
+      "waiting for child process to release the lock"
+    ).catch(e => {
+      Assert.ok(
+        !syncManager.isOtherInstanceRunning(),
+        "child process has released the lock"
+      );
+    });
+  }
 
   doTestFinish();
 });

toolkit/xre/MultiInstanceLock.cpp

@@ -65,7 +65,7 @@ MultiInstLockHandle OpenMultiInstanceLock(const char* nameToken,
       ::CreateFileW(PromiseFlatString(NS_ConvertUTF8toUTF16(filePath)).get(),
                     GENERIC_READ | GENERIC_WRITE,
                     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-                    nullptr, OPEN_ALWAYS, FILE_FLAG_DELETE_ON_CLOSE, nullptr);
+                    nullptr, OPEN_ALWAYS, 0, nullptr);
   if (h != INVALID_HANDLE_VALUE) {
     // The LockFileEx functions always require an OVERLAPPED structure even
     // though we did not open the lock file for overlapped I/O.
@@ -108,8 +108,6 @@ void ReleaseMultiInstanceLock(MultiInstLockHandle lock) {
 #ifdef XP_WIN
     OVERLAPPED o = {0};
     ::UnlockFileEx(lock, 0, 1, 0, &o);
-    // We've used FILE_FLAG_DELETE_ON_CLOSE, so if we are the last instance
-    // with a handle on the lock file, closing it here will delete it.
     ::CloseHandle(lock);
 
 #else