From d8725aa75497c75963fc4a931af0ab799e9b1285 Mon Sep 17 00:00:00 2001 From: Michael Kelly Date: Tue, 12 Jun 2018 21:49:58 -0700 Subject: [PATCH] Bug 1467929: Only collect browser JS errors from recent builds. r=Gijs Telemetry data suggests between 13%-40% of errors being collected are from builds older than a week. Since Nightly updates twice a day, errors from builds that old don't reflect the current state of Nightly, so we can ignore them and save some bandwidth. A build is considered "recent" if the date encoded at the start of the appBuildId (YYYYMMDD) is within 7 days of the current date. Since this is mostly for preventing high load on the collection service, the check does not handle problems with the local clock being inaccurate in order to simplify the implementation. MozReview-Commit-ID: BbCO4kaBprL --HG-- extra : rebase_source : 0292ae57272f903a6aef176ef4403d56503fc0db --- browser/modules/BrowserErrorReporter.jsm | 28 ++++++++ .../browser/browser_BrowserErrorReporter.js | 66 ++++++++++++++++--- 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/browser/modules/BrowserErrorReporter.jsm b/browser/modules/BrowserErrorReporter.jsm index c58ad3b625c4..39a3041bdace 100644 --- a/browser/modules/BrowserErrorReporter.jsm +++ b/browser/modules/BrowserErrorReporter.jsm @@ -22,6 +22,7 @@ const PREF_PROJECT_ID = "browser.chrome.errorReporter.projectId"; const PREF_PUBLIC_KEY = "browser.chrome.errorReporter.publicKey"; const PREF_SAMPLE_RATE = "browser.chrome.errorReporter.sampleRate"; const PREF_SUBMIT_URL = "browser.chrome.errorReporter.submitUrl"; +const RECENT_BUILD_AGE = 1000 * 60 * 60 * 24 * 7; // 7 days const SDK_NAME = "firefox-error-reporter"; const SDK_VERSION = "1.0.0"; const TELEMETRY_ERROR_COLLECTED = "browser.errors.collected_count"; @@ -83,9 +84,22 @@ const MODULE_SAMPLE_RATES = new Map([ * traces; see bug 1426482 for privacy review and server-side mitigation. */ class BrowserErrorReporter { + /** + * Generate a Date object corresponding to the date in the appBuildId. + */ + static getAppBuildIdDate() { + const appBuildId = Services.appinfo.appBuildID; + const buildYear = Number.parseInt(appBuildId.slice(0, 4)); + // Date constructor uses 0-indexed months + const buildMonth = Number.parseInt(appBuildId.slice(4, 6)) - 1; + const buildDay = Number.parseInt(appBuildId.slice(6, 8)); + return new Date(buildYear, buildMonth, buildDay); + } + constructor(options = {}) { // Test arguments for mocks and changing behavior this.fetch = options.fetch || defaultFetch; + this.now = options.now || null; this.chromeOnly = options.chromeOnly !== undefined ? options.chromeOnly : true; this.registerListener = ( options.registerListener || (() => Services.console.registerListener(this)) @@ -94,6 +108,8 @@ class BrowserErrorReporter { options.unregisterListener || (() => Services.console.unregisterListener(this)) ); + XPCOMUtils.defineLazyGetter(this, "appBuildIdDate", BrowserErrorReporter.getAppBuildIdDate); + // Values that don't change between error reports. this.requestBodyTemplate = { logger: "javascript", @@ -198,6 +214,13 @@ class BrowserErrorReporter { return "FILTERED"; } + isRecentBuild() { + // The local clock is not reliable, but this method doesn't need to be + // perfect. + const now = this.now || new Date(); + return (now - this.appBuildIdDate) <= RECENT_BUILD_AGE; + } + observe(message) { if (message instanceof Ci.nsIScriptError) { ChromeUtils.idleDispatch(() => this.handleMessage(message)); @@ -221,6 +244,11 @@ class BrowserErrorReporter { Services.telemetry.keyedScalarAdd(TELEMETRY_ERROR_COLLECTED_FILENAME, key.slice(0, 69), 1); } + // Old builds should not send errors to Sentry + if (!this.isRecentBuild()) { + return; + } + // Sample the amount of errors we send out let sampleRate = Number.parseFloat(this.sampleRatePref); for (const [regex, rate] of MODULE_SAMPLE_RATES) { diff --git a/browser/modules/test/browser/browser_BrowserErrorReporter.js b/browser/modules/test/browser/browser_BrowserErrorReporter.js index 25ae83ad896c..e764131c28a1 100644 --- a/browser/modules/test/browser/browser_BrowserErrorReporter.js +++ b/browser/modules/test/browser/browser_BrowserErrorReporter.js @@ -115,6 +115,7 @@ add_task(async function testInitPastMessages() { fetch: fetchSpy, registerListener: noop, unregisterListener: noop, + now: BrowserErrorReporter.getAppBuildIdDate(), }); await SpecialPowers.pushPrefEnv({set: [ [PREF_ENABLED, true], @@ -143,6 +144,7 @@ add_task(async function testEnabledPrefWatcher() { unregisterListener() { listening = false; }, + now: BrowserErrorReporter.getAppBuildIdDate(), }); await SpecialPowers.pushPrefEnv({set: [ [PREF_ENABLED, false], @@ -160,7 +162,10 @@ add_task(async function testEnabledPrefWatcher() { add_task(async function testNonErrorLogs() { const fetchSpy = sinon.spy(); - const reporter = new BrowserErrorReporter({fetch: fetchSpy}); + const reporter = new BrowserErrorReporter({ + fetch: fetchSpy, + now: BrowserErrorReporter.getAppBuildIdDate(), + }); await SpecialPowers.pushPrefEnv({set: [ [PREF_ENABLED, true], [PREF_SAMPLE_RATE, "1.0"], @@ -199,7 +204,10 @@ add_task(async function testNonErrorLogs() { add_task(async function testSampling() { const fetchSpy = sinon.spy(); - const reporter = new BrowserErrorReporter({fetch: fetchSpy}); + const reporter = new BrowserErrorReporter({ + fetch: fetchSpy, + now: BrowserErrorReporter.getAppBuildIdDate(), + }); await SpecialPowers.pushPrefEnv({set: [ [PREF_ENABLED, true], [PREF_SAMPLE_RATE, "1.0"], @@ -256,7 +264,10 @@ add_task(async function testSampling() { add_task(async function testNameMessage() { const fetchSpy = sinon.spy(); - const reporter = new BrowserErrorReporter({fetch: fetchSpy}); + const reporter = new BrowserErrorReporter({ + fetch: fetchSpy, + now: BrowserErrorReporter.getAppBuildIdDate(), + }); await SpecialPowers.pushPrefEnv({set: [ [PREF_ENABLED, true], [PREF_SAMPLE_RATE, "1.0"], @@ -305,9 +316,34 @@ add_task(async function testNameMessage() { ); }); +add_task(async function testRecentBuild() { + // Create date that is guaranteed to be a month newer than the build date. + const nowDate = BrowserErrorReporter.getAppBuildIdDate(); + nowDate.setMonth(nowDate.getMonth() + 1); + + const fetchSpy = sinon.spy(); + const reporter = new BrowserErrorReporter({ + fetch: fetchSpy, + now: nowDate, + }); + await SpecialPowers.pushPrefEnv({set: [ + [PREF_ENABLED, true], + [PREF_SAMPLE_RATE, "1.0"], + ]}); + + await reporter.handleMessage(createScriptError({message: "Is error"})); + ok( + !fetchPassedError(fetchSpy, "Is error"), + "Reporter does not collect errors from builds older than a week.", + ); +}); + add_task(async function testFetchArguments() { const fetchSpy = sinon.spy(); - const reporter = new BrowserErrorReporter({fetch: fetchSpy}); + const reporter = new BrowserErrorReporter({ + fetch: fetchSpy, + now: BrowserErrorReporter.getAppBuildIdDate(), + }); await SpecialPowers.pushPrefEnv({set: [ [PREF_ENABLED, true], [PREF_SAMPLE_RATE, "1.0"], @@ -405,7 +441,11 @@ add_task(async function testAddonIDMangle() { const fetchSpy = sinon.spy(); // Passing false here disables category checks on errors, which would // otherwise block errors directly from extensions. - const reporter = new BrowserErrorReporter({fetch: fetchSpy, chromeOnly: false}); + const reporter = new BrowserErrorReporter({ + fetch: fetchSpy, + chromeOnly: false, + now: BrowserErrorReporter.getAppBuildIdDate(), + }); await SpecialPowers.pushPrefEnv({set: [ [PREF_ENABLED, true], [PREF_SAMPLE_RATE, "1.0"], @@ -447,7 +487,11 @@ add_task(async function testExtensionTag() { const fetchSpy = sinon.spy(); // Passing false here disables category checks on errors, which would // otherwise block errors directly from extensions. - const reporter = new BrowserErrorReporter({fetch: fetchSpy, chromeOnly: false}); + const reporter = new BrowserErrorReporter({ + fetch: fetchSpy, + chromeOnly: false, + now: BrowserErrorReporter.getAppBuildIdDate(), + }); await SpecialPowers.pushPrefEnv({set: [ [PREF_ENABLED, true], [PREF_SAMPLE_RATE, "1.0"], @@ -488,7 +532,10 @@ add_task(async function testExtensionTag() { add_task(async function testScalars() { const fetchStub = sinon.stub(); - const reporter = new BrowserErrorReporter({fetch: fetchStub}); + const reporter = new BrowserErrorReporter({ + fetch: fetchStub, + now: BrowserErrorReporter.getAppBuildIdDate(), + }); await SpecialPowers.pushPrefEnv({set: [ [PREF_ENABLED, true], [PREF_SAMPLE_RATE, "1.0"], @@ -574,7 +621,10 @@ add_task(async function testScalars() { add_task(async function testCollectedFilenameScalar() { const fetchStub = sinon.stub(); - const reporter = new BrowserErrorReporter(fetchStub); + const reporter = new BrowserErrorReporter({ + fetch: fetchStub, + now: BrowserErrorReporter.getAppBuildIdDate(), + }); await SpecialPowers.pushPrefEnv({set: [ [PREF_ENABLED, true], [PREF_SAMPLE_RATE, "1.0"],