Bug 1052545 - Telemetry experiment branches are not saved correctly: we set the _dirty flag correctly, but then don't write the cache, r=gfritzsche, a=sledru
authorBenjamin Smedberg <benjamin@smedbergs.us>
Thu, 14 Aug 2014 16:21:15 -0400
changeset 217648 36adbb8b0b8386e11d1c9df823c1a8413d1d6a2d
parent 217647 16c940d832690f81d89e19d43b0b901f2b5ec9c2
child 217649 c665d9c7682d4d1026cc0585a6edb97170b47610
push id515
push userraliiev@mozilla.com
push dateMon, 06 Oct 2014 12:51:51 +0000
treeherdermozilla-release@267c7a481bef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgfritzsche, sledru
bugs1052545
milestone33.0a2
Bug 1052545 - Telemetry experiment branches are not saved correctly: we set the _dirty flag correctly, but then don't write the cache, r=gfritzsche, a=sledru
browser/experiments/Experiments.jsm
--- a/browser/experiments/Experiments.jsm
+++ b/browser/experiments/Experiments.jsm
@@ -70,16 +70,18 @@ const PREF_FORCE_SAMPLE = "force-sample-
 const PREF_HEALTHREPORT_ENABLED = "datareporting.healthreport.service.enabled";
 
 const PREF_BRANCH_TELEMETRY     = "toolkit.telemetry.";
 const PREF_TELEMETRY_ENABLED    = "enabled";
 
 const URI_EXTENSION_STRINGS     = "chrome://mozapps/locale/extensions/extensions.properties";
 const STRING_TYPE_NAME          = "type.%ID%.name";
 
+const CACHE_WRITE_RETRY_DELAY_SEC = 60 * 3;
+
 const TELEMETRY_LOG = {
   // log(key, [kind, experimentId, details])
   ACTIVATION_KEY: "EXPERIMENT_ACTIVATION",
   ACTIVATION: {
     // Successfully activated.
     ACTIVATED: "ACTIVATED",
     // Failed to install the add-on.
     INSTALL_FAILURE: "INSTALL_FAILURE",
@@ -337,20 +339,29 @@ Experiments.Policy.prototype = {
 
 function AlreadyShutdownError(message="already shut down") {
   Error.call(this, message);
   let error = new Error();
   this.name = "AlreadyShutdownError";
   this.message = message;
   this.stack = error.stack;
 }
-
 AlreadyShutdownError.prototype = Object.create(Error.prototype);
 AlreadyShutdownError.prototype.constructor = AlreadyShutdownError;
 
+function CacheWriteError(message="Error writing cache file") {
+  Error.call(this, message);
+  let error = new Error();
+  this.name = "CacheWriteError";
+  this.message = message;
+  this.stack = error.stack;
+}
+CacheWriteError.prototype = Object.create(Error.prototype);
+CacheWriteError.prototype.constructor = CacheWriteError;
+
 /**
  * Manages the experiments and provides an interface to control them.
  */
 
 Experiments.Experiments = function (policy=new Experiments.Policy()) {
   let log = Log.repository.getLoggerWithMessagePrefix(
       "Browser.Experiments.Experiments",
       "Experiments #" + gExperimentsCounter++ + "::");
@@ -696,16 +707,17 @@ Experiments.Experiments.prototype = {
    */
   setExperimentBranch: Task.async(function*(id, branchstr) {
     yield this._loadTask;
     let e = this._experiments.get(id);
     if (!e) {
       throw new Error("Experiment not found");
     }
     e.branch = String(branchstr);
+    this._log.trace("setExperimentBranch(" + id + ", " + e.branch + ") _dirty=" + this._dirty);
     this._dirty = true;
     Services.obs.notifyObservers(null, EXPERIMENTS_CHANGED_TOPIC, null);
     yield this._run();
   }),
   /**
    * Get the branch of the specified experiment. If the experiment is unknown,
    * throws an error.
    *
@@ -772,16 +784,18 @@ Experiments.Experiments.prototype = {
 
   _run: function() {
     this._log.trace("_run");
     this._checkForShutdown();
     if (!this._mainTask) {
       this._mainTask = Task.spawn(function*() {
         try {
           yield this._main();
+	} catch (e if e instanceof CacheWriteError) {
+	  // In this case we want to reschedule
         } catch (e) {
           this._log.error("_main caught error: " + e);
           return;
         } finally {
           this._mainTask = null;
         }
         this._log.trace("_main finished, scheduling next run");
         try {
@@ -807,17 +821,17 @@ Experiments.Experiments.prototype = {
       }
       yield this._evaluateExperiments();
       if (this._dirty) {
         yield this._saveToCache();
       }
       // If somebody called .updateManifest() or disableExperiment()
       // while we were running, go again right now.
     }
-    while (this._refresh || this._terminateReason);
+    while (this._refresh || this._terminateReason || this._dirty);
   },
 
   _loadManifest: function*() {
     this._log.trace("_loadManifest");
     let uri = Services.urlFormatter.formatURLPref(PREF_BRANCH + PREF_MANIFEST_URI);
 
     this._checkForShutdown();
 
@@ -1012,17 +1026,17 @@ Experiments.Experiments.prototype = {
       let encoder = new TextEncoder();
       let data = encoder.encode(textData);
       let options = { tmpPath: path + ".tmp", compression: "lz4" };
       yield OS.File.writeAtomic(path, data, options);
     } catch (e) {
       // We failed to write the cache, it's still dirty.
       this._dirty = true;
       this._log.error("_saveToCache failed and caught error: " + e);
-      return;
+      throw new CacheWriteError();
     }
 
     this._log.debug("_saveToCache saved to " + path);
   },
 
   /*
    * Task function, load the cached experiments manifest file from disk.
    */
@@ -1327,16 +1341,20 @@ Experiments.Experiments.prototype = {
     }
 
     if (!gExperimentsEnabled || this._experiments.length == 0) {
       return;
     }
 
     let time = null;
     let now = this._policy.now().getTime();
+    if (this._dirty) {
+      // If we failed to write the cache, we should try again periodically
+      time = now + 1000 * CACHE_WRITE_RETRY_DELAY_SEC;
+    }
 
     for (let [id, experiment] of this._experiments) {
       let scheduleTime = experiment.getScheduleTime();
       if (scheduleTime > now) {
         if (time !== null) {
           time = Math.min(time, scheduleTime);
         } else {
           time = scheduleTime;