Bug 1245833 - Don't send empty comments or email addresses in content crash reports. r?felipe draft
authorMike Conley <mconley@mozilla.com>
Thu, 04 Feb 2016 16:14:19 -0500
changeset 328971 d0ab57dc5517132c060d264a71e44dabfdf57294
parent 328969 5e024441510f6d2b460e570d0e6d2dee0dc89723
child 513885 c41349a48d358bbbcaa51806971e120f11cabc0b
push id10444
push usermconley@mozilla.com
push dateThu, 04 Feb 2016 21:28:27 +0000
reviewersfelipe
bugs1245833
milestone47.0a1
Bug 1245833 - Don't send empty comments or email addresses in content crash reports. r?felipe
browser/base/content/test/general/browser_aboutTabCrashed.js
browser/base/content/test/general/head.js
browser/modules/ContentCrashHandlers.jsm
--- a/browser/base/content/test/general/browser_aboutTabCrashed.js
+++ b/browser/base/content/test/general/browser_aboutTabCrashed.js
@@ -120,73 +120,73 @@ function crashTabTestHelper(fieldValues,
 
 /**
  * Tests what we send with the crash report by default. By default, we do not
  * send any comments, the URL of the crashing page, or the email address of
  * the user.
  */
 add_task(function* test_default() {
   yield crashTabTestHelper({}, {
-    "Comments": "",
+    "Comments": null,
     "URL": "",
-    "Email": "",
+    "Email": null,
   });
 });
 
 /**
  * Test just sending a comment.
  */
 add_task(function* test_just_a_comment() {
   yield crashTabTestHelper({
     comments: COMMENTS,
   }, {
     "Comments": COMMENTS,
     "URL": "",
-    "Email": "",
+    "Email": null,
   });
 });
 
 /**
  * Test that we don't send email if emailMe is unchecked
  */
 add_task(function* test_no_email() {
   yield crashTabTestHelper({
     email: EMAIL,
     emailMe: false,
   }, {
-    "Comments": "",
+    "Comments": null,
     "URL": "",
-    "Email": "",
+    "Email": null,
   });
 });
 
 /**
  * Test that we can send an email address if emailMe is checked
  */
 add_task(function* test_yes_email() {
   yield crashTabTestHelper({
     email: EMAIL,
     emailMe: true,
   }, {
-    "Comments": "",
+    "Comments": null,
     "URL": "",
     "Email": EMAIL,
   });
 });
 
 /**
  * Test that we will send the URL of the page if includeURL is checked.
  */
 add_task(function* test_send_URL() {
   yield crashTabTestHelper({
     includeURL: true,
   }, {
-    "Comments": "",
+    "Comments": null,
     "URL": PAGE,
-    "Email": "",
+    "Email": null,
   });
 });
 
 /**
  * Test that we can send comments, the email address, and the URL
  */
 add_task(function* test_send_all() {
   yield crashTabTestHelper({
@@ -195,8 +195,9 @@ add_task(function* test_send_all() {
     email: EMAIL,
     comments: COMMENTS,
   }, {
     "Comments": COMMENTS,
     "URL": PAGE,
     "Email": EMAIL,
   });
 });
+
--- a/browser/base/content/test/general/head.js
+++ b/browser/base/content/test/general/head.js
@@ -1144,29 +1144,34 @@ function getPropertyBagValue(bag, key) {
   return null;
 }
 
 /**
  * Returns a Promise that resolves once a crash report has
  * been submitted. This function will also test the crash
  * reports extra data to see if it matches expectedExtra.
  *
- * @param expectedExtra
+ * @param expectedExtra (object)
  *        An Object whose key-value pairs will be compared
  *        against the key-value pairs in the extra data of the
  *        crash report. A test failure will occur if there is
  *        a mismatch.
  *
- *        Note that this will only check the values that exist
+ *        If the value of the key-value pair is "null", this will
+ *        be interpreted as "this key should not be included in the
+ *        extra data", and will cause a test failure if it is detected
+ *        in the crash report.
+ *
+ *        Note that this will ignore any keys that are not included
  *        in expectedExtra. It's possible that the crash report
  *        will contain other extra information that is not
  *        compared against.
  * @returns Promise
  */
-function promiseCrashReport(expectedExtra) {
+function promiseCrashReport(expectedExtra={}) {
   return Task.spawn(function*() {
     info("Starting wait on crash-report-status");
     let [subject, data] =
       yield TestUtils.topicObserved("crash-report-status", (subject, data) => {
         return data == "success";
       });
     info("Topic observed!");
 
@@ -1195,14 +1200,18 @@ function promiseCrashReport(expectedExtr
     }
 
     info("Iterating crash report extra keys");
     let enumerator = extra.enumerator;
     while (enumerator.hasMoreElements()) {
       let key = enumerator.getNext().QueryInterface(Ci.nsIProperty).name;
       let value = extra.getPropertyAsAString(key);
       if (key in expectedExtra) {
-        is(value, expectedExtra[key],
-           `Crash report had the right extra value for ${key}`);
+        if (expectedExtra[key] == null) {
+          ok(false, `Got unexpected key ${key} with value ${value}`);
+        } else {
+          is(value, expectedExtra[key],
+             `Crash report had the right extra value for ${key}`);
+        }
       }
     }
   });
 }
--- a/browser/modules/ContentCrashHandlers.jsm
+++ b/browser/modules/ContentCrashHandlers.jsm
@@ -158,23 +158,41 @@ this.TabCrashHandler = {
     let {
       includeURL,
       comments,
       email,
       emailMe,
       URL,
     } = message.data;
 
+    let extraExtraKeyVals = {
+      "Comments": comments,
+      "Email": email,
+      "URL": URL,
+    };
+
+    // For the entries in extraExtraKeyVals, we only want to submit the
+    // extra data values where they are not the empty string.
+    for (let key in extraExtraKeyVals) {
+      let val = extraExtraKeyVals[key].trim();
+      if (!val) {
+        delete extraExtraKeyVals[key];
+      }
+    }
+
+    // URL is special, since it's already been written to extra data by
+    // default. In order to make sure we don't send it, we overwrite it
+    // with the empty string.
+    if (!includeURL) {
+      extraExtraKeyVals["URL"] = "";
+    }
+
     CrashSubmit.submit(dumpID, {
       recordSubmission: true,
-      extraExtraKeyVals: {
-        Comments: comments,
-        Email: email,
-        URL: URL,
-      },
+      extraExtraKeyVals,
     }).then(null, Cu.reportError);
 
     this.prefs.setBoolPref("sendReport", true);
     this.prefs.setBoolPref("includeURL", includeURL);
     this.prefs.setBoolPref("emailMe", emailMe);
     if (emailMe) {
       this.prefs.setCharPref("email", email);
     } else {