Bug 1413673 - Sync shield recipe-client v78 from Github (commit 1553a2e) r=rhelmer draft
authorMike Cooper <mcooper@mozilla.com>
Wed, 01 Nov 2017 13:29:03 -0700
changeset 703960 702f060b07ac926021f5480386a54c89c66bab71
parent 703959 d33ac0db7103cfd681798233a1521b18cb8e908f
child 741943 389e73924a2899e8555646c8e9cbe171b1b7e0a8
push id91011
push userbmo:mcooper@mozilla.com
push dateTue, 28 Nov 2017 00:17:14 +0000
reviewersrhelmer
bugs1413673
milestone59.0a1
Bug 1413673 - Sync shield recipe-client v78 from Github (commit 1553a2e) r=rhelmer MozReview-Commit-ID: G68WgaqHZSv
browser/extensions/shield-recipe-client/install.rdf.in
browser/extensions/shield-recipe-client/lib/RecipeRunner.jsm
browser/extensions/shield-recipe-client/test/browser/browser_RecipeRunner.js
browser/extensions/shield-recipe-client/test/unit/head_xpc.js
--- a/browser/extensions/shield-recipe-client/install.rdf.in
+++ b/browser/extensions/shield-recipe-client/install.rdf.in
@@ -3,17 +3,17 @@
 #filter substitution
 
 <RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:em="http://www.mozilla.org/2004/em-rdf#">
   <Description about="urn:mozilla:install-manifest">
     <em:id>shield-recipe-client@mozilla.org</em:id>
     <em:type>2</em:type>
     <em:bootstrap>true</em:bootstrap>
     <em:unpack>false</em:unpack>
-    <em:version>76.1</em:version>
+    <em:version>78</em:version>
     <em:name>Shield Recipe Client</em:name>
     <em:description>Client to download and run recipes for SHIELD, Heartbeat, etc.</em:description>
     <em:multiprocessCompatible>true</em:multiprocessCompatible>
 
     <em:targetApplication>
       <Description>
         <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
         <em:minVersion>@MOZ_APP_VERSION@</em:minVersion>
--- a/browser/extensions/shield-recipe-client/lib/RecipeRunner.jsm
+++ b/browser/extensions/shield-recipe-client/lib/RecipeRunner.jsm
@@ -34,112 +34,171 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "Uptake",
                                   "resource://shield-recipe-client/lib/Uptake.jsm");
 
 Cu.importGlobalProperties(["fetch"]);
 
 this.EXPORTED_SYMBOLS = ["RecipeRunner"];
 
 const log = LogManager.getLogger("recipe-runner");
-const prefs = Services.prefs.getBranch("extensions.shield-recipe-client.");
 const TIMER_NAME = "recipe-client-addon-run";
-const RUN_INTERVAL_PREF = "run_interval_seconds";
-const FIRST_RUN_PREF = "first_run";
 const PREF_CHANGED_TOPIC = "nsPref:changed";
 
