Bug 875562 - Part 9: Change Health Report to pull from crashes manager; r=bsmedberg, r=rnewman
☠☠ backed out by 56c230aeaa61 ☠ ☠
authorGregory Szorc <gps@mozilla.com>
Thu, 30 Jan 2014 16:48:52 -0800
changeset 173489 21793ea94b09b2c2df3a3cefac56ef7822e5d96f
parent 173488 5efe0f428d4b2b4cb4936e201527b2cc27eecfc2
child 173490 76d856e4ec61d8702bb5bb06351319823d34707c
push id26406
push userkwierso@gmail.com
push dateFri, 14 Mar 2014 02:09:34 +0000
treeherdermozilla-central@f073b3d6db1f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg, rnewman
bugs875562
milestone30.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 875562 - Part 9: Change Health Report to pull from crashes manager; r=bsmedberg, r=rnewman This patch changes Firefox Health Report to pull crash counts from the newly-implemented crash manager. The crash manager is now the canonical source of crash data, so all the code for reading crash dump files from disk has been removed. This regresses the collection capability of Firefox Health Report by removing plugin crashes and hangs from the reported values. This regression is intended to be temporary and a follow-up bug will be created to add plugin process event recording to the crash events system. This regression has been justified because the old crash reporting mechanism was severely flawed and wasn't sending accurate data (not all crashes were being saved to dumps and some dumps would be deleted).
services/healthreport/docs/dataformat.rst
services/healthreport/providers.jsm
services/healthreport/tests/xpcshell/test_provider_crashes.js
toolkit/components/crashes/CrashManager.jsm
toolkit/components/crashes/docs/index.rst
--- a/services/healthreport/docs/dataformat.rst
+++ b/services/healthreport/docs/dataformat.rst
@@ -1019,34 +1019,53 @@ Example
       ]
     }
 
 org.mozilla.crashes.crashes
 ---------------------------
 
 This measurement contains a historical record of application crashes.
 
+Version 2
+^^^^^^^^^
+
+The switch to version 2 coincides with the introduction of the
+:ref:`crashes_crashmanager`, which provides a more robust source of
+crash data.
+
+This measurement will be reported on each day there was a crash. The
+following fields may be present in each record:
+
+mainCrash
+   The number of main process crashes that occurred on the given day.
+
+Yes, version 2 does not track submissions like version 1. It is very
+likely submissions will be re-added later.
+
+Also absent from version 2 are plugin crashes and hangs. These will be
+re-added, likely in version 3.
+
 Version 1
 ^^^^^^^^^
 
 This measurement will be reported on each day there was a crash. The
 following properties are reported:
 
 pending
     The number of crash reports that haven't been submitted.
 submitted
     The number of crash reports that were submitted.
 
 Notes
 ^^^^^
 
-Crashes are typically submitted immediately after they occur (by checking
-a box in the crash reporter, which should appear automatically after a
-crash). If the crash reporter submits the crash successfully, we get a
-submitted crash. Else, we leave it as pending.
+Main process crashes are typically submitted immediately after they
+occur (by checking a box in the crash reporter, which should appear
+automatically after a crash). If the crash reporter submits the crash
+successfully, we get a submitted crash. Else, we leave it as pending.
 
 A pending crash does not mean it will eventually be submitted.
 
 Pending crash reports can be submitted post-crash by going to
 about:crashes.
 
 If a pending crash is submitted via about:crashes, the submitted count
 increments but the pending count does not decrement. This is because FHR
@@ -1057,16 +1076,20 @@ Example
 ^^^^^^^
 
 ::
 
     "org.mozilla.crashes.crashes": {
       "_v": 1,
       "pending": 1,
       "submitted": 2
+    },
+    "org.mozilla.crashes.crashes": {
+      "_v": 2,
+      "mainCrash": 2
     }
 
 org.mozilla.healthreport.submissions
 ------------------------------------
 
 This measurement contains a history of FHR's own data submission activity.
 It was added in Firefox 23 in early May 2013.
 
--- a/services/healthreport/providers.jsm
+++ b/services/healthreport/providers.jsm
@@ -988,196 +988,90 @@ AddonsProvider.prototype = Object.freeze
 
     data.counts["plugin"] = pluginTags.length;
 
     return data;
   },
 });
 
 
