From 6f0caf4d3f269fc3da3307904162f4120566a916 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 31 Jul 2023 20:35:50 -0230 Subject: [PATCH] Capture Sentry errors prior to initialization (#20265) (#20330) * Capture Sentry errors prior to initialization Sentry errors captured before/during the wallet initialization are currently not captured because we don't have the controller state yet to determine whether the user has consented. The Sentry setup has been updated to check the persisted state for whether the user has consented, as a fallback in case the controller state hasn't been initialized yet. This ensures that we capture errors during initialization if the user has opted in. * Always await async check for whether the user has opted in * Remove unused import * Update JSDoc return type * Remove unused driver method * Fix metametrics controller unit tests * Fix e2e tests * Fix e2e test on Firefox * Start session upon install rather than toggle Co-authored-by: Mark Stacey --- app/scripts/background.js | 4 + app/scripts/lib/sentry-filter-events.ts | 8 +- app/scripts/lib/setup-persisted-state-hook.js | 10 + app/scripts/lib/setupSentry.js | 20 +- app/scripts/ui.js | 4 + test/e2e/tests/errors.spec.js | 199 ++++++++++++++---- 6 files changed, 195 insertions(+), 50 deletions(-) create mode 100644 app/scripts/lib/setup-persisted-state-hook.js diff --git a/app/scripts/background.js b/app/scripts/background.js index e3416b796..44d3870ad 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -2,6 +2,10 @@ * @file The entry point for the web extension singleton process. */ +// This import sets up a global function required for Sentry to function. +// It must be run first in case an error is thrown later during initialization. +import './lib/setup-persisted-state-hook'; + import EventEmitter from 'events'; import endOfStream from 'end-of-stream'; import pump from 'pump'; diff --git a/app/scripts/lib/sentry-filter-events.ts b/app/scripts/lib/sentry-filter-events.ts index 050f0bcd2..50d2f4c77 100644 --- a/app/scripts/lib/sentry-filter-events.ts +++ b/app/scripts/lib/sentry-filter-events.ts @@ -29,7 +29,7 @@ export class FilterEvents implements Integration { * @returns `true` if MetaMask's state has been initialized, and MetaMetrics * is enabled, `false` otherwise. */ - private getMetaMetricsEnabled: () => boolean; + private getMetaMetricsEnabled: () => Promise; /** * @param options - Constructor options. @@ -40,7 +40,7 @@ export class FilterEvents implements Integration { constructor({ getMetaMetricsEnabled, }: { - getMetaMetricsEnabled: () => boolean; + getMetaMetricsEnabled: () => Promise; }) { this.getMetaMetricsEnabled = getMetaMetricsEnabled; } @@ -56,13 +56,13 @@ export class FilterEvents implements Integration { addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub, ): void { - addGlobalEventProcessor((currentEvent: SentryEvent) => { + addGlobalEventProcessor(async (currentEvent: SentryEvent) => { // Sentry integrations use the Sentry hub to get "this" references, for // reasons I don't fully understand. // eslint-disable-next-line consistent-this const self = getCurrentHub().getIntegration(FilterEvents); if (self) { - if (!self.getMetaMetricsEnabled()) { + if (!(await self.getMetaMetricsEnabled())) { logger.warn(`Event dropped due to MetaMetrics setting.`); return null; } diff --git a/app/scripts/lib/setup-persisted-state-hook.js b/app/scripts/lib/setup-persisted-state-hook.js new file mode 100644 index 000000000..9b29fad26 --- /dev/null +++ b/app/scripts/lib/setup-persisted-state-hook.js @@ -0,0 +1,10 @@ +import LocalStore from './local-store'; +import ReadOnlyNetworkStore from './network-store'; + +const localStore = process.env.IN_TEST + ? new ReadOnlyNetworkStore() + : new LocalStore(); + +globalThis.stateHooks.getPersistedState = async function () { + return await localStore.get(); +}; diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js index bac3c79ce..dd0eb7bd6 100644 --- a/app/scripts/lib/setupSentry.js +++ b/app/scripts/lib/setupSentry.js @@ -118,16 +118,20 @@ export default function setupSentry({ release, getState }) { * @returns `true` if MetaMask's state has been initialized, and MetaMetrics * is enabled, `false` otherwise. */ - function getMetaMetricsEnabled() { - if (getState) { - const appState = getState(); - if (!appState?.store?.metamask?.participateInMetaMetrics) { - return false; - } - } else { + async function getMetaMetricsEnabled() { + const appState = getState(); + if (Object.keys(appState) > 0) { + return Boolean(appState?.store?.metamask?.participateInMetaMetrics); + } + try { + const persistedState = await globalThis.stateHooks.getPersistedState(); + return Boolean( + persistedState?.data?.MetaMetricsController?.participateInMetaMetrics, + ); + } catch (error) { + console.error(error); return false; } - return true; } Sentry.init({ diff --git a/app/scripts/ui.js b/app/scripts/ui.js index 148ce0397..565e8187a 100644 --- a/app/scripts/ui.js +++ b/app/scripts/ui.js @@ -4,6 +4,10 @@ import '@formatjs/intl-relativetimeformat/polyfill'; // dev only, "react-devtools" import is skipped in prod builds import 'react-devtools'; +// This import sets up a global function required for Sentry to function. +// It must be run first in case an error is thrown later during initialization. +import './lib/setup-persisted-state-hook'; + import PortStream from 'extension-port-stream'; import browser from 'webextension-polyfill'; diff --git a/test/e2e/tests/errors.spec.js b/test/e2e/tests/errors.spec.js index 194578558..960135215 100644 --- a/test/e2e/tests/errors.spec.js +++ b/test/e2e/tests/errors.spec.js @@ -1,9 +1,26 @@ const { strict: assert } = require('assert'); +const { Browser } = require('selenium-webdriver'); const { convertToHexValue, withFixtures } = require('../helpers'); const FixtureBuilder = require('../fixture-builder'); describe('Sentry errors', function () { - async function mockSentry(mockServer) { + const migrationError = + process.env.SELENIUM_BROWSER === Browser.CHROME + ? `Cannot read properties of undefined (reading 'version')` + : 'meta is undefined'; + async function mockSentryMigratorError(mockServer) { + return await mockServer + .forPost('https://sentry.io/api/0000000/envelope/') + .withBodyIncluding(migrationError) + .thenCallback(() => { + return { + statusCode: 200, + json: {}, + }; + }); + } + + async function mockSentryTestError(mockServer) { return await mockServer .forPost('https://sentry.io/api/0000000/envelope/') .withBodyIncluding('Test Error') @@ -23,43 +40,149 @@ describe('Sentry errors', function () { }, ], }; - it('should send error events', async function () { - await withFixtures( - { - fixtures: new FixtureBuilder() - .withMetaMetricsController({ - metaMetricsId: 'fake-metrics-id', - participateInMetaMetrics: true, - }) - .build(), - ganacheOptions, - title: this.test.title, - failOnConsoleError: false, - testSpecificMock: mockSentry, - }, - async ({ driver, mockedEndpoint }) => { - await driver.navigate(); - await driver.fill('#password', 'correct horse battery staple'); - await driver.press('#password', driver.Key.ENTER); - // Trigger error - driver.executeScript('window.stateHooks.throwTestError()'); - // Wait for Sentry request - await driver.wait(async () => { + + describe('before initialization', function () { + it('should NOT send error events when participateInMetaMetrics is false', async function () { + await withFixtures( + { + fixtures: { + ...new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: null, + participateInMetaMetrics: false, + }) + .build(), + // Intentionally corrupt state to trigger migration error during initialization + meta: undefined, + }, + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryMigratorError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + + // Wait for Sentry request + await driver.delay(3000); const isPending = await mockedEndpoint.isPending(); - return isPending === false; - }, 10000); - const [mockedRequest] = await mockedEndpoint.getSeenRequests(); - const mockTextBody = mockedRequest.body.text.split('\n'); - const mockJsonBody = JSON.parse(mockTextBody[2]); - const { level, extra } = mockJsonBody; - const [{ type, value }] = mockJsonBody.exception.values; - const { participateInMetaMetrics } = extra.appState.store.metamask; - // Verify request - assert.equal(type, 'TestError'); - assert.equal(value, 'Test Error'); - assert.equal(level, 'error'); - assert.equal(participateInMetaMetrics, true); - }, - ); + assert.ok( + isPending, + 'A request to sentry was sent when it should not have been', + ); + }, + ); + }); + it('should send error events', async function () { + await withFixtures( + { + fixtures: { + ...new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: 'fake-metrics-id', + participateInMetaMetrics: true, + }) + .build(), + // Intentionally corrupt state to trigger migration error during initialization + meta: undefined, + }, + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryMigratorError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + + // Wait for Sentry request + await driver.wait(async () => { + const isPending = await mockedEndpoint.isPending(); + return isPending === false; + }, 3000); + + const [mockedRequest] = await mockedEndpoint.getSeenRequests(); + const mockTextBody = mockedRequest.body.text.split('\n'); + const mockJsonBody = JSON.parse(mockTextBody[2]); + const { level } = mockJsonBody; + const [{ type, value }] = mockJsonBody.exception.values; + // Verify request + assert.equal(type, 'TypeError'); + assert(value.includes(migrationError)); + assert.equal(level, 'error'); + }, + ); + }); + }); + + describe('after initialization', function () { + it('should NOT send error events when participateInMetaMetrics is false', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: null, + participateInMetaMetrics: false, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryTestError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.fill('#password', 'correct horse battery staple'); + await driver.press('#password', driver.Key.ENTER); + // Trigger error + driver.executeScript('window.stateHooks.throwTestError()'); + driver.delay(3000); + // Wait for Sentry request + const isPending = await mockedEndpoint.isPending(); + assert.ok( + isPending, + 'A request to sentry was sent when it should not have been', + ); + }, + ); + }); + it('should send error events', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder() + .withMetaMetricsController({ + metaMetricsId: 'fake-metrics-id', + participateInMetaMetrics: true, + }) + .build(), + ganacheOptions, + title: this.test.title, + failOnConsoleError: false, + testSpecificMock: mockSentryTestError, + }, + async ({ driver, mockedEndpoint }) => { + await driver.navigate(); + await driver.fill('#password', 'correct horse battery staple'); + await driver.press('#password', driver.Key.ENTER); + // Trigger error + driver.executeScript('window.stateHooks.throwTestError()'); + // Wait for Sentry request + await driver.wait(async () => { + const isPending = await mockedEndpoint.isPending(); + return isPending === false; + }, 10000); + const [mockedRequest] = await mockedEndpoint.getSeenRequests(); + const mockTextBody = mockedRequest.body.text.split('\n'); + const mockJsonBody = JSON.parse(mockTextBody[2]); + const { level, extra } = mockJsonBody; + const [{ type, value }] = mockJsonBody.exception.values; + const { participateInMetaMetrics } = extra.appState.store.metamask; + // Verify request + assert.equal(type, 'TestError'); + assert.equal(value, 'Test Error'); + assert.equal(level, 'error'); + assert.equal(participateInMetaMetrics, true); + }, + ); + }); }); });