Bug 837292 - Part 2: Report new measurement version only; r=rnewman

services/healthreport/healthreporter.jsm

@@ -644,8 +644,14 @@ AbstractHealthReporter.prototype = Object.freeze({
           measurements: {},
         };
 
+        // Measurement name to recorded version.
+        let lastVersions = {};
+        // Day string to mapping of measurement name to recorded version.
+        let dayVersions = {};
+
         for (let [measurementKey, measurement] of provider.measurements) {
           let name = providerName + "." + measurement.name;
+          let version = measurement.version;
 
           let serializer;
           try {
@@ -669,7 +675,15 @@ AbstractHealthReporter.prototype = Object.freeze({
 
           if (data.singular.size) {
             try {
-              o.data.last[name] = serializer.singular(data.singular);
+              let serialized = serializer.singular(data.singular);
+              if (serialized) {
+                // Only replace the existing data if there is no data or if our
+                // version is newer than the old one.
+                if (!(name in o.data.last) || version > lastVersions[name]) {
+                  o.data.last[name] = serialized;
+                  lastVersions[name] = version;
+                }
+              }
             } catch (ex) {
               this._recordError("Error serializing singular data: " + name,
                                 ex);
@@ -693,9 +707,14 @@ AbstractHealthReporter.prototype = Object.freeze({
 
               if (!(dateFormatted in outputDataDays)) {
                 outputDataDays[dateFormatted] = {};
+                dayVersions[dateFormatted] = {};
               }
 
+              if (!(name in outputDataDays[dateFormatted]) ||
+                  version > dayVersions[dateFormatted][name]) {
                 outputDataDays[dateFormatted][name] = serialized;
+                dayVersions[dateFormatted][name] = version;
+              }
             } catch (ex) {
               this._recordError("Error populating data for day: " + name, ex);
               continue;

services/healthreport/tests/xpcshell/test_healthreporter.js

@@ -8,6 +8,7 @@ const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 Cu.import("resource://services-common/observers.js");
 Cu.import("resource://services-common/preferences.js");
 Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
+Cu.import("resource://gre/modules/Metrics.jsm");
 Cu.import("resource://gre/modules/services/healthreport/healthreporter.jsm");
 Cu.import("resource://gre/modules/services/datareporting/policy.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
@@ -396,6 +397,83 @@ add_task(function test_json_payload_multiple_days() {
   }
 });
 
+add_task(function test_json_payload_newer_version_overwrites() {
+  let reporter = yield getReporter("json_payload_newer_version_overwrites");
+
+  try {
+    let now = new Date();
+    // Instead of hacking up the internals to ensure consistent order in Map
+    // iteration (which would be difficult), we instead opt to generate a lot
+    // of measurements of different versions and verify their iterable order
+    // is not increasing.
+    let versions = [1, 6, 3, 9, 2, 3, 7, 4, 10, 8];
+    let protos = [];
+    for (let version of versions) {
+      let m = function () {
+        Metrics.Measurement.call(this);
+      };
+      m.prototype = {
+        __proto__: DummyMeasurement.prototype,
+        name: "DummyMeasurement",
+        version: version,
+      };
+
+      protos.push(m);
+    }
+
+    let ctor = function () {
+      Metrics.Provider.call(this);
+    };
+    ctor.prototype = {
+      __proto__: DummyProvider.prototype,
+
+      name: "MultiMeasurementProvider",
+      measurementTypes: protos,
+    };
+
+    let provider = new ctor();
+
+    yield reporter._providerManager.registerProvider(provider);
+
+    let haveUnordered = false;
+    let last = -1;
+    let highestVersion = -1;
+    for (let [key, measurement] of provider.measurements) {
+      yield measurement.setDailyLastNumeric("daily-last-numeric",
+                                            measurement.version, now);
+      yield measurement.setLastNumeric("last-numeric",
+                                       measurement.version, now);
+
+      if (measurement.version > highestVersion) {
+        highestVersion = measurement.version;
+      }
+
+      if (measurement.version < last) {
+        haveUnordered = true;
+      }
+
+      last = measurement.version;
+    }
+
+    // Ensure Map traversal isn't ordered. If this ever fails, then we'll need
+    // to monkeypatch.
+    do_check_true(haveUnordered);
+
+    let payload = yield reporter.getJSONPayload();
+    let o = JSON.parse(payload);
+    do_check_true("MultiMeasurementProvider.DummyMeasurement" in o.data.last);
+    do_check_eq(o.data.last["MultiMeasurementProvider.DummyMeasurement"]._v, highestVersion);
+
+    let day = reporter._formatDate(now);
+    do_check_true(day in o.data.days);
+    do_check_true("MultiMeasurementProvider.DummyMeasurement" in o.data.days[day]);
+    do_check_eq(o.data.days[day]["MultiMeasurementProvider.DummyMeasurement"]._v, highestVersion);
+
+  } finally {
+    reporter._shutdown();
+  }
+});
+
 add_task(function test_idle_daily() {
   let reporter = yield getReporter("idle_daily");
   try {