Backed out changeset 523b4c1d489c (bug 1451702) for failures on browser_BrowserErrorReporter.js CLOSED TREE
authorBogdan Tara <btara@mozilla.com>
Tue, 19 Jun 2018 19:48:46 +0300
changeset 479752 10a60c58343097c99a8e5a272f39a6ea122542b3
parent 479751 f2909770e169489b93445827f00f464feb696228
child 479753 5d130a2d962c247af9316edb7506b19165e506dc
push id1757
push userffxbld-merge
push dateFri, 24 Aug 2018 17:02:43 +0000
treeherdermozilla-release@736023aebdb1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1451702
milestone62.0a1
backs out523b4c1d489c29d79e5782b62f01d21b9fc07eb2
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
Backed out changeset 523b4c1d489c (bug 1451702) for failures on browser_BrowserErrorReporter.js CLOSED TREE
browser/modules/BrowserErrorReporter.jsm
browser/modules/test/browser/browser_BrowserErrorReporter.js
--- a/browser/modules/BrowserErrorReporter.jsm
+++ b/browser/modules/BrowserErrorReporter.jsm
@@ -148,34 +148,16 @@ class BrowserErrorReporter {
     );
     XPCOMUtils.defineLazyPreferenceGetter(
       this,
       "sampleRatePref",
       PREF_SAMPLE_RATE,
       "0.0",
       this.handleSampleRatePrefChanged.bind(this),
     );
-
-    // Prefix mappings for the mangleFilePaths transform.
-    this.manglePrefixes = options.manglePrefixes || {
-      greDir: Services.dirsvc.get("GreD", Ci.nsIFile),
-      profileDir: Services.dirsvc.get("ProfD", Ci.nsIFile),
-    };
-    // File paths are encoded by nsIURI, so let's do the same for the prefixes
-    // we're comparing them to.
-    for (const [name, prefixFile] of Object.entries(this.manglePrefixes)) {
-      let filePath = Services.io.newFileURI(prefixFile).filePath;
-
-      // filePath might not have a trailing slash in some cases
-      if (!filePath.endsWith("/")) {
-        filePath += "/";
-      }
-
-      this.manglePrefixes[name] = filePath;
-    }
   }
 
   /**
    * Lazily-created logger
    */
   get logger() {
     const logger = Log.repository.getLogger("BrowserErrorReporter");
     logger.addAppender(new Log.ConsoleAppender(new Log.BasicFormatter()));
@@ -289,17 +271,16 @@ class BrowserErrorReporter {
       },
     };
 
     const transforms = [
       addErrorMessage,
       addStacktrace,
       addModule,
       mangleExtensionUrls,
-      this.mangleFilePaths.bind(this),
       tagExtensionErrors,
     ];
     for (const transform of transforms) {
       await transform(message, exceptionValue, requestBody);
     }
 
     const url = new URL(Services.prefs.getCharPref(PREF_SUBMIT_URL));
     url.searchParams.set("sentry_client", `${SDK_NAME}/${SDK_VERSION}`);
@@ -313,58 +294,22 @@ class BrowserErrorReporter {
           "Content-Type": "application/json",
           "Accept": "application/json",
         },
         // Sentry throws an auth error without a referrer specified.
         referrer: "https://fake.mozilla.org",
         body: JSON.stringify(requestBody)
       });
       Services.telemetry.scalarAdd(TELEMETRY_ERROR_REPORTED, 1);
-      this.logger.debug(`Sent error "${message.errorMessage}" successfully.`);
+      this.logger.debug("Sent error successfully.");
     } catch (error) {
       Services.telemetry.scalarAdd(TELEMETRY_ERROR_REPORTED_FAIL, 1);
-      this.logger.warn(`Failed to send error "${message.errorMessage}": ${error}`);
-    }
-  }
-
-  /**
-   * Alters file: and jar: paths to remove leading file paths that may contain
-   * user-identifying or platform-specific paths.
-   *
-   * prefixes is a mapping of replacementName -> filePath, where filePath is a
-   * path on the filesystem that should be replaced, and replacementName is the
-   * text that will replace it.
-   */
-  mangleFilePaths(message, exceptionValue) {
-    exceptionValue.module = this._transformFilePath(exceptionValue.module);
-    for (const frame of exceptionValue.stacktrace.frames) {
-      frame.module = this._transformFilePath(frame.module);
+      this.logger.warn(`Failed to send error: ${error}`);
     }
   }
