From 52f38fb0a9494433c20a3c3668002a234c634f81 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Thu, 2 Nov 2017 20:06:45 -0400 Subject: [PATCH] POST error messages to reporting server (#11913) * POST error messages to reporting server Fixes #8849. * Fix types --- src/error.js | 93 +++++------ test/functional/test-error.js | 295 +++++++++++++--------------------- 2 files changed, 156 insertions(+), 232 deletions(-) diff --git a/src/error.js b/src/error.js index 641e49b3983bb..066d2e52dfe7f 100644 --- a/src/error.js +++ b/src/error.js @@ -263,11 +263,13 @@ function reportErrorToServer(message, filename, line, col, error) { // due to buggy browser extensions may be helpful to notify authors. return; } - const url = getErrorReportUrl(message, filename, line, col, error, + const data = getErrorReportData(message, filename, line, col, error, hasNonAmpJs); - if (url) { + if (data) { reportingBackoff(() => { - new Image().src = url; + const xhr = new XMLHttpRequest(); + xhr.open('POST', urls.errorReporting, true); + xhr.send(JSON.stringify(data)); }); } } @@ -280,10 +282,10 @@ function reportErrorToServer(message, filename, line, col, error) { * @param {string|undefined} col * @param {*|undefined} error * @param {boolean} hasNonAmpJs - * @return {string|undefined} The URL + * @return {!JsonObject|undefined} The data to post * visibleForTesting */ -export function getErrorReportUrl(message, filename, line, col, error, +export function getErrorReportData(message, filename, line, col, error, hasNonAmpJs) { let expected = false; if (error) { @@ -335,63 +337,61 @@ export function getErrorReportUrl(message, filename, line, col, error, } // This is the App Engine app in - // ../tools/errortracker + // https://github.com/ampproject/error-tracker // It stores error reports via https://cloud.google.com/error-reporting/ // for analyzing production issues. - let url = urls.errorReporting + - '?v=' + getMode().rtvVersion + - '&noAmp=' + (hasNonAmpJs ? 1 : 0) + - '&m=' + encodeURIComponent(message.replace(USER_ERROR_SENTINEL, '')) + - '&a=' + (isUserError ? 1 : 0); - if (expected) { - // Errors are tagged with "ex" ("expected") label to allow loggers to - // classify these errors as benchmarks and not exceptions. - url += '&ex=1'; - } + const data = /** @type {!JsonObject} */ (Object.create(null)); + data['v'] = getMode().rtvVersion; + data['noAmp'] = hasNonAmpJs ? '1' : '0'; + data['m'] = message.replace(USER_ERROR_SENTINEL, ''); + data['a'] = isUserError ? '1' : '0'; + + // Errors are tagged with "ex" ("expected") label to allow loggers to + // classify these errors as benchmarks and not exceptions. + data['ex'] = expected ? '1' : '0'; let runtime = '1p'; if (self.context && self.context.location) { - url += '&3p=1'; + data['3p'] = '1'; runtime = '3p'; } else if (getMode().runtime) { runtime = getMode().runtime; } - url += '&rt=' + runtime; + data['rt'] = runtime; // TODO(erwinm): Remove ca when all systems read `bt` instead of `ca` to // identify js binary type. - if (isCanary(self)) { - url += '&ca=1'; - } + data['ca'] = isCanary(self) ? '1' : '0'; + // Pass binary type. - url += '&bt=' + getBinaryType(self); + data['bt'] = getBinaryType(self); if (self.location.ancestorOrigins && self.location.ancestorOrigins[0]) { - url += '&or=' + encodeURIComponent(self.location.ancestorOrigins[0]); + data['or'] = self.location.ancestorOrigins[0]; } if (self.viewerState) { - url += '&vs=' + encodeURIComponent(self.viewerState); + data['vs'] = self.viewerState; } // Is embedded? if (self.parent && self.parent != self) { - url += '&iem=1'; + data['iem'] = '1'; } if (self.AMP && self.AMP.viewer) { const resolvedViewerUrl = self.AMP.viewer.getResolvedViewerUrl(); const messagingOrigin = self.AMP.viewer.maybeGetMessagingOrigin(); if (resolvedViewerUrl) { - url += `&rvu=${encodeURIComponent(resolvedViewerUrl)}`; + data['rvu'] = resolvedViewerUrl; } if (messagingOrigin) { - url += `&mso=${encodeURIComponent(messagingOrigin)}`; + data['mso'] = messagingOrigin; } } if (!detectedJsEngine) { detectedJsEngine = detectJsEngineFromStack(); } - url += `&jse=${detectedJsEngine}`; + data['jse'] = detectedJsEngine; const exps = []; const experiments = experimentTogglesOrNull(self); @@ -399,44 +399,35 @@ export function getErrorReportUrl(message, filename, line, col, error, const on = experiments[exp]; exps.push(`${exp}=${on ? '1' : '0'}`); } - url += `&exps=${encodeURIComponent(exps.join(','))}`; + data['exps'] = exps.join(','); if (error) { const tagName = error && error.associatedElement ? error.associatedElement.tagName : 'u'; // Unknown - url += `&el=${encodeURIComponent(tagName)}`; + data['el'] = tagName; + if (error.args) { - url += `&args=${encodeURIComponent(JSON.stringify(error.args))}`; + data['args'] = JSON.stringify(error.args); } if (!isUserError && !error.ignoreStack && error.stack) { - // Shorten - const stack = (error.stack || '').substr(0, 1000); - url += `&s=${encodeURIComponent(stack)}`; + data['s'] = error.stack; } error.message += ' _reported_'; } else { - url += '&f=' + encodeURIComponent(filename || '') + - '&l=' + encodeURIComponent(line || '') + - '&c=' + encodeURIComponent(col || ''); + data['f'] = filename || ''; + data['l'] = line || ''; + data['c'] = col || ''; } - url += '&r=' + encodeURIComponent(self.document.referrer); - url += '&ae=' + encodeURIComponent(accumulatedErrorMessages.join(',')); + data['r'] = self.document.referrer; + data['ae'] = accumulatedErrorMessages.join(','); + data['fr'] = self.location.originalHash || self.location.hash; + accumulatedErrorMessages.push(message); - url += '&fr=' + encodeURIComponent(self.location.originalHash - || self.location.hash); - - // Google App Engine maximum URL length. - if (url.length >= 2072) { - url = url.substr(0, 2072 - 8 /* length of suffix */) - // Full remove last URL encoded entity. - .replace(/\%[^&%]+$/, '') - // Sentinel - + '&SHORT=1'; - } - return url; + + return data; } /** diff --git a/test/functional/test-error.js b/test/functional/test-error.js index 990f3a9b3ec29..d55a81eb4f220 100644 --- a/test/functional/test-error.js +++ b/test/functional/test-error.js @@ -17,14 +17,13 @@ import { cancellation, detectNonAmpJs, - getErrorReportUrl, + getErrorReportData, installErrorReporting, isCancellation, reportError, detectJsEngineFromStack, reportErrorToAnalytics, } from '../../src/error'; -import {parseUrl, parseQueryString} from '../../src/url'; import {user} from '../../src/log'; import { resetExperimentTogglesForTesting, @@ -116,91 +115,70 @@ describe('reportErrorToServer', () => { if (!e.stack || e.stack.indexOf('SHOULD_BE_IN_STACK') == -1) { e.stack = 'SHOULD_BE_IN_STACK'; } - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e, - true)); - const query = parseQueryString(url.search); - expect(url.href.indexOf( - 'https://amp-error-reporting.appspot.com/r?')).to.equal(0); - - expect(query.m).to.equal('XYZ'); - expect(query.el).to.equal('u'); - expect(query.a).to.equal('0'); - expect(query.s).to.contain('SHOULD_BE_IN_STACK'); - expect(query['3p']).to.equal(undefined); + const data = getErrorReportData(undefined, undefined, undefined, undefined, + e, true); + expect(data.m).to.equal('XYZ'); + expect(data.el).to.equal('u'); + expect(data.a).to.equal('0'); + expect(data.s).to.contain('SHOULD_BE_IN_STACK'); + expect(data['3p']).to.equal(undefined); expect(e.message).to.contain('_reported_'); if (location.ancestorOrigins) { - expect(query.or).to.contain('http://localhost'); + expect(data.or).to.contain('http://localhost'); } - expect(query.vs).to.be.undefined; - expect(query.ae).to.equal(''); - expect(query.r).to.contain('http://localhost'); - expect(query.noAmp).to.equal('1'); - expect(query.args).to.be.undefined; + expect(data.vs).to.be.undefined; + expect(data.ae).to.equal(''); + expect(data.r).to.contain('http://localhost'); + expect(data.noAmp).to.equal('1'); + expect(data.args).to.be.undefined; }); it('reportError with error and ignore stack', () => { const e = new Error('XYZ'); e.ignoreStack = true; - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e, - true)); - const query = parseQueryString(url.search); - expect(query.s).to.be.undefined; - - expect(url.href.indexOf( - 'https://amp-error-reporting.appspot.com/r?')).to.equal(0); - expect(query.m).to.equal('XYZ'); - expect(query.el).to.equal('u'); - expect(query.a).to.equal('0'); - expect(query['3p']).to.equal(undefined); + const data = getErrorReportData(undefined, undefined, undefined, undefined, + e, true); + expect(data.m).to.equal('XYZ'); + expect(data.el).to.equal('u'); + expect(data.a).to.equal('0'); + expect(data['3p']).to.equal(undefined); expect(e.message).to.contain('_reported_'); }); it('reportError with error object w/args', () => { const e = new Error('XYZ'); e.args = {x: 1}; - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e, - true)); - const query = parseQueryString(url.search); - - expect(query.args).to.equal(JSON.stringify({x: 1})); + const data = getErrorReportData(undefined, undefined, undefined, undefined, + e, true); + expect(data.args).to.equal(JSON.stringify({x: 1})); }); it('reportError with a string instead of error', () => { - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, - 'string error', - true)); - const query = parseQueryString(url.search); - expect(query.m).to.equal('string error'); + const data = getErrorReportData(undefined, undefined, undefined, undefined, + 'string error', + true); + expect(data.m).to.equal('string error'); }); it('reportError with no error', () => { - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, - undefined, - true)); - const query = parseQueryString(url.search); - expect(query.m).to.equal('Unknown error'); + const data = getErrorReportData(undefined, undefined, undefined, undefined, + undefined, + true); + expect(data.m).to.equal('Unknown error'); }); it('reportError with associatedElement', () => { const e = new Error('XYZ'); const el = document.createElement('foo-bar'); e.associatedElement = el; - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e, - false)); - const query = parseQueryString(url.search); - - expect(query.m).to.equal('XYZ'); - expect(query.el).to.equal('FOO-BAR'); - expect(query.a).to.equal('0'); - expect(query.v).to.equal( + const data = getErrorReportData(undefined, undefined, undefined, undefined, + e, false); + expect(data.m).to.equal('XYZ'); + expect(data.el).to.equal('FOO-BAR'); + expect(data.a).to.equal('0'); + expect(data.v).to.equal( getRtvVersionForTesting(window, getMode().localDev)); - expect(query.noAmp).to.equal('0'); + expect(data.noAmp).to.equal('0'); }); it('reportError mark asserts', () => { @@ -210,13 +188,11 @@ describe('reportErrorToServer', () => { } catch (error) { e = error; } - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e)); - const query = parseQueryString(url.search); - - expect(query.m).to.equal('XYZ'); - expect(query.a).to.equal('1'); - expect(query.v).to.equal( + const data = getErrorReportData(undefined, undefined, undefined, undefined, + e); + expect(data.m).to.equal('XYZ'); + expect(data.a).to.equal('1'); + expect(data.v).to.equal( getRtvVersionForTesting(window, getMode().localDev)); }); @@ -227,13 +203,10 @@ describe('reportErrorToServer', () => { } catch (error) { e = error; } - const url = parseUrl( - getErrorReportUrl(e.message, undefined, undefined, undefined)); - const query = parseQueryString(url.search); - - expect(query.m).to.equal('XYZ'); - expect(query.a).to.equal('1'); - expect(query.v).to.equal( + const data = getErrorReportData(e.message, undefined, undefined, undefined); + expect(data.m).to.equal('XYZ'); + expect(data.a).to.equal('1'); + expect(data.v).to.equal( getRtvVersionForTesting(window, getMode().localDev)); }); @@ -243,12 +216,10 @@ describe('reportErrorToServer', () => { }; const e = new Error('XYZ'); e.fromAssert = true; - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e)); - const query = parseQueryString(url.search); - - expect(query.m).to.equal('XYZ'); - expect(query['3p']).to.equal('1'); + const data = getErrorReportData(undefined, undefined, undefined, undefined, + e); + expect(data.m).to.equal('XYZ'); + expect(data['3p']).to.equal('1'); }); it('reportError marks canary and viewerState', () => { @@ -258,13 +229,11 @@ describe('reportErrorToServer', () => { }; const e = new Error('XYZ'); e.fromAssert = true; - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e)); - const query = parseQueryString(url.search); - - expect(query.m).to.equal('XYZ'); - expect(query['ca']).to.equal('1'); - expect(query['vs']).to.equal('some-state'); + const data = getErrorReportData(undefined, undefined, undefined, undefined, + e); + expect(data.m).to.equal('XYZ'); + expect(data['ca']).to.equal('1'); + expect(data['vs']).to.equal('some-state'); }); it('reportError marks binary type', () => { @@ -272,84 +241,52 @@ describe('reportErrorToServer', () => { type: 'canary', }; const e = new Error('XYZ'); - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e)); - const query = parseQueryString(url.search); - - expect(query.m).to.equal('XYZ'); - expect(query['bt']).to.equal('canary'); + const data = getErrorReportData(undefined, undefined, undefined, undefined, + e); + expect(data.m).to.equal('XYZ'); + expect(data['bt']).to.equal('canary'); window.AMP_CONFIG = { type: 'control', }; const e1 = new Error('XYZ'); - const url1 = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e1)); - const query1 = parseQueryString(url1.search); - - expect(query1.m).to.equal('XYZ'); - expect(query1['bt']).to.equal('control'); + const data1 = getErrorReportData(undefined, undefined, undefined, undefined, + e1); + expect(data1.m).to.equal('XYZ'); + expect(data1['bt']).to.equal('control'); window.AMP_CONFIG = {}; const e2 = new Error('ABC'); - const url2 = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e2)); - const query2 = parseQueryString(url2.search); - - expect(query2.m).to.equal('ABC'); - expect(query2['bt']).to.equal('unknown'); + const data2 = getErrorReportData(undefined, undefined, undefined, undefined, + e2); + expect(data2.m).to.equal('ABC'); + expect(data2['bt']).to.equal('unknown'); }); it('reportError without error object', () => { - const url = parseUrl( - getErrorReportUrl('foo bar', 'foo.js', '11', '22', undefined)); - const query = parseQueryString(url.search); - expect(url.href.indexOf( - 'https://amp-error-reporting.appspot.com/r?')).to.equal(0); - - expect(query.m).to.equal('foo bar'); - expect(query.f).to.equal('foo.js'); - expect(query.l).to.equal('11'); - expect(query.c).to.equal('22'); - expect(query.SHORT).to.be.undefined; + const data = getErrorReportData('foo bar', 'foo.js', '11', '22', undefined); + expect(data.m).to.equal('foo bar'); + expect(data.f).to.equal('foo.js'); + expect(data.l).to.equal('11'); + expect(data.c).to.equal('22'); }); it('should accumulate errors', () => { - parseUrl(getErrorReportUrl(undefined, undefined, undefined, undefined, - new Error('1'),true)); - parseUrl(getErrorReportUrl(undefined, undefined, undefined, undefined, - new Error('2'),true)); - const url = parseUrl(getErrorReportUrl(undefined, undefined, undefined, - undefined, new Error('3'),true)); - const query = parseQueryString(url.search); - expect(url.href.indexOf( - 'https://amp-error-reporting.appspot.com/r?')).to.equal(0); - - expect(query.m).to.equal('3'); - expect(query.ae).to.equal('1,2'); - }); - - it('should shorten very long reports', () => { - let message = 'TEST'; - for (let i = 0; i < 4000; i++) { - message += '&'; - } - const url = parseUrl(getErrorReportUrl(undefined, undefined, undefined, - undefined, new Error(message),true)); - const query = parseQueryString(url.search); - expect(url.href.indexOf( - 'https://amp-error-reporting.appspot.com/r?')).to.equal(0); - - expect(query.m).to.match(/^TEST/); - expect(url.href.length <= 2072).to.be.ok; - expect(query.SHORT).to.equal('1'); + getErrorReportData(undefined, undefined, undefined, undefined, + new Error('1'),true); + getErrorReportData(undefined, undefined, undefined, undefined, + new Error('2'),true); + const data = getErrorReportData(undefined, undefined, undefined, + undefined, new Error('3'),true); + expect(data.m).to.equal('3'); + expect(data.ae).to.equal('1,2'); }); it('should not double report', () => { const e = new Error('something _reported_'); - const url = - getErrorReportUrl(undefined, undefined, undefined, undefined, e); - expect(url).to.be.undefined; + const data = + getErrorReportData(undefined, undefined, undefined, undefined, e); + expect(data).to.be.undefined; }); it('should construct cancellation', () => { @@ -375,9 +312,9 @@ describe('reportErrorToServer', () => { it('reportError with error object', () => { const e = cancellation(); - const url = - getErrorReportUrl(undefined, undefined, undefined, undefined, e); - expect(url).to.be.undefined; + const data = + getErrorReportData(undefined, undefined, undefined, undefined, e); + expect(data).to.be.undefined; }); it('should throttle user errors', () => { @@ -388,62 +325,60 @@ describe('reportErrorToServer', () => { } catch (error) { e = error; } - const url = - getErrorReportUrl(undefined, undefined, undefined, undefined, e); - expect(url).to.be.undefined; + const data = + getErrorReportData(undefined, undefined, undefined, undefined, e); + expect(data).to.be.undefined; }); it('should not report load errors', () => { nextRandomNumber = 1e-3 + 1e-4; const e = new Error('Failed to load:'); - const url = - getErrorReportUrl(undefined, undefined, undefined, undefined, e); - expect(url).to.be.undefined; + const data = + getErrorReportData(undefined, undefined, undefined, undefined, e); + expect(data).to.be.undefined; }); it('should report throttled load errors at threshold', () => { nextRandomNumber = 1e-3; const e = new Error('Failed to load:'); - const url = - getErrorReportUrl(undefined, undefined, undefined, undefined, e); - expect(url).to.be.ok; - expect(url).to.contain('&ex=1'); + const data = + getErrorReportData(undefined, undefined, undefined, undefined, e); + expect(data).to.be.ok; + expect(data.ex).to.equal('1'); }); it('should not report Script errors', () => { nextRandomNumber = 1e-3 + 1e-4; const e = new Error('Script error.'); - const url = - getErrorReportUrl(undefined, undefined, undefined, undefined, e); - expect(url).to.be.undefined; + const data = + getErrorReportData(undefined, undefined, undefined, undefined, e); + expect(data).to.be.undefined; }); it('should report throttled Script errors at threshold', () => { nextRandomNumber = 1e-3; const e = new Error('Script error.'); - const url = - getErrorReportUrl(undefined, undefined, undefined, undefined, e); - expect(url).to.be.ok; - expect(url).to.contain('&ex=1'); + const data = + getErrorReportData(undefined, undefined, undefined, undefined, e); + expect(data).to.be.ok; + expect(data.ex).to.contain('1'); }); it('should report throttled load errors under threshold', () => { nextRandomNumber = 1e-3 - 1e-4; const e = new Error('Failed to load:'); - const url = - getErrorReportUrl(undefined, undefined, undefined, undefined, e); - expect(url).to.be.ok; - expect(url).to.contain('&ex=1'); + const data = + getErrorReportData(undefined, undefined, undefined, undefined, e); + expect(data).to.be.ok; + expect(data.ex).to.contain('1'); }); it('should omit the error stack for user errors', () => { const e = user().createError('123'); - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e, - true)); - const query = parseQueryString(url.search); - expect(query.s).to.be.undefined; + const data = getErrorReportData(undefined, undefined, undefined, undefined, + e, true); + expect(data.s).to.be.undefined; }); it('should report experiments', () => { @@ -453,11 +388,9 @@ describe('reportErrorToServer', () => { toggleExperiment(window, 'disabled-exp', true); toggleExperiment(window, 'disabled-exp', false); const e = user().createError('123'); - const url = parseUrl( - getErrorReportUrl(undefined, undefined, undefined, undefined, e, - true)); - const query = parseQueryString(url.search); - expect(query.exps).to.equal('test-exp=1,disabled-exp=0'); + const data = getErrorReportData(undefined, undefined, undefined, undefined, + e, true); + expect(data.exps).to.equal('test-exp=1,disabled-exp=0'); }); describe('detectNonAmpJs', () => {