Bug 1464521: Process collected browser JS errors during idle. r=kmag
authorMichael Kelly <mkelly@mozilla.com>
Tue, 05 Jun 2018 08:58:45 -0700
changeset 421822 e6d89c09bc10800ddc6a7f544da7809ad1105a56
parent 421821 dc080b49ef9c91ce0282d00712f7ddd2349b54d8
child 421823 9528ab66c3bfeb0e4c0d9b4c6dae645c4a755b40
push id104125
push useraciure@mozilla.com
push dateThu, 07 Jun 2018 21:57:03 +0000
treeherdermozilla-inbound@38c222c1bf73 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1464521
milestone62.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 1464521: Process collected browser JS errors during idle. r=kmag 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
     );