-function DailyCrashesMeasurement() {
+function DailyCrashesMeasurement1() {
   Metrics.Measurement.call(this);
 }
 
-DailyCrashesMeasurement.prototype = Object.freeze({
+DailyCrashesMeasurement1.prototype = Object.freeze({
   __proto__: Metrics.Measurement.prototype,
 
   name: "crashes",
   version: 1,
 
   fields: {
     pending: DAILY_COUNTER_FIELD,
     submitted: DAILY_COUNTER_FIELD,
   },
 });
 
+function DailyCrashesMeasurement2() {
+  Metrics.Measurement.call(this);
+}
+
+DailyCrashesMeasurement2.prototype = Object.freeze({
+  __proto__: Metrics.Measurement.prototype,
+
+  name: "crashes",
+  version: 2,
+
+  fields: {
+    mainCrash: DAILY_LAST_NUMERIC_FIELD,
+  },
+});
+
 this.CrashesProvider = function () {
   Metrics.Provider.call(this);
+
+  // So we can unit test.
+  this._manager = Services.crashmanager;
 };
 
 CrashesProvider.prototype = Object.freeze({
   __proto__: Metrics.Provider.prototype,
 
   name: "org.mozilla.crashes",
 
-  measurementTypes: [DailyCrashesMeasurement],
+  measurementTypes: [
+    DailyCrashesMeasurement1,
+    DailyCrashesMeasurement2,
+  ],
 
   pullOnly: true,
 
-  collectConstantData: function () {
+  collectDailyData: function () {
     return this.storage.enqueueTransaction(this._populateCrashCounts.bind(this));
   },
 
   _populateCrashCounts: function () {
-    let now = new Date();
-    let service = new CrashDirectoryService();
-
-    let pending = yield service.getPendingFiles();
-    let submitted = yield service.getSubmittedFiles();
-
-    function getAgeLimit() {
-      return 0;
-    }
-
-    let lastCheck = yield this.getState("lastCheck");
-    if (!lastCheck) {
-      lastCheck = getAgeLimit();
-    } else {
-      lastCheck = parseInt(lastCheck, 10);
-      if (Number.isNaN(lastCheck)) {
-        lastCheck = getAgeLimit();
-      }
-    }
-
-    let m = this.getMeasurement("crashes", 1);
-
-    // Aggregate counts locally to avoid excessive storage interaction.
-    let counts = {
-      pending: new Metrics.DailyValues(),
-      submitted: new Metrics.DailyValues(),
+    this._log.info("Grabbing crash counts from crash manager.");
+    let crashCounts = yield this._manager.getCrashCountsByDay();
+    let fields = {
+      "main-crash": "mainCrash",
     };
 
-    // FUTURE detect mtimes in the future and react more intelligently.
-    for (let filename in pending) {
-      let modified = pending[filename].modified;
-
-      if (modified.getTime() < lastCheck) {
-        continue;
-      }
-
-      counts.pending.appendValue(modified, 1);
-    }
-
-    for (let filename in submitted) {
-      let modified = submitted[filename].modified;
-
-      if (modified.getTime() < lastCheck) {
-        continue;
-      }
-
-      counts.submitted.appendValue(modified, 1);
-    }
-
-    for (let [date, values] in counts.pending) {
-      yield m.incrementDailyCounter("pending", date, values.length);
-    }
-
-    for (let [date, values] in counts.submitted) {
-      yield m.incrementDailyCounter("submitted", date, values.length);
-    }
-
-    yield this.setState("lastCheck", "" + now.getTime());
-  },
-});
-
+    let m = this.getMeasurement("crashes", 2);
 
-/**
- * Helper for interacting with the crashes directory.
- *
- * FUTURE Extract to JSM alongside crashreporter. Use in about:crashes.
- */
-this.CrashDirectoryService = function () {
-  let base = Cc["@mozilla.org/file/directory_service;1"]
-               .getService(Ci.nsIProperties)
-               .get("UAppData", Ci.nsIFile);
-
-  let cr = base.clone();
-  cr.append("Crash Reports");
-
-  let submitted = cr.clone();
-  submitted.append("submitted");
-
-  let pending = cr.clone();
-  pending.append("pending");
-
-  this._baseDir = base.path;
-  this._submittedDir = submitted.path;
-  this._pendingDir = pending.path;
-};
-
-CrashDirectoryService.prototype = Object.freeze({
-  RE_SUBMITTED_FILENAME: /^bp-.+\.txt$/,
-  RE_PENDING_FILENAME: /^.+\.dmp$/,
-
-  getPendingFiles: function () {
-    return this._getDirectoryEntries(this._pendingDir,
-                                     this.RE_PENDING_FILENAME);
-  },
-
-  getSubmittedFiles: function () {
-    return this._getDirectoryEntries(this._submittedDir,
-                                     this.RE_SUBMITTED_FILENAME);
-  },
-
-  _getDirectoryEntries: function (path, re) {
-    let files = {};
-
-    return Task.spawn(function iterateDirectory() {
-      // If the directory doesn't exist, exit immediately. Else, re-throw
-      // any errors.
-      try {
-        yield OS.File.stat(path);
-      } catch (ex if ex instanceof OS.File.Error) {
-        if (ex.becauseNoSuchFile) {
-          throw new Task.Result({});
+    for (let [day, types] of crashCounts) {
+      let date = Metrics.daysToDate(day);
+      for (let [type, count] of types) {
+        if (!(type in fields)) {
+          this._log.warn("Unknown crash type encountered: " + type);
+          continue;
         }
 
-        throw ex;
+        yield m.setDailyLastNumeric(fields[type], count, date);
       }
-
-      let iterator = new OS.File.DirectoryIterator(path);
-
-      try {
-        while (true) {
-          let entry;
-          try {
-            entry = yield iterator.next();
-          } catch (ex if ex == StopIteration) {
-            break;
-          }
-
-          if (!entry.name.match(re)) {
-            continue;
-          }
-
-          let info = yield OS.File.stat(entry.path);
-
-          files[entry.name] = {
-            // Last modified should be adequate, because crash files aren't
-            // modified after they're first written.
-            modified: info.lastModificationDate,
-            size: info.size,
-          };
-        }
-
-        throw new Task.Result(files);
-      } finally {
-        iterator.close();
-      }
-    });
+    }
   },
 });
 
 
 /**
  * Holds basic statistics about the Places database.
  */
 function PlacesMeasurement() {
--- a/services/healthreport/tests/xpcshell/test_provider_crashes.js
+++ b/services/healthreport/tests/xpcshell/test_provider_crashes.js
@@ -3,154 +3,86 @@
 
 "use strict";
 
 const {utils: Cu} = Components;
 
 
 Cu.import("resource://gre/modules/Metrics.jsm");
 Cu.import("resource://gre/modules/services/healthreport/providers.jsm");
+Cu.import("resource://testing-common/AppData.jsm");
 Cu.import("resource://testing-common/services/healthreport/utils.jsm");
-Cu.import("resource://testing-common/AppData.jsm");
-
-
-const MILLISECONDS_PER_DAY = 24 * 60 * 60 * 1000;
+Cu.import("resource://testing-common/CrashManagerTest.jsm");
 
 
 function run_test() {
   run_next_test();
 }
 
-// run_test() needs to finish synchronously, so we do async init here.
-add_task(function test_init() {
+add_task(function* init() {
+  do_get_profile();
   yield makeFakeAppDir();
 });
 
-let gPending = {};
-let gSubmitted = {};
-
-add_task(function test_directory_service() {
-  let d = new CrashDirectoryService();
-
-  let entries = yield d.getPendingFiles();
-  do_check_eq(typeof(entries), "object");
-  do_check_eq(Object.keys(entries).length, 0);
-
-  entries = yield d.getSubmittedFiles();
-  do_check_eq(typeof(entries), "object");
-  do_check_eq(Object.keys(entries).length, 0);
-
-  let now = new Date();
-
-  // We lose granularity when writing to filesystem.
-  now.setUTCMilliseconds(0);
-  let dates = [];
-  for (let i = 0; i < 10; i++) {
-    dates.push(new Date(now.getTime() - i * MILLISECONDS_PER_DAY));
-  }
-
-  let pending = {};
-  let submitted = {};
-  for (let date of dates) {
-    pending[createFakeCrash(false, date)] = date;
-    submitted[createFakeCrash(true, date)] = date;
-  }
-
-  entries = yield d.getPendingFiles();
-  do_check_eq(Object.keys(entries).length, Object.keys(pending).length);
-  for (let id in pending) {
-    let filename = id + ".dmp";
-    do_check_true(filename in entries);
-    do_check_eq(entries[filename].modified.getTime(), pending[id].getTime());
-  }
-
-  entries = yield d.getSubmittedFiles();
-  do_check_eq(Object.keys(entries).length, Object.keys(submitted).length);
-  for (let id in submitted) {
-    let filename = "bp-" + id + ".txt";
-    do_check_true(filename in entries);
-    do_check_eq(entries[filename].modified.getTime(), submitted[id].getTime());
-  }
-
-  gPending = pending;
-  gSubmitted = submitted;
+add_task(function test_constructor() {
+  let provider = new CrashesProvider();
 });
 
-add_test(function test_constructor() {
-  let provider = new CrashesProvider();
-
-  run_next_test();
-});
-
-add_task(function test_init() {
+add_task(function* test_init() {
   let storage = yield Metrics.Storage("init");
   let provider = new CrashesProvider();
   yield provider.init(storage);
   yield provider.shutdown();
 
   yield storage.close();
 });
 
-add_task(function test_collect() {
+add_task(function* test_collect() {
   let storage = yield Metrics.Storage("collect");
   let provider = new CrashesProvider();
   yield provider.init(storage);
 
-  // FUTURE Don't rely on state from previous test.
-  yield provider.collectConstantData();
+  // Install custom manager so we don't interfere with other tests.
+  let manager = yield getManager();
+  provider._manager = manager;
+
+  let day1 = new Date(2014, 0, 1, 0, 0, 0);
+  let day2 = new Date(2014, 0, 3, 0, 0, 0);
 
-  let m = provider.getMeasurement("crashes", 1);
-  let values = yield m.getValues();
-  do_check_eq(values.days.size, Object.keys(gPending).length);
-  for each (let date in gPending) {
-    do_check_true(values.days.hasDay(date));
+  // FUTURE Bug 982836 CrashManager will grow public APIs for adding crashes.
+  // Switch to that here.
+  let store = yield manager._getStore();
+  store.addMainProcessCrash("id1", day1);
+  store.addMainProcessCrash("id2", day1);
+  store.addMainProcessCrash("id3", day2);
+
+  // Flush changes (this may not be needed but it doesn't hurt).
+  yield store.save();
+
+  yield provider.collectDailyData();
 
-    let value = values.days.getDay(date);
-    do_check_true(value.has("pending"));
-    do_check_true(value.has("submitted"));
-    do_check_eq(value.get("pending"), 1);
-    do_check_eq(value.get("submitted"), 1);
-  }
+  let m = provider.getMeasurement("crashes", 2);
+  let values = yield m.getValues();
+  do_check_eq(values.days.size, 2);
+  do_check_true(values.days.hasDay(day1));
+  do_check_true(values.days.hasDay(day2));
+
+  let value = values.days.getDay(day1);
+  do_check_true(value.has("mainCrash"));
+  do_check_eq(value.get("mainCrash"), 2);
 
-  let currentState = yield provider.getState("lastCheck");
-  do_check_eq(typeof(currentState), "string");
-  do_check_true(currentState.length > 0);
-  let lastState = currentState;
+  value = values.days.getDay(day2);
+  do_check_eq(value.get("mainCrash"), 1);
 
-  // If we collect again, we should get no new data.
-  yield provider.collectConstantData();
+  // Check that adding a new crash increments counter on next collect.
+  store = yield manager._getStore();
+  store.addMainProcessCrash("id4", day2);
+  yield store.save();
+
+  yield provider.collectDailyData();
   values = yield m.getValues();
-  for each (let date in gPending) {
-    let day = values.days.getDay(date);
-    do_check_eq(day.get("pending"), 1);
-    do_check_eq(day.get("submitted"), 1);
-  }
-
-  currentState = yield provider.getState("lastCheck");
-  do_check_neq(currentState, lastState);
-  do_check_true(currentState > lastState);
-
-  let now = new Date();
-  let tomorrow = new Date(now.getTime() + MILLISECONDS_PER_DAY);
-  let yesterday = new Date(now.getTime() - MILLISECONDS_PER_DAY);
-
-  createFakeCrash(false, yesterday);
-
-  // Create multiple to test that multiple are handled properly.
-  createFakeCrash(false, tomorrow);
-  createFakeCrash(false, tomorrow);
-  createFakeCrash(false, tomorrow);
-
-  yield provider.collectConstantData();
-  values = yield m.getValues();
-  do_check_eq(values.days.size, 11);
-  do_check_eq(values.days.getDay(tomorrow).get("pending"), 3);
-
-  for each (let date in gPending) {
-    let day = values.days.getDay(date);
-    do_check_eq(day.get("pending"), 1);
-    do_check_eq(day.get("submitted"), 1);
-  }
+  value = values.days.getDay(day2);
+  do_check_eq(value.get("mainCrash"), 2);
 
   yield provider.shutdown();
   yield storage.close();
 });
 
--- a/toolkit/components/crashes/CrashManager.jsm
+++ b/toolkit/components/crashes/CrashManager.jsm
@@ -511,16 +511,24 @@ this.CrashManager.prototype = Object.fre
    */
   getCrashes: function () {
     return Task.spawn(function* () {
       let store = yield this._getStore();
 
       return store.crashes;
     }.bind(this));
   },
+
+  getCrashCountsByDay: function () {
+    return Task.spawn(function* () {
+      let store = yield this._getStore();
+
+      return store._countsByDay;
+    }.bind(this));
+  },
 });
 
 let gCrashManager;
 
 /**
  * Interface to storage of crash data.
  *
  * This type handles storage of crash metadata. It exists as a separate type
--- a/toolkit/components/crashes/docs/index.rst
+++ b/toolkit/components/crashes/docs/index.rst
@@ -1,8 +1,10 @@
+.. _crashes_crashmanager:
+
 =============
 Crash Manager
 =============
 
 The **Crash Manager** is a service and interface for managing crash
 data within the Gecko application.
 
 From JavaScript, the service can be accessed via::