-
-  _transformFilePath(path) {
-    try {
-      const uri = Services.io.newURI(path);
-      if (uri.schemeIs("jar")) {
-        return uri.filePath;
-      }
-      if (uri.schemeIs("file")) {
-        for (const [name, prefix] of Object.entries(this.manglePrefixes)) {
-          console.log(`Evaluating prefix: ${name}: ${prefix} against ${uri.filePath}`);
-          if (uri.filePath.startsWith(prefix)) {
-            return uri.filePath.replace(prefix, `[${name}]/`);
-          }
-        }
-
-        return "[UNKNOWN_LOCAL_FILEPATH]";
-      }
-    } catch (err) {}
-
-    return path;
-  }
 }
 
 function defaultFetch(...args) {
   // Do not make network requests while running in automation
   if (Cu.isInAutomation) {
     return null;
   }
 
@@ -445,19 +390,20 @@ function mangleExtensionUrls(message, ex
 
   // Replaces any instances of moz-extension:// URLs with internal UUIDs to use
   // the add-on ID instead.
   function mangleExtURL(string, anchored = true) {
     if (!string) {
       return string;
     }
 
-    const re = new RegExp(`${anchored ? "^" : ""}moz-extension://([^/]+)/`, "g");
+    let re = new RegExp(`${anchored ? "^" : ""}moz-extension://([^/]+)/`, "g");
+
     return string.replace(re, (m0, m1) => {
-      const id = extensions.has(m1) ? extensions.get(m1).id : m1;
+      let id = extensions.has(m1) ? extensions.get(m1).id : m1;
       return `moz-extension://${id}/`;
     });
   }
 
   exceptionValue.value = mangleExtURL(exceptionValue.value, false);
   exceptionValue.module = mangleExtURL(exceptionValue.module);
   for (const frame of exceptionValue.stacktrace.frames) {
     frame.module = mangleExtURL(frame.module);
--- a/browser/modules/test/browser/browser_BrowserErrorReporter.js
+++ b/browser/modules/test/browser/browser_BrowserErrorReporter.js
@@ -1,16 +1,15 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 "use strict";
 
+ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm", this);
 ChromeUtils.import("resource:///modules/BrowserErrorReporter.jsm", this);
 ChromeUtils.import("resource://gre/modules/AppConstants.jsm", this);
-ChromeUtils.import("resource://gre/modules/FileUtils.jsm", this);
-ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm", this);
 
 /* global sinon */
 Services.scriptloader.loadSubScript("resource://testing-common/sinon-2.3.2.js");
 registerCleanupFunction(function() {
   delete window.sinon;
 });
 
 const PREF_ENABLED = "browser.chrome.errorReporter.enabled";
@@ -21,55 +20,29 @@ const PREF_SUBMIT_URL = "browser.chrome.
 const TELEMETRY_ERROR_COLLECTED = "browser.errors.collected_count";
 const TELEMETRY_ERROR_COLLECTED_FILENAME = "browser.errors.collected_count_by_filename";
 const TELEMETRY_ERROR_COLLECTED_STACK = "browser.errors.collected_with_stack_count";
 const TELEMETRY_ERROR_REPORTED = "browser.errors.reported_success_count";
 const TELEMETRY_ERROR_REPORTED_FAIL = "browser.errors.reported_failure_count";
 const TELEMETRY_ERROR_SAMPLE_RATE = "browser.errors.sample_rate";
 
 function createScriptError(options = {}) {
-  let scriptError = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
+  const scriptError = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
   scriptError.init(
     options.message || "",
     "sourceName" in options ? options.sourceName : null,
     options.sourceLine || null,
     options.lineNumber || null,
     options.columnNumber || null,
     options.flags || Ci.nsIScriptError.errorFlag,
     options.category || "chrome javascript",
   );
-
-  // You can't really set the stack of a scriptError in JS, so we shadow it instead.
-  if (options.stack) {
-    scriptError = Object.create(scriptError, {
-      stack: {
-        value: createStack(options.stack),
-      },
-    });
-  }
-
   return scriptError;
 }
 
-function createStack(frames) {
-  for (let k = 0; k < frames.length - 1; k++) {
-    frames[k].parent = frames[k + 1];
-  }
-  return frames[0];
-}
-
-function frame(options = {}) {
-  return Object.assign({
-    functionDisplayName: "fooFunction",
-    source: "resource://modules/BrowserErrorReporter.jsm",
-    line: 5,
-    column: 10,
-  }, options);
-}
-
 function noop() {
   // Do nothing
 }
 
 // Clears the console of any previous messages. Should be called at the end of
 // each test that logs to the console.
 function resetConsole() {
   Services.console.logStringMessage("");
@@ -576,20 +549,25 @@ add_task(async function testScalars() {
     createScriptError({message: "More no name", sourceName: "resource://gre/modules/Bar.jsm"}),
     createScriptError({message: "Yeah sures", sourceName: "unsafe://gre/modules/Bar.jsm"}),
     createScriptError({message: "Addon", sourceName: "moz-extension://foo/Bar.jsm"}),
     createScriptError({
       message: "long",
       sourceName: "resource://gre/modules/long/long/long/long/long/long/long/long/long/long/",
     }),
     {message: "Not a scripterror instance."},
-    createScriptError({message: "Whatever", stack: [frame()]}),
+
+    // No easy way to create an nsIScriptError with a stack, so let's pretend.
+    Object.create(
+      createScriptError({message: "Whatever"}),
+      {stack: {value: new Error().stack}},
+    ),
   ];
 
-  // Use handleMessage to avoid errors from other code messing up our counts.
+  // Use observe to avoid errors from other code messing up our counts.
   for (const message of messages) {
     await reporter.handleMessage(message);
   }
 
   await SpecialPowers.pushPrefEnv({set: [[PREF_SAMPLE_RATE, "0.0"]]});
   await reporter.handleMessage(createScriptError({message: "Additionally no name"}));
 
   await SpecialPowers.pushPrefEnv({set: [[PREF_SAMPLE_RATE, "1.0"]]});
@@ -662,17 +640,17 @@ add_task(async function testCollectedFil
     ["chrome://global/Foo.jsm", true],
     ["chrome://browser/Foo.jsm", true],
     ["chrome://devtools/Foo.jsm", true],
   ];
 
   for (const [filename, shouldMatch] of testCases) {
     Services.telemetry.clearScalars();
 
-    // Use handleMessage to avoid errors from other code messing up our counts.
+    // Use observe to avoid errors from other code messing up our counts.
     await reporter.handleMessage(createScriptError({
       message: "Fine",
       sourceName: filename,
     }));
 
     const keyedScalars = (
       Services.telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false).parent
     );
@@ -689,77 +667,8 @@ add_task(async function testCollectedFil
       shouldMatch
         ? `${TELEMETRY_ERROR_COLLECTED_FILENAME} logs a key for ${filename}.`
         : `${TELEMETRY_ERROR_COLLECTED_FILENAME} logs a FILTERED key for ${filename}.`,
     );
   }
 
   resetConsole();
 });
-
-add_task(async function testFilePathMangle() {
-  const fetchSpy = sinon.spy();
-  const reporter = new BrowserErrorReporter({fetch: fetchSpy});
-  await SpecialPowers.pushPrefEnv({set: [
-    [PREF_ENABLED, true],
-    [PREF_SAMPLE_RATE, "1.0"],
-  ]});
-
-  const greDir = Services.dirsvc.get("GreD", Ci.nsIFile).path;
-  const profileDir = Services.dirsvc.get("ProfD", Ci.nsIFile).path;
-
-  const message = createScriptError({
-    message: "Whatever",
-    sourceName: "file:///path/to/main.jsm",
-    stack: [
-      frame({source: "jar:file:///path/to/jar!/inside/jar.jsm"}),
-      frame({source: `file://${greDir}/defaults/prefs/channel-prefs.js`}),
-      frame({source: `file://${profileDir}/prefs.js`}),
-    ],
-  });
-  await reporter.handleMessage(message);
-
-  const call = fetchCallForMessage(fetchSpy, "Whatever");
-  const body = JSON.parse(call.args[1].body);
-  const exception = body.exception.values[0];
-  is(exception.module, "[UNKNOWN_LOCAL_FILEPATH]", "Unrecognized local file paths are mangled");
-
-  // Stackframe order is reversed from what is in the message.
-  const stackFrames = exception.stacktrace.frames;
-  is(
-    stackFrames[0].module, "[profileDir]/prefs.js",
-    "Paths within the profile directory are preserved but mangled",
-  );
-  is(
-    stackFrames[1].module, "[greDir]/defaults/prefs/channel-prefs.js",
-    "Paths within the GRE directory are preserved but mangled",
-  );
-  is(
-    stackFrames[2].module, "/inside/jar.jsm",
-    "Paths within jarfiles are extracted from the full jar: URL",
-  );
-});
-
-add_task(async function testFilePathMangleWhitespace() {
-  const fetchSpy = sinon.spy();
-  const manglePrefixes = {
-    whitespace: new FileUtils.File("/fake/GreD/with whitespace/"),
-  };
-  const reporter = new BrowserErrorReporter({fetch: fetchSpy, manglePrefixes});
-  await SpecialPowers.pushPrefEnv({set: [
-    [PREF_ENABLED, true],
-    [PREF_SAMPLE_RATE, "1.0"],
-  ]});
-
-  const message = createScriptError({
-    message: "Whatever",
-    sourceName: "file:///fake/GreD/with whitespace/remaining/file.jsm",
-  });
-  await reporter.handleMessage(message);
-
-  const call = fetchCallForMessage(fetchSpy, "Whatever");
-  const body = JSON.parse(call.args[1].body);
-  const exception = body.exception.values[0];
-  is(
-    exception.module, "[whitespace]/remaining/file.jsm",
-    "Prefixes with whitespace are correctly mangled",
-  );
-});