Bug 1920927 - Automatically filter out crash annotations that are only used locally r=gsvelto

This adds a new `scope` field to `CrashAnnotations.yaml`, replacing the
`ping` field. It adds the `StackTraces` and `Comments` annotations
(which were missing), and an `object` annotation type for completeness.

There's a bit of reorganized code for the client UI test interaction
hook as well, and a better panic handler for the logic thread: the new
debug assertion around extra data before submission was failing because
I was missing the `Comments` annotation, however the logic thread
panicking resulted in the test hanging rather than exiting with the
panic message(s). This panic handler fixes that case (and I'm glad I
added the debug assertion!).

Differential Revision: https://phabricator.services.mozilla.com/D239078

toolkit/crashreporter/CrashAnnotations.cpp

@@ -31,12 +31,23 @@ Maybe<Annotation> AnnotationFromString(const nsACString& aValue) {
   return Some(static_cast<Annotation>(elem - begin(kAnnotationStrings)));
 }
 
-bool IsAnnotationAllowedForPing(Annotation aAnnotation) {
+template <size_t N>
+static bool AnnotationInList(Annotation aAnnotation,
+                             const Annotation (&aList)[N]) {
   const auto* elem = find_if(
-      begin(kCrashPingAllowedList), end(kCrashPingAllowedList),
+      begin(aList), end(aList),
       [&aAnnotation](Annotation aElement) { return aElement == aAnnotation; });
 
-  return elem != end(kCrashPingAllowedList);
+  return elem != end(aList);
+}
+
+bool IsAnnotationAllowedForPing(Annotation aAnnotation) {
+  return AnnotationInList(aAnnotation, kCrashPingAllowedList);
+}
+
+bool IsAnnotationAllowedForReport(Annotation aAnnotation) {
+  return AnnotationInList(aAnnotation, kCrashPingAllowedList) ||
+         AnnotationInList(aAnnotation, kCrashReportAllowedList);
 }
 
 bool ShouldIncludeAnnotation(Annotation aAnnotation, const char* aValue) {

toolkit/crashreporter/CrashAnnotations.h.in

@@ -32,6 +32,7 @@ enum class AnnotationType : uint8_t {
   U32 = 2,      // C/C++'s uint32_t or Rust's u32
   U64 = 3,      // C/C++'s uint64_t or Rust's u64
   USize = 4,    // C/C++'s size_t or Rust's usize
+  Object = 5,   // Not able to be read/written from the C++ API.
 };
 
 // Type of each annotation
@@ -41,7 +42,13 @@ ${types}
 
 // Allowlist of crash annotations that can be included in a crash ping
 const Annotation kCrashPingAllowedList[] = {
-${allowedlist}
+${pingallowedlist}
+};
+
+// Allowlist of crash annotations that can be included in a crash report
+// (excludes those in kCrashPingAllowedList).
+const Annotation kCrashReportAllowedList[] = {
+${reportallowedlist}
 };
 
 // Annotations which should be skipped when they have specific values
@@ -83,15 +90,25 @@ static inline const char* AnnotationToString(Annotation aAnnotation) {
 Maybe<Annotation> AnnotationFromString(const nsACString& aValue);
 
 /**
- * Checks if the given crash annotation is allowed for inclusion in the crash
+ * Checks if the given crash annotation is allowed for inclusion in a crash
  * ping.
  *
  * @param aAnnotation the crash annotation to be checked
- * @return true if the annotation can be included in the crash ping, false
+ * @return true if the annotation can be included in a crash ping, false
  *         otherwise
  */
 bool IsAnnotationAllowedForPing(Annotation aAnnotation);
 
+/**
+ * Checks if the given crash annotation is allowed for inclusion in a crash
+ * report.
+ *
+ * @param aAnnotation the crash annotation to be checked
+ * @return true if the annotation can be included in a crash report, false
+ *         otherwise
+ */
+bool IsAnnotationAllowedForReport(Annotation aAnnotation);
+
 /**
  * Checks if the annotation should be included. Some annotations are skipped if
  * their value matches a specific one (like the value 0).

toolkit/crashreporter/CrashSubmit.sys.mjs

@@ -125,6 +125,8 @@ Submitter.prototype = {
   },
 
   submitForm: function Submitter_submitForm() {
+    this.submittedExtraKeyVals = {};
+
     if (!("ServerURL" in this.extraKeyVals)) {
       return false;
     }
@@ -145,8 +147,15 @@ Submitter.prototype = {
     // tell the server not to throttle this if requested
     this.extraKeyVals.Throttleable = this.noThrottle ? "0" : "1";
 
-    // add the data
+    // add the data, keeping only annotations which are allowed for reports
-    let payload = Object.assign({}, this.extraKeyVals);
+    let payload = Object.fromEntries(
+      Object.entries(this.extraKeyVals).filter(
+        ([annotation, _]) =>
+          Services.appinfo.isAnnotationValid(annotation) &&
+          Services.appinfo.isAnnotationAllowedForReport(annotation)
+      )
+    );
+    this.submittedExtraKeyVals = payload;
     let json = new Blob([JSON.stringify(payload)], {
       type: "application/json",
     });
@@ -280,7 +289,7 @@ Submitter.prototype = {
     let extraKeyValsBag = Cc["@mozilla.org/hash-property-bag;1"].createInstance(
       Ci.nsIWritablePropertyBag2
     );
-    for (let key in this.extraKeyVals) {
+    for (let key in this.submittedExtraKeyVals) {
       extraKeyValsBag.setPropertyAsAString(key, this.extraKeyVals[key]);
     }
     propBag.setPropertyAsInterface("extra", extraKeyValsBag);
@@ -302,19 +311,8 @@ Submitter.prototype = {
   },
 
   readAnnotations: async function Submitter_readAnnotations(extra) {
-    // These annotations are used only by the crash reporter client and should
-    // not be submitted to Socorro.
-    const strippedAnnotations = [
-      "StackTraces",
-      "TelemetryClientId",
-      "TelemetryProfileGroupId",
-      "TelemetrySessionId",
-      "TelemetryServerURL",
-    ];
     let extraKeyVals = await IOUtils.readJSON(extra);
-
     this.extraKeyVals = { ...extraKeyVals, ...this.extraKeyVals };
-    strippedAnnotations.forEach(key => delete this.extraKeyVals[key]);
   },
 
   submit: async function Submitter_submit() {

toolkit/crashreporter/client/app/build.rs

@@ -6,7 +6,7 @@ use std::{env, path::Path};
 
 fn main() {
     windows_manifest();
-    crash_ping_annotations();
+    crash_annotations();
     set_mock_cfg();
     set_glean_metrics_file();
     generate_buildid_section();
@@ -35,8 +35,8 @@ fn windows_manifest() {
     println!("cargo:rerun-if-changed=build.rs");
 }
 
-/// Generate crash ping annotation information from the yaml definition file.
+/// Generate crash annotation information from the yaml definition file.
-fn crash_ping_annotations() {
+fn crash_annotations() {
     use std::fs::File;
     use std::io::{BufWriter, Write};
     use yaml_rust::{Yaml, YamlLoader};
@@ -46,7 +46,7 @@ fn crash_ping_annotations() {
         .unwrap();
     println!("cargo:rerun-if-changed={}", crash_annotations.display());
 
-    let crash_ping_file = Path::new(&env::var("OUT_DIR").unwrap()).join("ping_annotations.rs");
+    let crash_ping_file = Path::new(&env::var("OUT_DIR").unwrap()).join("crash_annotations.rs");
 
     let yaml = std::fs::read_to_string(crash_annotations).unwrap();
     let Yaml::Hash(entries) = YamlLoader::load_from_str(&yaml)
@@ -58,23 +58,33 @@ fn crash_ping_annotations() {
         panic!("unexpected crash annotations root type");
     };
 
-    let ping_annotations = entries.into_iter().filter_map(|(k, v)| {
+    let mut ping_annotations = phf_codegen::Set::new();
-        v["ping"]
+    let mut report_annotations = phf_codegen::Set::new();
-            .as_bool()
-            .unwrap_or(false)
-            .then(|| k.into_string().unwrap())
-    });
 
-    let mut phf_set = phf_codegen::Set::new();
+    for (k, v) in entries {
-    for annotation in ping_annotations {
+        let scope = v["scope"].as_str().unwrap_or("client");
-        phf_set.entry(annotation);
+        match scope {
+            "ping" => {
+                ping_annotations.entry(k.into_string().unwrap());
+            }
+            "report" => {
+                report_annotations.entry(k.into_string().unwrap());
+            }
+            _ => (),
+        }
     }
 
     let mut file = BufWriter::new(File::create(&crash_ping_file).unwrap());
     writeln!(
         &mut file,
         "static PING_ANNOTATIONS: phf::Set<&'static str> = {};",
-        phf_set.build()
+        ping_annotations.build(),
+    )
+    .unwrap();
+    writeln!(
+        &mut file,
+        "static REPORT_ANNOTATIONS: phf::Set<&'static str> = {};",
+        report_annotations.build(),
     )
     .unwrap();
 }

toolkit/crashreporter/client/app/src/logic.rs

@@ -10,7 +10,7 @@ use crate::std::{
     process::Command,
     sync::{
         atomic::{AtomicBool, Ordering::Relaxed},
-        Arc, Mutex,
+        Arc, Mutex, Weak,
     },
 };
 use crate::{
@@ -25,6 +25,8 @@ use crate::{
 use anyhow::Context;
 use uuid::Uuid;
 
+pub mod annotations;
+
 /// The main crash reporting logic.
 pub struct ReportCrash {
     pub settings: RefCell<Settings>,
@@ -32,16 +34,13 @@ pub struct ReportCrash {
     extra: serde_json::Value,
     settings_file: PathBuf,
     attempted_to_send: AtomicBool,
-    ui: Option<AsyncTask<ReportCrashUIState>>,
+    ui: Option<Arc<AsyncTask<ReportCrashUIState>>>,
     memtest: RefCell<Option<Memtest>>,
 }
 
 fn modify_extra_for_report(extra: &mut serde_json::Value) {
     if let Some(map) = extra.as_object_mut() {
-        // Remove these entries, they don't need to be sent.
+        map.retain(|k, _| annotations::send_in_report(k));
-        map.remove("ProfileDirectory");
-        map.remove("ServerURL");
-        map.remove("StackTraces");
     }
 
     extra["SubmittedFrom"] = "Client".into();
@@ -403,7 +402,17 @@ impl ReportCrash {
         );
 
         // Set the UI remote queue.
-        self.ui = Some(crash_ui.async_task());
+        let crash_ui_async_task = Arc::new(crash_ui.async_task());
+        struct PanicHandler(Weak<AsyncTask<ReportCrashUIState>>);
+        impl Drop for PanicHandler {
+            fn drop(&mut self) {
+                if let Some(ui) = self.0.upgrade() {
+                    ui.push(|_| panic!("logic thread panicked"));
+                }
+            }
+        }
+        let logic_panic_handler = PanicHandler(Arc::downgrade(&crash_ui_async_task));
+        self.ui = Some(crash_ui_async_task);
 
         // Spawn a separate thread to handle all interactions with `self`. This prevents blocking
         // the UI for any reason.
@@ -418,11 +427,16 @@ impl ReportCrash {
             // when the UI finishes so the scope can exit).
             let _logic_send = logic_send;
             s.spawn(move || {
+                let _logic_panic_handler = logic_panic_handler;
                 barrier.wait();
                 while let Ok(f) = logic_recv.recv() {
                     f(self);
                 }
-                // Clear the UI remote queue, using it after this point is an error.
+                // Save settings after UI is closed
+                self.save_settings();
+
+                // Clear the UI remote queue, using it after this point is an error. This also
+                // prevents the panic handler from engaging.
                 //
                 // NOTE we do this here because the compiler can't reason about `self` being safely
                 // accessible after `thread::scope` returns. This is effectively the same result

toolkit/crashreporter/client/app/src/logic/annotations.rs

@@ -0,0 +1,20 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+//! Crash annotation information.
+
+// Generated by `build.rs`.
+// static PING_ANNOTATIONS: phf::Set<&'static str>;
+// static REPORT_ANNOTATIONS: phf::Set<&'static str>;
+include!(concat!(env!("OUT_DIR"), "/crash_annotations.rs"));
+
+/// Return whether the given annotation can be sent in a crash ping.
+pub fn send_in_ping(annotation: &str) -> bool {
+    PING_ANNOTATIONS.contains(annotation)
+}
+
+/// Return whether the given annotation can be sent in a crash report.
+pub fn send_in_report(annotation: &str) -> bool {
+    REPORT_ANNOTATIONS.contains(annotation) || PING_ANNOTATIONS.contains(annotation)
+}

toolkit/crashreporter/client/app/src/net/ping/legacy_telemetry.rs

@@ -5,6 +5,7 @@
 //! Support for legacy telemetry ping creation. The ping supports serialization which should be
 //! used when submitting.
 
+use crate::logic::annotations;
 use crate::std;
 use anyhow::Context;
 use serde::Serialize;
@@ -15,10 +16,6 @@ use uuid::Uuid;
 const TELEMETRY_VERSION: u64 = 4;
 const PAYLOAD_VERSION: u64 = 1;
 
-// Generated by `build.rs`.
-// static PING_ANNOTATIONS: phf::Set<&'static str>;
-include!(concat!(env!("OUT_DIR"), "/ping_annotations.rs"));
-
 // We need a custom time serializer to encode at most 3 decimal digits in the fractions of a second
 // (millisecond precision).
 const TIME_CONFIG: iso8601::EncodedConfig = iso8601::Config::DEFAULT
@@ -106,8 +103,7 @@ impl<'a> Ping<'a> {
             .map(|map| {
                 map.iter()
                     .filter_map(|(k, v)| {
-                        PING_ANNOTATIONS
+                        annotations::send_in_ping(k)
-                            .contains(k)
                             .then(|| k.as_str())
                             .zip(v.as_str())
                     })

toolkit/crashreporter/client/app/src/test.rs

@@ -65,24 +65,10 @@ impl<T: std::fmt::Display> std::fmt::Display for FluentArg<T> {
 fn gui_interact<G, I, R>(gui: G, interact: I) -> R
 where
     G: FnOnce() -> R,
-    I: FnOnce(Interact) + Send + 'static,
+    I: FnOnce(&Interact) + Send + 'static,
 {
-    let i = Interact::hook();
+    let _spawned = Interact::spawn(interact);
-    let handle = {
+    gui()
-        let i = i.clone();
-        ::std::thread::spawn(move || {
-            i.wait_for_ready();
-            interact(i);
-        })
-    };
-    let ret = gui();
-    // In case the gui failed before launching.
-    i.cancel();
-    // If the gui failed, it's possible the interact thread hit a panic. However we can't check
-    // whether `R` is in a failure state, so we ignore the result of joining the thread. If there
-    // is an error, a backtrace will show the thread's panic message.
-    let _ = handle.join();
-    ret
 }
 
 const MOCK_MINIDUMP_EXTRA: &str = r#"{
@@ -244,7 +230,7 @@ impl GuiTest {
     /// Run the test as configured, using the given function to interact with the GUI.
     ///
     /// Returns the final result of the application logic.
-    pub fn try_run<F: FnOnce(Interact) + Send + 'static>(
+    pub fn try_run<F: FnOnce(&Interact) + Send + 'static>(
         &mut self,
         interact: F,
     ) -> anyhow::Result<bool> {
@@ -280,7 +266,7 @@ impl GuiTest {
     ///
     /// Panics if the application logic returns an error (which would normally be displayed to the
     /// user).
-    pub fn run<F: FnOnce(Interact) + Send + 'static>(&mut self, interact: F) {
+    pub fn run<F: FnOnce(&Interact) + Send + 'static>(&mut self, interact: F) {
         if let Err(e) = self.try_run(interact) {
             panic!(
                 "gui failure:{}",
@@ -935,12 +921,7 @@ fn details_window() {
              BuildID: 1234\n\
              ProductName: Bar\n\
              ReleaseChannel: release\n\
-             SomeNestedJson: {\"foo\":\"bar\"}\n\
              SubmittedFrom: Client\n\
-             TelemetryClientId: telemetry_client\n\
-             TelemetryProfileGroupId: telemetry_profile_group\n\
-             TelemetryServerURL: https://telemetry.example.com\n\
-             TelemetrySessionId: telemetry_session\n\
              Throttleable: 1\n\
              URL: https://url.example.com\n\
              Vendor: FooCorp\n\

toolkit/crashreporter/client/app/src/ui/test.rs

@@ -95,18 +95,59 @@ impl UI {
     }
 }
 
-/// Test interaction hook.
+/// Test interaction interface.
 #[derive(Clone)]
 pub struct Interact {
     state: Arc<State>,
 }
 
+/// Spawned test interaction.
+pub struct SpawnedInteract {
+    interact: Interact,
+    handle: ::std::mem::ManuallyDrop<::std::thread::JoinHandle<()>>,
+}
+
+impl Drop for SpawnedInteract {
+    fn drop(&mut self) {
+        self.interact.cancel();
+        if unsafe { ::std::mem::ManuallyDrop::take(&mut self.handle) }
+            .join()
+            .is_err()
+        {
+            if !std::thread::panicking() {
+                // Make sure this thread panics so we see the panic from the interact thread.
+                panic!("interact thread panicked");
+            }
+        }
+    }
+}
+
 impl Interact {
+    /// Spawn a thread which runs interactions according with the test UI.
+    ///
+    /// The returned `SpawnedInteract` should be dropped after the test UI is run.
+    ///
+    /// # Panic Safety
+    /// If a panic occurs in the interact thread, it will not be re-thrown (we assume that the
+    /// panic handler will still show the thread's panic message when the test completes).
+    pub fn spawn<F: FnOnce(&Self) + Send + 'static>(f: F) -> SpawnedInteract {
+        let interact = Self::hook();
+        let handle = ::std::thread::spawn(cc! { (interact) move || {
+            interact.wait_for_ready();
+            f(&interact);
+
+        }});
+        SpawnedInteract {
+            interact,
+            handle: ::std::mem::ManuallyDrop::new(handle),
+        }
+    }
+
     /// Create an interaction hook for the test UI.
     ///
     /// This should be done before running the UI, and must be done on the same thread that
     /// later runs it.
-    pub fn hook() -> Self {
+    fn hook() -> Interact {
         let v = Interact {
             state: Default::default(),
         };
@@ -118,7 +159,7 @@ impl Interact {
     }
 
     /// Wait for the render thread to be ready for interaction.
-    pub fn wait_for_ready(&self) {
+    fn wait_for_ready(&self) {
         let mut guard = self.state.interface.lock().unwrap();
         while guard.is_none() && !self.state.cancel.load(Relaxed) {
             guard = self.state.waiting_for_interface.wait(guard).unwrap();
@@ -126,7 +167,7 @@ impl Interact {
     }
 
     /// Cancel an Interact (which causes `wait_for_ready` to always return).
-    pub fn cancel(&self) {
+    fn cancel(&self) {
         let _guard = self.state.interface.lock().unwrap();
         self.state.cancel.store(true, Relaxed);
         self.state.waiting_for_interface.notify_all();

toolkit/crashreporter/generate_crash_reporter_sources.py

@@ -34,18 +34,26 @@ def validate_annotations(annotations):
         if "type" not in data:
             print("Annotation " + name + " does not have a type\n")
             sys.exit(1)
-        else:
+
-            annotation_type = data.get("type")
+        annotation_type = data.get("type")
-            valid_types = ["string", "boolean", "u32", "u64", "usize"]
+        valid_types = ["string", "boolean", "u32", "u64", "usize", "object"]
-            if not any(annotation_type == t for t in valid_types):
+        if annotation_type not in valid_types:
-                print(
+            print(
-                    "Annotation "
+                "Annotation " + name + " has an unknown type: " + annotation_type + "\n"
-                    + name
+            )
-                    + " has an unknown type: "
+            sys.exit(1)
-                    + annotation_type
+
-                    + "\n"
+        annotation_scope = data.get("scope", "client")
-                )
+        valid_scopes = ["client", "report", "ping"]
-                sys.exit(1)
+        if annotation_scope not in valid_scopes:
+            print(
+                "Annotation "
+                + name
+                + " has an unknown scope: "
+                + annotation_scope
+                + "\n"
+            )
+            sys.exit(1)
 
 
 def read_annotations(annotations_filename):
@@ -80,9 +88,20 @@ def read_template(template_filename):
 
 def extract_crash_ping_allowedlist(annotations):
     """Extract an array holding the names of the annotations allowed for
-    inclusion in the crash ping."""
+    inclusion in a crash ping."""
 
-    return [name for (name, data) in annotations if data.get("ping", False)]
+    return [
+        name for (name, data) in annotations if data.get("scope", "client") == "ping"
+    ]
+
+
+def extract_crash_report_allowedlist(annotations):
+    """Extract an array holding the names of the annotations allowed for
+    inclusion in a crash report (excluding those allowed for pings)."""
+
+    return [
+        name for (name, data) in annotations if data.get("scope", "client") == "report"
+    ]
 
 
 def extract_skiplist(annotations):
@@ -109,6 +128,8 @@ def type_to_enum(annotation_type):
         return "U64"
     elif annotation_type == "usize":
         return "USize"
+    elif annotation_type == "object":
+        return "Object"
 
 
 def extract_types(annotations):
@@ -174,7 +195,8 @@ def generate_header(template, annotations):
     """Generate a header by filling the template with the the list of
     annotations and return it as a string."""
 
-    allowedlist = extract_crash_ping_allowedlist(annotations)
+    pingallowedlist = extract_crash_ping_allowedlist(annotations)
+    reportallowedlist = extract_crash_report_allowedlist(annotations)
     skiplist = extract_skiplist(annotations)
     typelist = extract_types(annotations)
 
@@ -182,7 +204,10 @@ def generate_header(template, annotations):
         {
             "enum": generate_enum(annotations),
             "strings": generate_strings(annotations),
-            "allowedlist": generate_annotations_array_initializer(allowedlist),
+            "pingallowedlist": generate_annotations_array_initializer(pingallowedlist),
+            "reportallowedlist": generate_annotations_array_initializer(
+                reportallowedlist
+            ),
             "skiplist": generate_skiplist_initializer(skiplist),
             "types": generate_types_initializer(typelist),
         }
@@ -244,7 +269,7 @@ def emit_class(output, annotations_filename):
      * are kept in sync with the other C++ and JS users.
      */
     public class CrashReporterConstants {
-        public static final String[] ANNOTATION_ALLOWEDLIST = {
+        public static final String[] ANNOTATION_PING_ALLOWEDLIST = {
     ${allowedlist}
         };
     }"""

toolkit/crashreporter/nsExceptionHandler.cpp

@@ -422,8 +422,7 @@ static void CreateFileFromPath(const xpstring& path, nsIFile** file) {
       DependentPathString(path.c_str(), path.size()), file);
 }
 
-[[nodiscard]]
+[[nodiscard]] static std::optional<xpstring> CreatePathFromFile(nsIFile* file) {
-static std::optional<xpstring> CreatePathFromFile(nsIFile* file) {
   AutoPathString path;
 #ifdef XP_WIN
   nsresult rv = file->GetPath(path);
@@ -1416,6 +1415,9 @@ static void WriteAnnotationsForMainProcessCrash(PlatformWriter& pw,
           writer.Write(
               key, static_cast<uint64_t>(*reinterpret_cast<size_t*>(address)));
           break;
+        case AnnotationType::Object:
+          // Object annotations are only produced later by minidump-analyzer.
+          break;
       }
     }
   }
@@ -3216,6 +3218,9 @@ static void AddSharedAnnotations(AnnotationTable& aAnnotations) {
 #endif
           }
           break;
+        case AnnotationType::Object:
+          // Object annotations are only produced later by minidump-analyzer.
+          break;
       }
 
       if (!value.IsEmpty() && aAnnotations[key].IsEmpty() &&
@@ -3293,6 +3298,9 @@ static void AddChildProcessAnnotations(
 #endif
         }
         break;
+      case AnnotationType::Object:
+        // Object annotations are only produced later by minidump-analyzer.
+        break;
     }
 
     if (!value.IsEmpty() && ShouldIncludeAnnotation(id, value.get())) {

toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js

@@ -87,8 +87,9 @@ function check_submit_pending(tab, crashes) {
 
       return { id: CrashID, url: CrashURL, result };
     }).then(({ id, url, result }) => {
-      // Likewise, this is discarded before it gets to the server
+      // These should have been discarded before being sent.
       delete SubmittedCrash.extra.ServerURL;
+      delete SubmittedCrash.extra.DefinitelyNotAnAnnotationKey;
 
       CrashID = id;
       CrashURL = url;
@@ -183,7 +184,8 @@ function test() {
       ServerURL:
         "http://example.com/browser/toolkit/crashreporter/test/browser/crashreport.sjs",
       ProductName: "Test App",
-      Foo: "ABC=XYZ", // test that we don't truncat eat = (bug 512853)
+      TestKey: "ABC=XYZ", // test that we don't truncate at = (bug 512853)
+      DefinitelyNotAnAnnotationKey: "foobar", // test that we remove non-existent annotations
     })
   );
   crashes.sort((a, b) => b.date - a.date);

toolkit/xre/nsAppRunner.cpp

@@ -1836,6 +1836,13 @@ nsXULAppInfo::RemoveCrashReportAnnotation(const nsACString& key) {
   return NS_OK;
 }
 
+NS_IMETHODIMP
+nsXULAppInfo::IsAnnotationValid(const nsACString& aValue, bool* aIsValid) {
+  auto annotation = CrashReporter::AnnotationFromString(aValue);
+  *aIsValid = annotation.isSome();
+  return NS_OK;
+}
+
 NS_IMETHODIMP
 nsXULAppInfo::IsAnnotationAllowedForPing(const nsACString& aValue,
                                          bool* aIsAllowed) {
@@ -1846,6 +1853,16 @@ nsXULAppInfo::IsAnnotationAllowedForPing(const nsACString& aValue,
   return NS_OK;
 }
 
+NS_IMETHODIMP
+nsXULAppInfo::IsAnnotationAllowedForReport(const nsACString& aValue,
+                                           bool* aIsAllowed) {
+  auto annotation = CrashReporter::AnnotationFromString(aValue);
+  NS_ENSURE_TRUE(annotation.isSome(), NS_ERROR_INVALID_ARG);
+  *aIsAllowed = CrashReporter::IsAnnotationAllowedForReport(*annotation);
+
+  return NS_OK;
+}
+
 NS_IMETHODIMP
 nsXULAppInfo::AppendAppNotesToCrashReport(const nsACString& data) {
   return CrashReporter::AppendAppNotesToCrashReport(data);

xpcom/system/nsICrashReporter.idl

@@ -103,6 +103,16 @@ interface nsICrashReporter : nsISupports
    */
   void removeCrashReportAnnotation(in AUTF8String key);
 
+  /**
+   * Checks if an annotation is valid.
+   *
+   * @param key
+   *        Name of a crash annotation constant.
+   *
+   * @return True if the specified value is a valid annotation.
+   */
+  boolean isAnnotationValid(in ACString value);
+
   /**
    * Checks if an annotation is allowed for inclusion in the crash ping.
    *
@@ -115,6 +125,18 @@ interface nsICrashReporter : nsISupports
    */
   boolean isAnnotationAllowedForPing(in ACString value);
 
+  /**
+   * Checks if an annotation is allowed for inclusion in a crash report.
+   *
+   * @param key
+   *        Name of a known crash annotation constant.
+   *
+   * @return True if the specified value is a valid annotation and can be
+             included in a crash report, false otherwise.
+   * @throw NS_ERROR_INVALID_ARG if key contains an invalid value.
+   */
+  boolean isAnnotationAllowedForReport(in ACString value);
+
   /**
    * Append some data to the "Notes" field, to be submitted with a crash report.
    * Unlike annotateCrashReport, this method will append to existing data.