Bug 1804757 - Avoid shutdown crashes writing `targeting.snapshot.json`. r=barret

This commit trades away consistency in order to reduce code execution.
Hopefully this avoids executing code that is likely not "shutdown
safe" and is inadvertently restarting services or otherwise hanging
when run during shutdown.

The trade off is that the written targeting snapshot may now be
incomplete (which was always the case in the face of runtime errors,
e.g., corrupt a database) or internally inconsistent (which is
probably new).  For example, weekly usage could be populated but most
frecent sites could be empty, contradicting the usage.  This trade-off
is accepted: occassionally targeting the user "in the
past" (generally, with data captured 30 minutes ago) is preferable to
relatively frequent shutdown crashes.

I would have liked to have checked for `shuttingDown` in
[CachedTargetingGetter](https://searchfox.org/mozilla-central/rev/abf6758ed833c203f84703aa2e3e3d317571b1e9/browser/components/newtab/lib/ASRouterTargeting.jsm#152),
but doing so might throw exceptions (or inject `null` values) where
they were not anticipated, which carries risks that I couldn't easily
mitigate.

If the measure implemented here is ineffective, we'll likely need to
restrict the set of targeting data collected, but I hope to avoid
that, since it reduces the flexibility of the background messaging
targeting significantly, and we may want the agility provided by rich
targeting at an unknown time in the future.

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

browser/components/newtab/lib/ASRouterTargeting.jsm

@@ -840,10 +840,13 @@ const ASRouterTargeting = {
    */
   async getEnvironmentSnapshot(target = ASRouterTargeting.Environment) {
     // One promise for each named property.  Label promises with property name.
-    let promises = Object.keys(target).map(async name => [
-      name,
-      await target[name],
-    ]);
+    let promises = Object.keys(target).map(async name => {
+      // Each promise needs to check if we're shutting down when it is evaluated.
+      if (Services.startup.shuttingDown) {
+        throw new Error("shutting down, so not querying targeting environment");
+      }
+      return [name, await target[name]];
+    });
 
     // Ignore properties that are rejected.
     let results = await Promise.allSettled(promises);

browser/components/newtab/test/xpcshell/test_ASRouterTargeting_snapshot.js

@@ -22,3 +22,33 @@ add_task(async function should_ignore_rejections() {
   let snapshot = await ASRouterTargeting.getEnvironmentSnapshot(target);
   deepEqual(snapshot, { environment: { foo: 1 }, version: 1 });
 });
+
+add_task(async function should_ignore_rejections() {
+  // The order that `ASRouterTargeting.getEnvironmentSnapshot`
+  // enumerates the target object matters here, but it's guaranteed to
+  // be consistent by the `for ... in` ordering: see
+  // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in#description.
+  let target = {
+    get foo() {
+      return new Promise(resolve => resolve(1));
+    },
+
+    get bar() {
+      return new Promise(resolve => {
+        // Pretend that we're about to shut down.
+        Services.startup.advanceShutdownPhase(
+          Services.startup.SHUTDOWN_PHASE_APPSHUTDOWN
+        );
+        resolve(2);
+      });
+    },
+
+    get baz() {
+      return new Promise(resolve => resolve(3));
+    },
+  };
+
+  let snapshot = await ASRouterTargeting.getEnvironmentSnapshot(target);
+  // `baz` is dropped since we're shutting down by the time it's processed.
+  deepEqual(snapshot, { environment: { foo: 1, bar: 2 }, version: 1 });
+});

toolkit/mozapps/update/BackgroundUpdate.jsm

@@ -649,7 +649,23 @@ var BackgroundUpdate = {
         lazy.log.debug(
           `${SLUG}: preparing to write Firefox Messaging System targeting information to ${path}`
         );
-        snapshot.data = await lazy.ASRouterTargeting.getEnvironmentSnapshot();
+
+        // Merge latest data into existing data.  This data may be partial, due
+        // to runtime errors and abbreviated collections, especially when
+        // shutting down.  We accept the risk of incomplete or even internally
+        // inconsistent data: it's generally better to have stale data (and
+        // potentially target a user as they appeared in the past) than to block
+        // shutdown for more accurate results.  An alternate approach would be
+        // to restrict the targeting data collected, but it's hard to
+        // distinguish expensive collection operations and the system loses
+        // flexibility when restrictions of this type are added.
+        let latestData = await lazy.ASRouterTargeting.getEnvironmentSnapshot();
+        // We expect to always have data, but: belt-and-braces.
+        if (snapshot?.data?.environment) {
+          Object.assign(snapshot.data.environment, latestData.environment);
+        } else {
+          snapshot.data = latestData;
+        }
       },
       path,
     });