+const TELEMETRY_ENABLED_PREF = "datareporting.healthreport.uploadEnabled";
+
+const SHIELD_PREF_PREFIX = "extensions.shield-recipe-client";
+const RUN_INTERVAL_PREF = `${SHIELD_PREF_PREFIX}.run_interval_seconds`;
+const FIRST_RUN_PREF = `${SHIELD_PREF_PREFIX}.first_run`;
+const SHIELD_ENABLED_PREF = `${SHIELD_PREF_PREFIX}.enabled`;
+const DEV_MODE_PREF = `${SHIELD_PREF_PREFIX}.dev_mode`;
+const API_URL_PREF = `${SHIELD_PREF_PREFIX}.api_url`;
+const LAZY_CLASSIFY_PREF = `${SHIELD_PREF_PREFIX}.experiments.lazy_classify`;
+
+const PREFS_TO_WATCH = [
+  RUN_INTERVAL_PREF,
+  TELEMETRY_ENABLED_PREF,
+  SHIELD_ENABLED_PREF,
+  API_URL_PREF,
+];
+
 this.RecipeRunner = {
   async init() {
-    if (!this.checkPrefs()) {
+    this.enabled = null;
+    this.checkPrefs();  // sets this.enabled
+    this.watchPrefs();
+
+    // Run if enabled immediately on first run, or if dev mode is enabled.
+    const firstRun = Services.prefs.getBoolPref(FIRST_RUN_PREF);
+    const devMode = Services.prefs.getBoolPref(DEV_MODE_PREF);
+
+    if (this.enabled && (devMode || firstRun)) {
+      await this.run();
+    }
+    if (firstRun) {
+      Services.prefs.setBoolPref(FIRST_RUN_PREF, false);
+    }
+  },
+
+  enable() {
+    if (this.enabled) {
+      return;
+    }
+    this.registerTimer();
+    this.enabled = true;
+  },
+
+  disable() {
+    if (!this.enabled) {
+      return;
+    }
+    this.unregisterTimer();
+    this.enabled = false;
+  },
+
+  /** Watch for prefs to change, and call this.observer when they do */
+  watchPrefs() {
+    for (const pref of PREFS_TO_WATCH) {
+      Services.prefs.addObserver(pref, this);
+    }
+
+    CleanupManager.addCleanupHandler(this.unwatchPrefs.bind(this));
+  },
+
+  unwatchPrefs() {
+    for (const pref of PREFS_TO_WATCH) {
+      Services.prefs.removeObserver(pref, this);
+    }
+  },
+
+  /** When prefs change, this is fired */
+  observe(subject, topic, data) {
+    switch (topic) {
+      case PREF_CHANGED_TOPIC: {
+        const prefName = data;
+
+        switch (prefName) {
+          case RUN_INTERVAL_PREF:
+            this.updateRunInterval();
+            break;
+
+          // explicit fall-through
+          case TELEMETRY_ENABLED_PREF:
+          case SHIELD_ENABLED_PREF:
+          case API_URL_PREF:
+            this.checkPrefs();
+            break;
+
+          default:
+            log.debug(`Observer fired with unexpected pref change: ${prefName}`);
+        }
+
+        break;
+      }
+    }
+  },
+
+  checkPrefs() {
+    // Only run if Unified Telemetry is enabled.
+    if (!Services.prefs.getBoolPref(TELEMETRY_ENABLED_PREF)) {
+      log.debug("Disabling RecipeRunner because Unified Telemetry is disabled.");
+      this.disable();
       return;
     }
 
-    // Run immediately on first run, or if dev mode is enabled.
-    if (prefs.getBoolPref(FIRST_RUN_PREF) || prefs.getBoolPref("dev_mode")) {
-      await this.run();
-      prefs.setBoolPref(FIRST_RUN_PREF, false);
+    if (!Services.prefs.getBoolPref(SHIELD_ENABLED_PREF)) {
+      log.debug(`Disabling Shield because ${SHIELD_ENABLED_PREF} is set to false`);
+      this.disable();
+      return;
     }
 
-    this.registerTimer();
+    const apiUrl = Services.prefs.getCharPref(API_URL_PREF);
+    if (!apiUrl || !apiUrl.startsWith("https://")) {
+      log.warn(`Disabling shield because ${API_URL_PREF} is not an HTTPS url: ${apiUrl}.`);
+      this.disable();
+      return;
+    }
+
+    this.enable();
   },
 
   registerTimer() {
     this.updateRunInterval();
     CleanupManager.addCleanupHandler(() => timerManager.unregisterTimer(TIMER_NAME));
-
-    // Watch for the run interval to change, and re-register the timer with the new value
-    prefs.addObserver(RUN_INTERVAL_PREF, this);
-    CleanupManager.addCleanupHandler(() => prefs.removeObserver(RUN_INTERVAL_PREF, this));
   },
 
-  checkPrefs() {
-    // Only run if Unified Telemetry is enabled.
-    if (!Services.prefs.getBoolPref("toolkit.telemetry.unified")) {
-      log.info("Disabling RecipeRunner because Unified Telemetry is disabled.");
-      return false;
-    }
-
-    if (!prefs.getBoolPref("enabled")) {
-      log.info("Recipe Client is disabled.");
-      return false;
-    }
-
-    const apiUrl = prefs.getCharPref("api_url");
-    if (!apiUrl || !apiUrl.startsWith("https://")) {
-      log.error(`Non HTTPS URL provided for extensions.shield-recipe-client.api_url: ${apiUrl}`);
-      return false;
-    }
-
-    return true;
-  },
-
-  observe(subject, topic, data) {
-    switch (topic) {
-      case PREF_CHANGED_TOPIC:
-        this.observePrefChange(data);
-        break;
-    }
-  },
-
-  /**
-   * Watch for preference changes from Services.pref.addObserver.
-   */
-  observePrefChange(prefName) {
-    if (prefName === RUN_INTERVAL_PREF) {
-      this.updateRunInterval();
-    } else {
-      log.debug(`Observer fired with unexpected pref change: ${prefName}`);
-    }
+  unregisterTimer() {
+    timerManager.unregisterTimer(TIMER_NAME);
   },
 
   updateRunInterval() {
     // Run once every `runInterval` wall-clock seconds. This is managed by setting a "last ran"
     // timestamp, and running if it is more than `runInterval` seconds ago. Even with very short
     // intervals, the timer will only fire at most once every few minutes.
-    const runInterval = prefs.getIntPref(RUN_INTERVAL_PREF);
+    const runInterval = Services.prefs.getIntPref(RUN_INTERVAL_PREF);
     timerManager.registerTimer(TIMER_NAME, () => this.run(), runInterval);
   },
 
   async run() {
     this.clearCaches();
     // Unless lazy classification is enabled, prep the classify cache.
-    if (!Preferences.get("extensions.shield-recipe-client.experiments.lazy_classify", false)) {
+    if (!Services.prefs.getBoolPref(LAZY_CLASSIFY_PREF, false)) {
       try {
         await ClientEnvironment.getClientClassification();
       } catch (err) {
         // Try to go on without this data; the filter expressions will
         // gracefully fail without this info if they need it.
       }
     }
 
     // Fetch recipes before execution in case we fail and exit early.
     let recipes;
     try {
       recipes = await NormandyApi.fetchRecipes({enabled: true});
     } catch (e) {
-      const apiUrl = prefs.getCharPref("api_url");
+      const apiUrl = Services.prefs.getCharPref(API_URL_PREF);
       log.error(`Could not fetch recipes from ${apiUrl}: "${e}"`);
 
       let status = Uptake.RUNNER_SERVER_ERROR;
       if (/NetworkError/.test(e)) {
         status = Uptake.RUNNER_NETWORK_ERROR;
       } else if (e instanceof NormandyApi.InvalidSignatureError) {
         status = Uptake.RUNNER_INVALID_SIGNATURE;
       }
@@ -292,21 +351,21 @@ this.RecipeRunner = {
   },
 
   /**
    * Clear out cached state and fetch/execute recipes from the given
    * API url. This is used mainly by the mock-recipe-server JS that is
    * executed in the browser console.
    */
   async testRun(baseApiUrl) {
-    const oldApiUrl = prefs.getCharPref("api_url");
-    prefs.setCharPref("api_url", baseApiUrl);
+    const oldApiUrl = Services.prefs.getCharPref(API_URL_PREF);
+    Services.prefs.setCharPref(API_URL_PREF, baseApiUrl);
 
     try {
       Storage.clearAllStorage();
       this.clearCaches();
       await this.run();
     } finally {
-      prefs.setCharPref("api_url", oldApiUrl);
+      Services.prefs.setCharPref(API_URL_PREF, oldApiUrl);
       this.clearCaches();
     }
   },
 };
--- a/browser/extensions/shield-recipe-client/test/browser/browser_RecipeRunner.js
+++ b/browser/extensions/shield-recipe-client/test/browser/browser_RecipeRunner.js
@@ -319,59 +319,146 @@ add_task(withMockNormandyApi(async funct
 
   const normalManager = managers.normalAction;
   ok(
     await normalManager.evalInSandbox("window.scriptRan"),
     "Implementations are run in the sandbox",
   );
 }));
 
+// test init() in dev mode
 decorate_task(
   withPrefEnv({
     set: [
       ["extensions.shield-recipe-client.dev_mode", true],
       ["extensions.shield-recipe-client.first_run", false],
     ],
   }),
   withStub(RecipeRunner, "run"),
   withStub(RecipeRunner, "registerTimer"),
   async function testInitDevMode(runStub, registerTimerStub, updateRunIntervalStub) {
     await RecipeRunner.init();
     ok(runStub.called, "RecipeRunner.run is called immediately when in dev mode");
     ok(registerTimerStub.called, "RecipeRunner.init registers a timer");
   }
 );
 
+// Test init() during normal operation
 decorate_task(
   withPrefEnv({
     set: [
       ["extensions.shield-recipe-client.dev_mode", false],
       ["extensions.shield-recipe-client.first_run", false],
     ],
   }),
   withStub(RecipeRunner, "run"),
   withStub(RecipeRunner, "registerTimer"),
   async function testInit(runStub, registerTimerStub) {
     await RecipeRunner.init();
     ok(!runStub.called, "RecipeRunner.run is called immediately when not in dev mode or first run");
     ok(registerTimerStub.called, "RecipeRunner.init registers a timer");
   }
 );
 
+// Test init() first run
 decorate_task(
   withPrefEnv({
     set: [
       ["extensions.shield-recipe-client.dev_mode", false],
       ["extensions.shield-recipe-client.first_run", true],
+      ["extensions.shield-recipe-client.api_url", "https://example.com"],
     ],
   }),
   withStub(RecipeRunner, "run"),
   withStub(RecipeRunner, "registerTimer"),
-  async function testInitFirstRun(runStub, registerTimerStub) {
+  withStub(RecipeRunner, "watchPrefs"),
+  async function testInitFirstRun(runStub, registerTimerStub, watchPrefsStub) {
     await RecipeRunner.init();
     ok(runStub.called, "RecipeRunner.run is called immediately on first run");
     ok(
       !Services.prefs.getBoolPref("extensions.shield-recipe-client.first_run"),
       "On first run, the first run pref is set to false"
     );
     ok(registerTimerStub.called, "RecipeRunner.registerTimer registers a timer");
+
+    // RecipeRunner.init() sets this to false, but SpecialPowers
+    // relies on the preferences it manages to actually change when it
+    // tries to change them. Settings this back to true here allows
+    // that to happen. Not doing this causes popPrefEnv to hang forever.
+    Services.prefs.setBoolPref("extensions.shield-recipe-client.first_run", true);
   }
 );
+
+// Test that prefs are watched correctly
+decorate_task(
+  withPrefEnv({
+    set: [
+      ["datareporting.healthreport.uploadEnabled", true],  // telemetry enabled
+      ["extensions.shield-recipe-client.dev_mode", false],
+      ["extensions.shield-recipe-client.first_run", false],
+      ["extensions.shield-recipe-client.enabled", true],
+      ["extensions.shield-recipe-client.api_url", "https://example.com"], // starts with "https://"
+    ],
+  }),
+  withStub(RecipeRunner, "run"),
+  withStub(RecipeRunner, "enable"),
+  withStub(RecipeRunner, "disable"),
+  withStub(CleanupManager, "addCleanupHandler"),
+
+  async function testPrefWatching(runStub, enableStub, disableStub, addCleanupHandlerStub) {
+    await RecipeRunner.init();
+    is(enableStub.callCount, 1, "Enable should be called initially");
+    is(disableStub.callCount, 0, "Disable should not be called initially");
+
+    await SpecialPowers.pushPrefEnv({ set: [["extensions.shield-recipe-client.enabled", false]] });
+    is(enableStub.callCount, 1, "Enable should not be called again");
+    is(disableStub.callCount, 1, "RecipeRunner should disable when Shield is disabled");
+
+    await SpecialPowers.pushPrefEnv({ set: [["extensions.shield-recipe-client.enabled", true]] });
+    is(enableStub.callCount, 2, "RecipeRunner should re-enable when Shield is enabled");
+    is(disableStub.callCount, 1, "Disable should not be called again");
+
+    await SpecialPowers.pushPrefEnv({ set: [["extensions.shield-recipe-client.api_url", "http://example.com"]] }); // does not start with https://
+    is(enableStub.callCount, 2, "Enable should not be called again");
+    is(disableStub.callCount, 2, "RecipeRunner should disable when an invalid api url is given");
+
+    await SpecialPowers.pushPrefEnv({ set: [["extensions.shield-recipe-client.api_url", "https://example.com"]] }); // ends with https://
+    is(enableStub.callCount, 3, "RecipeRunner should re-enable when a valid api url is given");
+    is(disableStub.callCount, 2, "Disable should not be called again");
+
+    await SpecialPowers.pushPrefEnv({ set: [["datareporting.healthreport.uploadEnabled", false]] });
+    is(enableStub.callCount, 3, "Enable should not be called again");
+    is(disableStub.callCount, 3, "RecipeRunner should disable when telemetry is disabled");
+
+    await SpecialPowers.pushPrefEnv({ set: [["datareporting.healthreport.uploadEnabled", true]] });
+    is(enableStub.callCount, 4, "RecipeRunner should re-enable when telemetry is enabled");
+    is(disableStub.callCount, 3, "Disable should not be called again");
+
+    is(runStub.callCount, 0, "RecipeRunner.run should not be called during this test");
+  }
+);
+
+// Test that enable and disable are idempotent
+decorate_task(
+  withStub(RecipeRunner, "registerTimer"),
+  withStub(RecipeRunner, "unregisterTimer"),
+
+  async function testPrefWatching(registerTimerStub, unregisterTimerStub) {
+    const originalEnabled = RecipeRunner.enabled;
+
+    try {
+      RecipeRunner.enabled = false;
+      RecipeRunner.enable();
+      RecipeRunner.enable();
+      RecipeRunner.enable();
+      is(registerTimerStub.callCount, 1, "Enable should be idempotent");
+
+      RecipeRunner.enabled = true;
+      RecipeRunner.disable();
+      RecipeRunner.disable();
+      RecipeRunner.disable();
+      is(registerTimerStub.callCount, 1, "Disable should be idempotent");
+
+    } finally {
+      RecipeRunner.enabled = originalEnabled;
+    }
+  }
+);
--- a/browser/extensions/shield-recipe-client/test/unit/head_xpc.js
+++ b/browser/extensions/shield-recipe-client/test/unit/head_xpc.js
@@ -1,12 +1,10 @@
 "use strict";
 
-// List these manually due to bug 1366719.
-/* global Cc, Ci, Cu */
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 Cu.import("resource://gre/modules/Services.jsm");
 
 // Load our bootstrap extension manifest so we can access our chrome/resource URIs.
 // Cargo culted from formautofill system add-on
 const EXTENSION_ID = "shield-recipe-client@mozilla.org";
 let extensionDir = Services.dirsvc.get("GreD", Ci.nsIFile);