Bug 1695797 - In background task mode, only process updates if we're the sole instance running. r=mhowell,application-update-reviewers,dthayer,bytesized

This commit does three main things:

1) It allows to configure the global singleton `nsUpdateSyncManager`
    with an `nsIFile` rather than having it use the ambient XPCOM
    directory service.  This allows to initialize the
    `nsUpdateSyncManager` very early: before processing updates and long
    before XPCOM is initialized.  This in turn allows us to determine if
    other instances early enough to skip processing updates when
    appropriate.

    When this initialization path is followed, i.e., in Firefox but not
    `xpcshell`, the `xpcom-startup` notification will be received but no
    action taken, since the singleton will already exist.

    There is a classic time-of-check, time-of-use race window in this
    implementation: an instance may be launched immediately after we check
    for other instances.  In practice this will result in behaviour that
    is alreay possible: two independent instances both processing updates.
    It is expected that the updater itself will exclude one of the
    instances using its existing mutex.

    To avoid the race, we could take the exclusive multi-instance lock
    ourselves, but that is strictly more complicated.

2) It updates an existing background task test to use an explicit
    `nsIFile` rather than the existing directory service method.  This
    exercises the newer API.  There are other tests that might benefit,
    but there's no harm in remaining with the previous approach, since
    both are required.

3) It adds a new background task test to verify that update processing
    is skipped if we're not the sole instance running.

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

toolkit/components/backgroundtasks/moz.build

@@ -49,6 +49,7 @@ TESTING_JS_MODULES.backgroundtasks += [
     "tests/BackgroundTask_backgroundtask_specific_pref.jsm",
     "tests/BackgroundTask_crash.jsm",
     "tests/BackgroundTask_policies.jsm",
+    "tests/BackgroundTask_shouldprocessupdates.jsm",
     "tests/BackgroundTask_update_sync_manager.jsm",
     "tests/BackgroundTask_wait.jsm",
 ]

toolkit/components/backgroundtasks/tests/BackgroundTask_shouldprocessupdates.jsm

@@ -0,0 +1,24 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+var EXPORTED_SYMBOLS = ["runBackgroundTask"];
+
+async function runBackgroundTask(commandLine) {
+  var env = Cc["@mozilla.org/process/environment;1"].getService(
+    Ci.nsIEnvironment
+  );
+
+  let exitCode =
+    env.get("MOZ_TEST_PROCESS_UPDATES") == "!ShouldProcessUpdates()" ? 80 : 81;
+  console.debug(`runBackgroundTask: shouldprocessupdates`, {
+    exists: env.exists("MOZ_TEST_PROCESS_UPDATES"),
+    get: env.get("MOZ_TEST_PROCESS_UPDATES"),
+  });
+  console.error(
+    `runBackgroundTask: shouldprocessupdates exiting with exitCode ${exitCode}`
+  );
+
+  return exitCode;
+}

toolkit/components/backgroundtasks/tests/BackgroundTask_update_sync_manager.jsm

@@ -10,6 +10,11 @@ async function runBackgroundTask(commandLine) {
     Ci.nsIUpdateSyncManager
   );
 
