Bug 1293656 - Store all crash annotations in the crash manager's database and filter out privacy-sensitive ones when sending a crash ping r=bsmedberg
☠☠ backed out by 93d785448a69 ☠ ☠
authorGabriele Svelto <gsvelto@mozilla.com>
Wed, 19 Oct 2016 12:48:19 +0200
changeset 325293 f59861923f8169c003f0f7f8bd50e3fae52e0b51
parent 325292 b997bb5bccb4da93ce1f5d3453ca562324ab1d5f
child 325294 c8545ffbd0cb18f7ae3a5810f5b98ec0ba8be0e0
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewersbsmedberg
bugs1293656
milestone53.0a1
Bug 1293656 - Store all crash annotations in the crash manager's database and filter out privacy-sensitive ones when sending a crash ping r=bsmedberg
toolkit/components/crashes/CrashManager.jsm
toolkit/components/crashes/docs/index.rst
toolkit/components/crashes/tests/xpcshell/test_crash_manager.js
toolkit/crashreporter/nsExceptionHandler.cpp
--- a/toolkit/components/crashes/CrashManager.jsm
+++ b/toolkit/components/crashes/CrashManager.jsm
@@ -34,16 +34,46 @@ const MILLISECONDS_IN_DAY = 24 * 60 * 60
 
 // Converts Date to days since UNIX epoch.
 // This was copied from /services/metrics.storage.jsm. The implementation
 // does not account for leap seconds.
 function dateToDays(date) {
   return Math.floor(date.getTime() / MILLISECONDS_IN_DAY);
 }
 
