Bug 1737117 - Pre: Add a reason to "should process updates at startup" logic. r=bytesized

I've elected to rename the function from `Should...` to
`ShouldNot...`, but not to rename the various test files.  The
functionality under test is both "should" and "should not", so I think
the churn of renaming is not justified.

This rearranges the deck chairs to accommodate testing the new
functionality in the next commit.

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

devtools/client/framework/browser-toolbox/test/browser_browser_toolbox_shouldprocessupdates.js

@@ -41,7 +41,7 @@ add_task(async function() {
   ok(result.exists, "MOZ_TEST_PROCESS_UPDATES exists in subprocess");
   is(
     result.get,
-    "!ShouldProcessUpdates()",
+    "ShouldNotProcessUpdates(): DevToolsLaunching",
     "MOZ_TEST_PROCESS_UPDATES is correct in subprocess"
   );
 

toolkit/components/backgroundtasks/tests/BackgroundTask_shouldprocessupdates.jsm

@@ -10,11 +10,17 @@ async function runBackgroundTask(commandLine) {
     Ci.nsIEnvironment
   );
 
-  let exitCode =
-    env.get("MOZ_TEST_PROCESS_UPDATES") == "!ShouldProcessUpdates()" ? 80 : 81;
+  const get = env.get("MOZ_TEST_PROCESS_UPDATES");
+  let exitCode = 81;
+  if (get == "ShouldNotProcessUpdates(): OtherInstanceRunning") {
+    exitCode = 80;
+  }
+  if (get == "ShouldNotProcessUpdates(): DevToolsLaunching") {
+    exitCode = 79;
+  }
   console.debug(`runBackgroundTask: shouldprocessupdates`, {
     exists: env.exists("MOZ_TEST_PROCESS_UPDATES"),
-    get: env.get("MOZ_TEST_PROCESS_UPDATES"),
+    get,
   });
   console.error(
     `runBackgroundTask: shouldprocessupdates exiting with exitCode ${exitCode}`

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

@@ -5,13 +5,13 @@
  * 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).
+  // The task returns 81 if !ShouldNotProcessUpdates(), <= 80 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,
+  // Since the behaviour under test (ShouldNotProcessUpdates()) 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"],
@@ -27,11 +27,10 @@ add_task(async function test_backgroundtask_shouldprocessupdates() {
   );
   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.
+  // The task returns 81 if !ShouldNotProcessUpdates(), <= 80 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("shouldprocessupdates");
   Assert.equal(81, exitCode);
 });

toolkit/xre/nsAppRunner.cpp

@@ -4162,7 +4162,23 @@ bool IsWaylandEnabled() {
 #endif
 
 #if defined(MOZ_UPDATER) && !defined(MOZ_WIDGET_ANDROID)
-bool ShouldProcessUpdates(nsXREDirProvider& aDirProvider) {
+enum struct ShouldNotProcessUpdatesReason {
+  DevToolsLaunching,
+  OtherInstanceRunning,
+};
+
+const char* ShouldNotProcessUpdatesReasonAsString(
+    ShouldNotProcessUpdatesReason aReason) {
+  switch (aReason) {
+    case ShouldNotProcessUpdatesReason::DevToolsLaunching:
+      return "DevToolsLaunching";
+    case ShouldNotProcessUpdatesReason::OtherInstanceRunning:
+      return "OtherInstanceRunning";
+  }
+}
+
+Maybe<ShouldNotProcessUpdatesReason> ShouldNotProcessUpdates(
+    nsXREDirProvider& aDirProvider) {
   // Do not process updates if we're launching devtools, as evidenced by
   // "--chrome ..." with the browser toolbox chrome document URL.
 
@@ -4177,8 +4193,8 @@ bool ShouldProcessUpdates(nsXREDirProvider& aDirProvider) {
   const char* chromeParam = nullptr;
   if (ARG_FOUND == CheckArg("chrome", &chromeParam, CheckArgFlag::None)) {
     if (!chromeParam || !strcmp(BROWSER_TOOLBOX_WINDOW_URL, chromeParam)) {
-      NS_WARNING("!ShouldProcessUpdates(): launching devtools");
-      return false;
+      NS_WARNING("ShouldNotProcessUpdates(): DevToolsLaunching");
+      return Some(ShouldNotProcessUpdatesReason::DevToolsLaunching);
     }
   }
 
@@ -4196,7 +4212,7 @@ bool ShouldProcessUpdates(nsXREDirProvider& aDirProvider) {
                                        getter_AddRefs(anAppFile));
     if (NS_FAILED(rv) || !anAppFile) {
       // Strange, but not a reason to skip processing updates.
-      return true;
+      return Nothing();
     }
 
     auto updateSyncManager = new nsUpdateSyncManager(anAppFile);
@@ -4204,13 +4220,13 @@ bool ShouldProcessUpdates(nsXREDirProvider& aDirProvider) {
     bool otherInstance = false;
     updateSyncManager->IsOtherInstanceRunning(&otherInstance);
     if (otherInstance) {
-      NS_WARNING("!ShouldProcessUpdates(): other instance is running");
-      return false;
+      NS_WARNING("ShouldNotProcessUpdates(): OtherInstanceRunning");
+      return Some(ShouldNotProcessUpdatesReason::OtherInstanceRunning);
     }
   }
 #  endif
 
-  return true;
+  return Nothing();
 }
 #endif
 
@@ -4574,7 +4590,9 @@ int XREMain::XRE_mainStartup(bool* aExitFlag) {
 #endif
 
 #if defined(MOZ_UPDATER) && !defined(MOZ_WIDGET_ANDROID)
-  if (ShouldProcessUpdates(mDirProvider)) {
+  Maybe<ShouldNotProcessUpdatesReason> shouldNotProcessUpdatesReason =
+      ShouldNotProcessUpdates(mDirProvider);
+  if (shouldNotProcessUpdatesReason.isNothing()) {
     // Check for and process any available updates
     nsCOMPtr<nsIFile> updRoot;
     bool persistent;
@@ -4632,7 +4650,12 @@ int XREMain::XRE_mainStartup(bool* aExitFlag) {
       // Support for testing *not* processing an update.  The launched process
       // can witness this environment variable and conclude that its runtime
       // environment resulted in not processing updates.
-      SaveToEnv("MOZ_TEST_PROCESS_UPDATES=!ShouldProcessUpdates()");
+
+      SaveToEnv(nsPrintfCString(
+                    "MOZ_TEST_PROCESS_UPDATES=ShouldNotProcessUpdates(): %s",
+                    ShouldNotProcessUpdatesReasonAsString(
+                        shouldNotProcessUpdatesReason.value()))
+                    .get());
     }
   }
 #endif