+  let appPath = commandLine.getArgument(0);
+  let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
+  file.initWithPath(appPath);
+  syncManager.resetLock(file);
+
   let exitCode = syncManager.isOtherInstanceRunning() ? 80 : 81;
   console.error(
     `runBackgroundTask: update_sync_manager exiting with exitCode ${exitCode}`

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

@@ -0,0 +1,37 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
+ * vim: sw=4 ts=4 sts=4 et
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+add_task(async function test_backgroundtask_shouldprocessupdates() {
+  // The task returns 80 if !ShouldProcessUpdates(), 81 otherwise.  xpcshell
+  // itself counts as an instance, so the background task will see it and think
+  // another instance is running.  N.b.: this isn't as robust as it could be:
+  // running Firefox instances and parallel tests interact here (mostly
+  // harmlessly).
+  //
+  // Since the behaviour under test (ShouldProcessUpdates()) happens at startup,
+  // we can't easily change the lock location of the background task.
+  let exitCode = await do_backgroundtask("shouldprocessupdates", {
+    extraArgs: ["--test-process-updates"],
+  });
+  Assert.equal(80, exitCode);
+
+  // If we change our lock location, the background task won't see our instance
+  // running.
+  let file = do_get_profile();
+  file.append("customExePath");
+  let syncManager = Cc["@mozilla.org/updates/update-sync-manager;1"].getService(
+    Ci.nsIUpdateSyncManager
+  );
+  syncManager.resetLock(file);
+
+  // The task returns 80 if !ShouldProcessUpdates(), 81 if
+  // ShouldProcessUpdates(). Since we've changed the xpcshell executable name,
+  // the background task won't see us and think another instance is running.
+  // N.b.: this generally races with other parallel tests and in the future it
+  // could be made more robust.
+  exitCode = await do_backgroundtask("shouldprocessupdates");
+  Assert.equal(81, exitCode);
+});

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

@@ -7,47 +7,22 @@
 add_task(async function test_backgroundtask_update_sync_manager() {
   // The task returns 80 if another instance is running, 81 otherwise.  xpcshell
   // itself counts as an instance, so the background task will see it and think
-  // another instance is running.  N.b.: this isn't as robust as it could be:
-  // running Firefox instances and parallel tests interact here (mostly
-  // harmlessly).
-  let exitCode = await do_backgroundtask("update_sync_manager");
+  // another instance is running.
+  //
+  // 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 exitCode = await do_backgroundtask("update_sync_manager", {
+    extraArgs: [Services.dirsvc.get("XREExeF", Ci.nsIFile).path],
+  });
   Assert.equal(80, exitCode);
 
-  // This functionality is copied from tests in `toolkit/mozapps/update/tests`.
-  let dirProvider = {
-    getFile: function AGP_DP_getFile(aProp, aPersistent) {
-      // Set the value of persistent to false so when this directory provider is
-      // unregistered it will revert back to the original provider.
-      aPersistent.value = false;
-      // The sync manager only needs XREExeF, so that's all we provide.
-      if (aProp == "XREExeF") {
-        let file = do_get_profile();
-        file.append("customExePath");
-        return file;
-      }
-      return null;
-    },
-    QueryInterface: ChromeUtils.generateQI(["nsIDirectoryServiceProvider"]),
-  };
-  let ds = Services.dirsvc.QueryInterface(Ci.nsIDirectoryService);
-  ds.QueryInterface(Ci.nsIProperties).undefine("XREExeF");
-  ds.registerProvider(dirProvider);
-
-  try {
-    // Now that we've overridden the directory provider, the name of the update
-    // lock needs to be changed to match the overridden path.
-    let syncManager = Cc[
-      "@mozilla.org/updates/update-sync-manager;1"
-    ].getService(Ci.nsIUpdateSyncManager);
-    syncManager.resetLock();
-
-    // The task returns 80 if another instance is running, 81 otherwise.  Since
-    // we've changed the xpcshell executable name, the background task won't see
-    // us and think another instance is running.  N.b.: this generally races
-    // with other parallel tests and in the future it could be made more robust.
-    exitCode = await do_backgroundtask("update_sync_manager");
-    Assert.equal(81, exitCode);
-  } finally {
-    ds.unregisterProvider(dirProvider);
-  }
+  // If we tell the backgroundtask to use a unique appPath, the
+  // background task won't see any other instances running.
+  let file = do_get_profile();
+  file.append("customExePath");
+  exitCode = await do_backgroundtask("update_sync_manager", {
+    extraArgs: [file.path],
+  });
+  Assert.equal(81, exitCode);
 });

toolkit/components/backgroundtasks/tests/xpcshell/xpcshell.ini

@@ -11,6 +11,7 @@ support-files =
 
 [test_backgroundtask_exitcodes.js]
 [test_backgroundtask_policies.js]