+/**
+ * Parse the string stored in the specified field as JSON and then remove the
+ * field from the object. The string might also be returned without parsing.
+ *
+ * @param obj {Object} The object holding the field
+ * @param field {String} The name of the field to be parsed and removed
+ * @param [parseAsJson=true] {Boolean} If true parse the field's contents as if
+ *        it were JSON code, otherwise return the rew string.
+ *
+ * @returns {Object|String} the parsed object or the raw string
+ */
+function parseAndRemoveField(obj, field, parseAsJson = true) {
+  let value = null;
+
+  if (field in obj) {
+    if (!parseAsJson) {
+      value = obj[field];
+    } else {
+      try {
+        value = JSON.parse(obj[field]);
+      } catch (e) {
+        Cu.reportError(e);
+      }
+    }
+
+    delete obj[field];
+  }
+
+  return value;
+}
 
 /**
  * A gateway to crash-related data.
  *
  * This type is generic and can be instantiated any number of times.
  * However, most applications will typically only have one instance
  * instantiated and that instance will point to profile and user appdata
  * directories.
@@ -169,16 +199,51 @@ this.CrashManager.prototype = Object.fre
   // The following are return codes for individual event file processing.
   // File processed OK.
   EVENT_FILE_SUCCESS: "ok",
   // The event appears to be malformed.
   EVENT_FILE_ERROR_MALFORMED: "malformed",
   // The type of event is unknown.
   EVENT_FILE_ERROR_UNKNOWN_EVENT: "unknown-event",
 
+  // A whitelist of crash annotations which do not contain sensitive data
+  // and are saved in the crash record and sent with Firefox Health Report.
+  ANNOTATION_WHITELIST: [
+    "AsyncShutdownTimeout",
+    "BuildID",
+    "ProductID",
+    "ProductName",
+    "ReleaseChannel",
+    "SecondsSinceLastCrash",
+    "ShutdownProgress",
+    "StartupCrash",
+    "TelemetryEnvironment",
+    "Version",
+    // The following entries are not normal annotations that can be found in
+    // the .extra file but are included in the crash record/FHR:
+    "AvailablePageFile",
+    "AvailablePhysicalMemory",
+    "AvailableVirtualMemory",
+    "BlockedDllList",
+    "BlocklistInitFailed",
+    "ContainsMemoryReport",
+    "CrashTime",
+    "EventLoopNestingLevel",
+    "IsGarbageCollecting",
+    "MozCrashReason",
+    "OOMAllocationSize",
+    "SystemMemoryUsePercentage",
+    "TextureUsage",
+    "TotalPageFile",
+    "TotalPhysicalMemory",
+    "TotalVirtualMemory",
+    "UptimeTS",
+    "User32BeforeBlocklist",
+  ],
+
   /**
    * Obtain a list of all dumps pending upload.
    *
    * The returned value is a promise that resolves to an array of objects
    * on success. Each element in the array has the following properties:
    *
    *   id (string)
    *      The ID of the crash (a UUID).
@@ -539,16 +604,61 @@ this.CrashManager.prototype = Object.fre
       }
       let date = new Date(time * 1000);
       let payload = data.substring(start);
 
       return this._handleEventFilePayload(store, entry, type, date, payload);
     }.bind(this));
   },
 
+  _filterAnnotations: function (annotations) {
+    let filteredAnnotations = {};
+
+    for (let line in annotations) {
+      if (this.ANNOTATION_WHITELIST.includes(line)) {
+        filteredAnnotations[line] = annotations[line];
+      }
+    }
+
+    return filteredAnnotations;
+  },
+
+  _sendCrashPing: function (crashId, type, date, metadata) {
+    // If we have a saved environment, use it. Otherwise report
+    // the current environment.
+    let reportMeta = Cu.cloneInto(metadata, myScope);
+    let crashEnvironment = parseAndRemoveField(reportMeta,
+                                               "TelemetryEnvironment");
+    let sessionId = parseAndRemoveField(reportMeta, "TelemetrySessionId",
+                                        /* parseAsJson */ false);
+    let stackTraces = parseAndRemoveField(reportMeta, "StackTraces");
+
+    // Filter the remaining annotations to remove privacy-sensitive ones
+    reportMeta = this._filterAnnotations(reportMeta);
+
+    TelemetryController.submitExternalPing("crash",
+      {
+        version: 1,
+        crashDate: date.toISOString().slice(0, 10), // YYYY-MM-DD
+        sessionId: sessionId,
+        crashId: crashId,
+        processType: type,
+        stackTraces: stackTraces,
+        metadata: reportMeta,
+        hasCrashEnvironment: (crashEnvironment !== null),
+      },
+      {
+        retentionDays: 180,
+        addClientId: true,
+        addEnvironment: true,
+        overrideEnvironment: crashEnvironment,
+      }
+    );
+  },
+
   _handleEventFilePayload: function(store, entry, type, date, payload) {
       // The payload types and formats are documented in docs/crash-events.rst.
       // Do not change the format of an existing type. Instead, invent a new
       // type.
       // DO NOT ADD NEW TYPES WITHOUT DOCUMENTING!
       let lines = payload.split("\n");
 
       switch (type) {
@@ -560,58 +670,17 @@ this.CrashManager.prototype = Object.fre
           }
           // fall-through
         case "crash.main.2":
           let crashID = lines[0];
           let metadata = parseKeyValuePairsFromLines(lines.slice(1));
           store.addCrash(this.PROCESS_TYPE_MAIN, this.CRASH_TYPE_CRASH,
                          crashID, date, metadata);
 
-          // If we have a saved environment, use it. Otherwise report
-          // the current environment.
-          let crashEnvironment = null;
-          let sessionId = null;
-          let stackTraces = null;
-          let reportMeta = Cu.cloneInto(metadata, myScope);
-          if ('TelemetryEnvironment' in reportMeta) {
-            try {
-              crashEnvironment = JSON.parse(reportMeta.TelemetryEnvironment);
-            } catch (e) {
-              Cu.reportError(e);
-            }
-            delete reportMeta.TelemetryEnvironment;
-          }
-          if ('TelemetrySessionId' in reportMeta) {
-            sessionId = reportMeta.TelemetrySessionId;
-            delete reportMeta.TelemetrySessionId;
-          }
-          if ('StackTraces' in reportMeta) {
-            try {
-              stackTraces = JSON.parse(reportMeta.StackTraces);
-            } catch (e) {
-              Cu.reportError(e);
-            }
-            delete reportMeta.StackTraces;
-          }
-          TelemetryController.submitExternalPing("crash",
-            {
-              version: 1,
-              crashDate: date.toISOString().slice(0, 10), // YYYY-MM-DD
-              sessionId: sessionId,
-              crashId: entry.id,
-              stackTraces: stackTraces,
-              metadata: reportMeta,
-              hasCrashEnvironment: (crashEnvironment !== null),
-            },
-            {
-              retentionDays: 180,
-              addClientId: true,
-              addEnvironment: true,
-              overrideEnvironment: crashEnvironment,
-            });
+          this._sendCrashPing(crashID, this.PROCESS_TYPE_MAIN, date, metadata);
           break;
 
         case "crash.submission.1":
           if (lines.length == 3) {
             let [crashID, result, remoteID] = lines;
             store.addCrash(this.PROCESS_TYPE_MAIN, this.CRASH_TYPE_CRASH,
                            crashID, date);
 
--- a/toolkit/components/crashes/docs/index.rst
+++ b/toolkit/components/crashes/docs/index.rst
@@ -10,15 +10,21 @@ data within the Gecko application.
 From JavaScript, the service can be accessed via::
 
    Cu.import("resource://gre/modules/Services.jsm");
    let crashManager = Services.crashmanager;
 
 That will give you an instance of ``CrashManager`` from ``CrashManager.jsm``.
 From there, you can access and manipulate crash data.
 
+The crash manager stores statistical information about crashes as well as
+detailed information for both browser and content crashes. The crash manager
+automatically detects new browser crashes at startup by scanning for
+:ref:`crash-events`. Content process crash information on the other hand is
+provided externally.
+
 Other Documents
 ===============
 
 .. toctree::
    :maxdepth: 1
 
    crash-events
--- a/toolkit/components/crashes/tests/xpcshell/test_crash_manager.js
+++ b/toolkit/components/crashes/tests/xpcshell/test_crash_manager.js
@@ -204,82 +204,100 @@ add_task(function* test_schedule_mainten
   yield m.createEventsFile("2", "crash.main.2", oldDate, "id2");
 
   yield m.scheduleMaintenance(25);
   let crashes = yield m.getCrashes();
   Assert.equal(crashes.length, 1);
   Assert.equal(crashes[0].id, "id1");
 });
 
+const crashId = "3cb67eba-0dc7-6f78-6a569a0e-172287ec";
+const productName = "Firefox";
+const productId = "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}";
+
 add_task(function* test_main_crash_event_file() {
   let ac = new TelemetryArchiveTesting.Checker();
   yield ac.promiseInit();
   let theEnvironment = TelemetryEnvironment.currentEnvironment;
-  let sessionId = "be66af2f-2ee5-4330-ae95-44462dfbdf0c";
+  const sessionId = "be66af2f-2ee5-4330-ae95-44462dfbdf0c";
   let stackTraces = { status: "OK" };
 
   // To test proper escaping, add data to the environment with an embedded
   // double-quote
   theEnvironment.testValue = "MyValue\"";
 
   let m = yield getManager();
-  const fileContent = "id1\nk1=v1\nk2=v2\n" +
+  const fileContent = crashId + "\n" +
+    "ProductName=" + productName + "\n" +
+    "ProductID=" + productId + "\n" +
     "TelemetryEnvironment=" + JSON.stringify(theEnvironment) + "\n" +
     "TelemetrySessionId=" + sessionId + "\n" +
-    "StackTraces=" + JSON.stringify(stackTraces) + "\n";
+    "StackTraces=" + JSON.stringify(stackTraces) + "\n" +
+    "ThisShouldNot=end-up-in-the-ping\n";
 
-  yield m.createEventsFile("1", "crash.main.2", DUMMY_DATE, fileContent);
+  yield m.createEventsFile(crashId, "crash.main.2", DUMMY_DATE, fileContent);
   let count = yield m.aggregateEventsFiles();
   Assert.equal(count, 1);
 
   let crashes = yield m.getCrashes();
   Assert.equal(crashes.length, 1);
-  Assert.equal(crashes[0].id, "id1");
+  Assert.equal(crashes[0].id, crashId);
   Assert.equal(crashes[0].type, "main-crash");
-  Assert.equal(crashes[0].metadata.k1, "v1");
-  Assert.equal(crashes[0].metadata.k2, "v2");
+  Assert.equal(crashes[0].metadata.ProductName, productName);
+  Assert.equal(crashes[0].metadata.ProductID, productId);
   Assert.ok(crashes[0].metadata.TelemetryEnvironment);
-  Assert.equal(Object.getOwnPropertyNames(crashes[0].metadata).length, 5);
+  Assert.equal(Object.getOwnPropertyNames(crashes[0].metadata).length, 6);
   Assert.equal(crashes[0].metadata.TelemetrySessionId, sessionId);
   Assert.ok(crashes[0].metadata.StackTraces);
   Assert.deepEqual(crashes[0].crashDate, DUMMY_DATE);
 
   let found = yield ac.promiseFindPing("crash", [
     [["payload", "hasCrashEnvironment"], true],
-    [["payload", "metadata", "k1"], "v1"],
-    [["payload", "crashId"], "1"],
+    [["payload", "metadata", "ProductName"], productName],
+    [["payload", "metadata", "ProductID"], productId],
+    [["payload", "crashId"], crashId],
     [["payload", "stackTraces", "status"], "OK"],
     [["payload", "sessionId"], sessionId],
   ]);
   Assert.ok(found, "Telemetry ping submitted for found crash");
-  Assert.deepEqual(found.environment, theEnvironment, "The saved environment should be present");
+  Assert.deepEqual(found.environment, theEnvironment,
+                   "The saved environment should be present");
+  Assert.equal(found.payload.metadata.ThisShouldNot, undefined,
+               "Non-whitelisted fields should be filtered out");
 
   count = yield m.aggregateEventsFiles();
   Assert.equal(count, 0);
 });
 
 add_task(function* test_main_crash_event_file_noenv() {
   let ac = new TelemetryArchiveTesting.Checker();
   yield ac.promiseInit();
+  const fileContent = crashId + "\n" +
+    "ProductName=" + productName + "\n" +
+    "ProductID=" + productId + "\n";
 
   let m = yield getManager();
-  yield m.createEventsFile("1", "crash.main.2", DUMMY_DATE, "id1\nk1=v3\nk2=v2");
+  yield m.createEventsFile(crashId, "crash.main.2", DUMMY_DATE, fileContent);
   let count = yield m.aggregateEventsFiles();
   Assert.equal(count, 1);
 
   let crashes = yield m.getCrashes();
   Assert.equal(crashes.length, 1);
-  Assert.equal(crashes[0].id, "id1");
+  Assert.equal(crashes[0].id, crashId);
   Assert.equal(crashes[0].type, "main-crash");
-  Assert.deepEqual(crashes[0].metadata, { k1: "v3", k2: "v2"});
+  Assert.deepEqual(crashes[0].metadata, {
+    ProductName: productName,
+    ProductID: productId
+  });
   Assert.deepEqual(crashes[0].crashDate, DUMMY_DATE);
 
   let found = yield ac.promiseFindPing("crash", [
     [["payload", "hasCrashEnvironment"], false],
-    [["payload", "metadata", "k1"], "v3"],
+    [["payload", "metadata", "ProductName"], productName],
+    [["payload", "metadata", "ProductID"], productId],
   ]);
   Assert.ok(found, "Telemetry ping submitted for found crash");
   Assert.ok(found.environment, "There is an environment");
 
   count = yield m.aggregateEventsFiles();
   Assert.equal(count, 0);
 });
 
--- a/toolkit/crashreporter/nsExceptionHandler.cpp
+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
@@ -178,44 +178,16 @@ typedef std::string xpstring;
 static const XP_CHAR dumpFileExtension[] = XP_TEXT(".dmp");
 #endif
 
 static const XP_CHAR childCrashAnnotationBaseName[] = XP_TEXT("GeckoChildCrash");
 static const XP_CHAR extraFileExtension[] = XP_TEXT(".extra");
 static const XP_CHAR memoryReportExtension[] = XP_TEXT(".memory.json.gz");
 static xpstring *defaultMemoryReportPath = nullptr;
 
-// A whitelist of crash annotations which do not contain sensitive data
-// and are saved in the crash record and sent with Firefox Health Report.
-static char const * const kCrashEventAnnotations[] = {
-  "AsyncShutdownTimeout",
-  "BuildID",
-  "ProductID",
-  "ProductName",
-  "ReleaseChannel",
-  "SecondsSinceLastCrash",
-  "ShutdownProgress",
-  "StartupCrash",
-  "TelemetryEnvironment",
-  "Version",
-  // The following entries are not normal annotations but are included
-  // in the crash record/FHR:
-  // "ContainsMemoryReport"
-  // "EventLoopNestingLevel"
-  // "IsGarbageCollecting"
-  // "AvailablePageFile"
-  // "AvailableVirtualMemory"
-  // "SystemMemoryUsePercentage"
-  // "OOMAllocationSize"
-  // "TotalPageFile"
-  // "TotalPhysicalMemory"
-  // "TotalVirtualMemory"
-  // "MozCrashReason"
-};
-
 static const char kCrashMainID[] = "crash.main.2\n";
 
 static google_breakpad::ExceptionHandler* gExceptionHandler = nullptr;
 
 static XP_CHAR* pendingDirectory;
 static XP_CHAR* crashReporterPath;
 static XP_CHAR* memoryReportPath;
 
@@ -2158,27 +2130,16 @@ static void ReplaceChar(nsCString& str, 
     str.Replace(pos - 1, 1, replacement);
 
     str.BeginReading(iter);
     iter.advance(pos + replacement.Length() - 1);
     str.EndReading(end);
   }
 }
 
