Bug 1464521: Process collected browser JS errors during idle. draft
authorMichael Kelly <mkelly@mozilla.com>
Tue, 05 Jun 2018 08:58:45 -0700
changeset 805302 0b2a2680e64b5565924060451a00811d6f51b3b7
parent 802711 9900cebb1f9000bd05731ba67736b7c51f7eb812
push id112625
push userbmo:mkelly@mozilla.com
push dateThu, 07 Jun 2018 16:34:06 +0000
bugs1464521
milestone62.0a1
Bug 1464521: Process collected browser JS errors during idle. testFetchArguments and the add-on ID mangling tests already cover error collection end-to-end, so no new tests are needed to cover testing the idle callback. MozReview-Commit-ID: DJrqT5jCq44
browser/modules/BrowserErrorReporter.jsm
browser/modules/test/browser/browser_BrowserErrorReporter.js
--- a/browser/modules/BrowserErrorReporter.jsm
+++ b/browser/modules/BrowserErrorReporter.jsm
@@ -193,23 +193,23 @@ class BrowserErrorReporter {
     // WebExtensions get grouped separately from other errors
     if (filename.startsWith("moz-extension://")) {
         return "MOZEXTENSION";
     }
 
     return "FILTERED";
   }
 
-  async observe(message) {
-    try {
-      message.QueryInterface(Ci.nsIScriptError);
-    } catch (err) {
-      return; // Not an error
+  observe(message) {
+    if (message instanceof Ci.nsIScriptError) {
+      ChromeUtils.idleDispatch(() => this.handleMessage(message));
     }
+  }
 
+  async handleMessage(message) {
     const isWarning = message.flags & message.warningFlag;
     const isFromChrome = REPORTED_CATEGORIES.has(message.category);
     if ((this.chromeOnly && !isFromChrome) || isWarning) {
       return;
     }
 
     // Record that we collected an error prior to applying the sample rate
     Services.telemetry.scalarAdd(TELEMETRY_ERROR_COLLECTED, 1);
--- a/browser/modules/test/browser/browser_BrowserErrorReporter.js
+++ b/browser/modules/test/browser/browser_BrowserErrorReporter.js
@@ -161,141 +161,141 @@ add_task(async function testEnabledPrefW
 add_task(async function testNonErrorLogs() {
   const fetchSpy = sinon.spy();
   const reporter = new BrowserErrorReporter({fetch: fetchSpy});
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
 
-  reporter.observe({message: "Not a scripterror instance."});
+  await reporter.handleMessage({message: "Not a scripterror instance."});
   ok(
     !fetchPassedError(fetchSpy, "Not a scripterror instance."),
     "Reporter does not collect normal log messages or warnings.",
   );
 
-  await reporter.observe(createScriptError({
+  await reporter.handleMessage(createScriptError({
     message: "Warning message",
     flags: Ci.nsIScriptError.warningFlag,
   }));
   ok(
     !fetchPassedError(fetchSpy, "Warning message"),
     "Reporter does not collect normal log messages or warnings.",
   );
 
-  await reporter.observe(createScriptError({
+  await reporter.handleMessage(createScriptError({
     message: "Non-chrome category",
     category: "totally from a website",
   }));
   ok(
     !fetchPassedError(fetchSpy, "Non-chrome category"),
     "Reporter does not collect normal log messages or warnings.",
   );
 
-  await reporter.observe(createScriptError({message: "Is error"}));
+  await reporter.handleMessage(createScriptError({message: "Is error"}));
   ok(
     fetchPassedError(fetchSpy, "Is error"),
     "Reporter collects error messages.",
   );
 });
 
 add_task(async function testSampling() {
   const fetchSpy = sinon.spy();
   const reporter = new BrowserErrorReporter({fetch: fetchSpy});
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
 
-  await reporter.observe(createScriptError({message: "Should log"}));
+  await reporter.handleMessage(createScriptError({message: "Should log"}));
   ok(
     fetchPassedError(fetchSpy, "Should log"),
     "A 1.0 sample rate will cause the reporter to always collect errors.",
   );
 
-  await reporter.observe(createScriptError({message: "undefined", sourceName: undefined}));
+  await reporter.handleMessage(createScriptError({message: "undefined", sourceName: undefined}));
   ok(
     fetchPassedError(fetchSpy, "undefined"),
     "A missing sourceName doesn't break reporting.",
   );
 
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_SAMPLE_RATE, "0.0"],
   ]});
-  await reporter.observe(createScriptError({message: "Shouldn't log"}));
+  await reporter.handleMessage(createScriptError({message: "Shouldn't log"}));
   ok(
     !fetchPassedError(fetchSpy, "Shouldn't log"),
     "A 0.0 sample rate will cause the reporter to never collect errors.",
   );
 
-  await reporter.observe(createScriptError({
+  await reporter.handleMessage(createScriptError({
     message: "chromedevtools",
     sourceName: "chrome://devtools/Foo.jsm",
   }));
   ok(
     fetchPassedError(fetchSpy, "chromedevtools"),
     "chrome://devtools/ paths are sampled at 100% even if the default rate is 0.0.",
   );
 
-  await reporter.observe(createScriptError({
+  await reporter.handleMessage(createScriptError({
     message: "resourcedevtools",
     sourceName: "resource://devtools/Foo.jsm",
   }));
   ok(
     fetchPassedError(fetchSpy, "resourcedevtools"),
     "resource://devtools/ paths are sampled at 100% even if the default rate is 0.0.",
   );
 
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_SAMPLE_RATE, ")fasdf"],
   ]});
-  await reporter.observe(createScriptError({message: "Also shouldn't log"}));
+  await reporter.handleMessage(createScriptError({message: "Also shouldn't log"}));
   ok(
     !fetchPassedError(fetchSpy, "Also shouldn't log"),
     "An invalid sample rate will cause the reporter to never collect errors.",
   );
 });
 
 add_task(async function testNameMessage() {
   const fetchSpy = sinon.spy();
   const reporter = new BrowserErrorReporter({fetch: fetchSpy});
   await SpecialPowers.pushPrefEnv({set: [
     [PREF_ENABLED, true],
     [PREF_SAMPLE_RATE, "1.0"],
   ]});
 
-  await reporter.observe(createScriptError({message: "No name"}));
+  await reporter.handleMessage(createScriptError({message: "No name"}));
   let call = fetchCallForMessage(fetchSpy, "No name");
   let body = JSON.parse(call.args[1].body);
   is(
     body.exception.values[0].type,
     "Error",
     "Reporter uses a generic type when no name is in the message.",
   );
   is(
     body.exception.values[0].value,
     "No name",
     "Reporter uses error message as the exception value.",
   );
 
-  await reporter.observe(createScriptError({message: "FooError: Has name"}));
+  await reporter.handleMessage(createScriptError({message: "FooError: Has name"}));
   call = fetchCallForMessage(fetchSpy, "Has name");
   body = JSON.parse(call.args[1].body);
   is(
     body.exception.values[0].type,
     "FooError",
     "Reporter uses the error type from the message.",
   );
   is(
     body.exception.values[0].value,
     "Has name",
     "Reporter uses error message as the value parameter.",
   );
 
-  await reporter.observe(createScriptError({message: "FooError: Has :extra: colons"}));
+  await reporter.handleMessage(createScriptError({message: "FooError: Has :extra: colons"}));
   call = fetchCallForMessage(fetchSpy, "Has :extra: colons");
   body = JSON.parse(call.args[1].body);
   is(
     body.exception.values[0].type,
     "FooError",
     "Reporter uses the error type from the message.",
   );
   is(
@@ -475,17 +475,17 @@ add_task(async function testExtensionTag
     `Wait for error from ${id} to be logged`,
   );
   let body = JSON.parse(call.args[1].body);
   ok(body.tags.isExtensionError, "Errors from extensions have an isExtensionError=true tag.");
 
   await extension.unload();
   reporter.uninit();
 
-  await reporter.observe(createScriptError({message: "testExtensionTag not from extension"}));
+  await reporter.handleMessage(createScriptError({message: "testExtensionTag not from extension"}));
   call = fetchCallForMessage(fetchSpy, "testExtensionTag not from extension");
   body = JSON.parse(call.args[1].body);
   is(body.tags.isExtensionError, false, "Normal errors have an isExtensionError=false tag.");
 });
 
 add_task(async function testScalars() {
   const fetchStub = sinon.stub();
   const reporter = new BrowserErrorReporter({fetch: fetchStub});
@@ -512,25 +512,25 @@ add_task(async function testScalars() {
     Object.create(
       createScriptError({message: "Whatever"}),
       {stack: {value: new Error().stack}},
     ),
   ];
 
   // Use observe to avoid errors from other code messing up our counts.
   for (const message of messages) {
-    await reporter.observe(message);
+    await reporter.handleMessage(message);
   }
 
   await SpecialPowers.pushPrefEnv({set: [[PREF_SAMPLE_RATE, "0.0"]]});
-  await reporter.observe(createScriptError({message: "Additionally no name"}));
+  await reporter.handleMessage(createScriptError({message: "Additionally no name"}));
 
   await SpecialPowers.pushPrefEnv({set: [[PREF_SAMPLE_RATE, "1.0"]]});
   fetchStub.rejects(new Error("Could not report"));
-  await reporter.observe(createScriptError({message: "Maybe name?"}));
+  await reporter.handleMessage(createScriptError({message: "Maybe name?"}));
 
   const optin = Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN;
   const scalars = Services.telemetry.snapshotScalars(optin, false).parent;
   is(
     scalars[TELEMETRY_ERROR_COLLECTED],
     9,
     `${TELEMETRY_ERROR_COLLECTED} is incremented when an error is collected.`,
   );
@@ -591,17 +591,17 @@ add_task(async function testCollectedFil
     ["chrome://browser/Foo.jsm", true],
     ["chrome://devtools/Foo.jsm", true],
   ];
 
   for (const [filename, shouldMatch] of testCases) {
     Services.telemetry.clearScalars();
 
     // Use observe to avoid errors from other code messing up our counts.
-    await reporter.observe(createScriptError({
+    await reporter.handleMessage(createScriptError({
       message: "Fine",
       sourceName: filename,
     }));
 
     const keyedScalars = (
       Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false).parent
     );