+[test_backgroundtask_shouldprocessupdates.js]
 [test_backgroundtask_specific_pref.js]
 [test_backgroundtask_update_sync_manager.js]
 [test_manifest_with_backgroundtask.js]

toolkit/mozapps/update/nsIUpdateService.idl

@@ -461,10 +461,12 @@ interface nsIUpdateSyncManager : nsISupports
 
   /**
    * Should only be used for testing.
-   * Closes and reopens the lock file, possibly under a different name if the
-   * path hash has changed (which should only happen if a test is forcing it).
+   *
+   * Closes and reopens the lock file, possibly under a different name if a
+   * parameter is given (or the path hash has changed, which should only happen
+   * if a test is forcing it).
    */
-  void resetLock();
+  void resetLock([optional] in nsIFile anAppFile);
 };
 
 /**

toolkit/xre/nsAppRunner.cpp

@@ -35,6 +35,7 @@
 #include "mozilla/Bootstrap.h"
 #if defined(MOZ_UPDATER) && !defined(MOZ_WIDGET_ANDROID)
 #  include "nsUpdateDriver.h"
+#  include "nsUpdateSyncManager.h"
 #endif
 #include "ProfileReset.h"
 
@@ -4183,7 +4184,7 @@ bool IsWaylandEnabled() {
 #endif
 
 #if defined(MOZ_UPDATER) && !defined(MOZ_WIDGET_ANDROID)
-bool ShouldProcessUpdates() {
+bool ShouldProcessUpdates(nsXREDirProvider& aDirProvider) {
   // Do not process updates if we're launching devtools, as evidenced by
   // "--chrome ..." with the browser toolbox chrome document URL.
 
@@ -4203,6 +4204,34 @@ bool ShouldProcessUpdates() {
     }
   }
 
+#  ifdef MOZ_BACKGROUNDTASKS
+  // Do not process updates if we're running a background task mode and another
+  // instance is already running.  This avoids periodic maintenance updating
+  // underneath a browsing session.
+  if (BackgroundTasks::IsBackgroundTaskMode()) {
+    // At this point we have a dir provider but no XPCOM directory service.  We
+    // launch the update sync manager using that information so that it doesn't
+    // need to ask for (and fail to find) the directory service.
+    nsCOMPtr<nsIFile> anAppFile;
+    bool persistent;
+    nsresult rv = aDirProvider.GetFile(XRE_EXECUTABLE_FILE, &persistent,
+                                       getter_AddRefs(anAppFile));
+    if (NS_FAILED(rv) || !anAppFile) {
+      // Strange, but not a reason to skip processing updates.
+      return true;
+    }
+
+    auto updateSyncManager = new nsUpdateSyncManager(anAppFile);
+
+    bool otherInstance = false;
+    updateSyncManager->IsOtherInstanceRunning(&otherInstance);
+    if (otherInstance) {
+      NS_WARNING("!ShouldProcessUpdates(): other instance is running");
+      return false;
+    }
+  }
+#  endif
+
   return true;
 }
 #endif
@@ -4562,7 +4591,7 @@ int XREMain::XRE_mainStartup(bool* aExitFlag) {
 #endif
 
 #if defined(MOZ_UPDATER) && !defined(MOZ_WIDGET_ANDROID)
-  if (ShouldProcessUpdates()) {
+  if (ShouldProcessUpdates(mDirProvider)) {
     // Check for and process any available updates
     nsCOMPtr<nsIFile> updRoot;
     bool persistent;

toolkit/xre/nsUpdateSyncManager.cpp

@@ -24,7 +24,10 @@ nsUpdateSyncManager* gUpdateSyncManager = nullptr;
 
 NS_IMPL_ISUPPORTS(nsUpdateSyncManager, nsIUpdateSyncManager, nsIObserver)
 
-nsUpdateSyncManager::nsUpdateSyncManager() { OpenLock(); }
+nsUpdateSyncManager::nsUpdateSyncManager(nsIFile* anAppFile /* = nullptr */) {
+  gUpdateSyncManager = this;
+  OpenLock(anAppFile);
+}
 
 nsUpdateSyncManager::~nsUpdateSyncManager() {
   ReleaseLock();
@@ -33,7 +36,7 @@ nsUpdateSyncManager::~nsUpdateSyncManager() {
 
 already_AddRefed<nsUpdateSyncManager> nsUpdateSyncManager::GetSingleton() {
   if (!gUpdateSyncManager) {
-    gUpdateSyncManager = new nsUpdateSyncManager();
+    new nsUpdateSyncManager();  // This sets gUpdateSyncManager.
   }
   return do_AddRef(gUpdateSyncManager);
 }
@@ -63,7 +66,7 @@ NS_IMETHODIMP nsUpdateSyncManager::Observe(nsISupports* aSubject,
   return NS_OK;
 }
 
-nsresult nsUpdateSyncManager::OpenLock() {
+nsresult nsUpdateSyncManager::OpenLock(nsIFile* anAppFile) {
   if (mLock != MULTI_INSTANCE_LOCK_HANDLE_ERROR) {
     // Lock is already open.
     return NS_OK;
@@ -75,14 +78,22 @@ nsresult nsUpdateSyncManager::OpenLock() {
     return NS_OK;
   }
 
-  nsCOMPtr<nsIProperties> dirSvc =
-      do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID);
-  NS_ENSURE_TRUE(dirSvc, NS_ERROR_SERVICE_NOT_AVAILABLE);
-
+  // If we're given an app file, use it; otherwise, get it from the ambient
+  // directory service.
+  nsresult rv;
   nsCOMPtr<nsIFile> appFile;
-  nsresult rv = dirSvc->Get(XRE_EXECUTABLE_FILE, NS_GET_IID(nsIFile),
-                            getter_AddRefs(appFile));
-  NS_ENSURE_SUCCESS(rv, rv);
+  if (anAppFile) {
+    rv = anAppFile->Clone(getter_AddRefs(appFile));
+    NS_ENSURE_SUCCESS(rv, rv);
+  } else {
+    nsCOMPtr<nsIProperties> dirSvc =
+        do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID);
+    NS_ENSURE_TRUE(dirSvc, NS_ERROR_SERVICE_NOT_AVAILABLE);
+
+    rv = dirSvc->Get(XRE_EXECUTABLE_FILE, NS_GET_IID(nsIFile),
+                     getter_AddRefs(appFile));
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
 
   nsCOMPtr<nsIFile> appDirFile;
   rv = appFile->GetParent(getter_AddRefs(appDirFile));
@@ -124,7 +135,7 @@ NS_IMETHODIMP nsUpdateSyncManager::IsOtherInstanceRunning(bool* aResult) {
   return NS_OK;
 }
 
-NS_IMETHODIMP nsUpdateSyncManager::ResetLock() {
+NS_IMETHODIMP nsUpdateSyncManager::ResetLock(nsIFile* anAppFile = nullptr) {
   ReleaseLock();
-  return OpenLock();
+  return OpenLock(anAppFile);
 }

toolkit/xre/nsUpdateSyncManager.h

@@ -24,7 +24,7 @@
 class nsUpdateSyncManager final : public nsIUpdateSyncManager,
                                   public nsIObserver {
  public:
-  nsUpdateSyncManager();
+  explicit nsUpdateSyncManager(nsIFile* anAppFile = nullptr);
 
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIUPDATESYNCMANAGER
@@ -40,7 +40,7 @@ class nsUpdateSyncManager final : public nsIUpdateSyncManager,
   nsUpdateSyncManager& operator=(nsUpdateSyncManager&) = delete;
   nsUpdateSyncManager& operator=(nsUpdateSyncManager&&) = delete;
 
-  nsresult OpenLock();
+  nsresult OpenLock(nsIFile* anAppFile = nullptr);
   void ReleaseLock();
 
   mozilla::MultiInstLockHandle mLock = MULTI_INSTANCE_LOCK_HANDLE_ERROR;