-static bool
-IsInWhitelist(const nsACString& key)
-{
-  for (size_t i = 0; i < ArrayLength(kCrashEventAnnotations); ++i) {
-    if (key.EqualsASCII(kCrashEventAnnotations[i])) {
-      return true;
-    }
-  }
-  return false;
-}
-
 // This function is miscompiled with MSVC 2005/2008 when PGO is on.
 #ifdef _MSC_VER
 #pragma optimize("", off)
 #endif
 static nsresult
 EscapeAnnotation(const nsACString& key, const nsACString& data, nsCString& escapedData)
 {
   if (FindInReadable(NS_LITERAL_CSTRING("="), key) ||
@@ -2305,19 +2266,17 @@ nsresult AnnotateCrashReport(const nsACS
     const nsACString& key = it.Key();
     nsCString entry = it.Data();
     if (!entry.IsEmpty()) {
       NS_NAMED_LITERAL_CSTRING(kEquals, "=");
       NS_NAMED_LITERAL_CSTRING(kNewline, "\n");
       nsAutoCString line = key + kEquals + entry + kNewline;
 
       crashReporterAPIData->Append(line);
-      if (IsInWhitelist(key)) {
-        crashEventAPIData->Append(line);
-      }
+      crashEventAPIData->Append(line);
     }
   }
 
   return NS_OK;
 }
 
 nsresult RemoveCrashReportAnnotation(const nsACString& key)
 {