Skip to content
8 changes: 4 additions & 4 deletions app/scripts/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ import {
PersistenceManager,
} from './lib/stores/persistence-manager';
import ExtensionStore from './lib/stores/extension-store';
import ReadOnlyNetworkStore from './lib/stores/read-only-network-store';
import { FixtureExtensionStore } from './lib/stores/fixture-extension-store';
import migrations from './migrations';
import Migrator from './lib/migrator';
import { updateRemoteFeatureFlags } from './lib/update-remote-feature-flags';
Expand Down Expand Up @@ -114,10 +114,10 @@ const BADGE_COLOR_NOTIFICATION = '#D73847';
const BADGE_MAX_COUNT = 9;

const inTest = process.env.IN_TEST;
const useReadOnlyNetworkStore =
const useFixtureStore =
inTest && getManifestFlags().testing?.forceExtensionStore !== true;
const localStore = useReadOnlyNetworkStore
? new ReadOnlyNetworkStore()
const localStore = useFixtureStore
? new FixtureExtensionStore()
: new ExtensionStore();
const persistenceManager = new PersistenceManager({ localStore });

Expand Down
4 changes: 2 additions & 2 deletions app/scripts/lib/setup-initial-state-hooks.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { maskObject } from '../../../shared/modules/object.utils';
import ExtensionPlatform from '../platforms/extension';
import { SENTRY_BACKGROUND_STATE } from '../constants/sentry-state';
import ReadOnlyNetworkStore from './stores/read-only-network-store';
import { FixtureExtensionStore } from './stores/fixture-extension-store';
import ExtensionStore from './stores/extension-store';
import { PersistenceManager } from './stores/persistence-manager';

Expand All @@ -10,7 +10,7 @@ const platform = new ExtensionPlatform();
// This instance of `localStore` is used by Sentry to get the persisted state
const sentryLocalStore = new PersistenceManager({
localStore: process.env.IN_TEST
? new ReadOnlyNetworkStore()
? new FixtureExtensionStore()
: new ExtensionStore(),
});

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import log from 'loglevel';
import nock from 'nock';
import ReadOnlyNetworkStore from './read-only-network-store';
import browser from 'webextension-polyfill';
import { FixtureExtensionStore } from './fixture-extension-store';

const FIXTURE_SERVER_HOST = 'localhost';
const FIXTURE_SERVER_PORT = 12345;
Expand All @@ -13,15 +14,28 @@ const DEFAULT_INITIAL_STATE = {

const MOCK_STATE = { data: { config: { foo: 'bar' } }, meta: { version: 1 } };

/**
* Initiatilizes a ReadOnlyNetworkStore for testing
*
* @returns store - a ReadOnlyNetworkStore
*/
function setupReadOnlyNetworkStore() {
const store = new ReadOnlyNetworkStore();
return store;
}
jest.mock('webextension-polyfill', () => {
class MockBrowserStorage {
#state: unknown = null;

async get() {
return this.#state;
}

async set(value: unknown) {
this.#state = value;
}

async clear() {
this.#state = null;
}
}

return {
runtime: { lastError: null },
storage: { local: new MockBrowserStorage() },
};
});

/**
* Create a Nock scope for the fixture server response.
Expand All @@ -43,16 +57,16 @@ function setMockFixtureServerReply(
mockFixtureServerInterceptor().reply(200, state);
}

describe('ReadOnlyNetworkStore', () => {
beforeEach(() => {
jest.resetModules();
describe('FixtureExtensionStore', () => {
beforeEach(async () => {
await browser.storage.local.clear();
nock.cleanAll();
});

describe('constructor', () => {
it('loads state from the network if fetch is successful and response is ok', async () => {
setMockFixtureServerReply(MOCK_STATE);
const store = setupReadOnlyNetworkStore();
const store = new FixtureExtensionStore();

const result = await store.get();

Expand All @@ -64,7 +78,7 @@ describe('ReadOnlyNetworkStore', () => {
.spyOn(log, 'debug')
.mockImplementation(() => undefined);
mockFixtureServerInterceptor().reply(400);
const store = setupReadOnlyNetworkStore();
const store = new FixtureExtensionStore();

const result = await store.get();

Expand All @@ -79,7 +93,7 @@ describe('ReadOnlyNetworkStore', () => {
const logDebugSpy = jest
.spyOn(log, 'debug')
.mockImplementation(() => undefined);
const store = setupReadOnlyNetworkStore();
const store = new FixtureExtensionStore();

const result = await store.get();

Expand All @@ -91,18 +105,9 @@ describe('ReadOnlyNetworkStore', () => {
});

describe('get', () => {
it('returns null if #state is null', async () => {
mockFixtureServerInterceptor().reply(200);
const store = setupReadOnlyNetworkStore();

const result = await store.get();

expect(result).toBe(null);
});

it('returns null if state is null', async () => {
it('returns fixture state after waiting for init', async () => {
setMockFixtureServerReply(MOCK_STATE);
const store = setupReadOnlyNetworkStore();
const store = new FixtureExtensionStore();

const result = await store.get();

Expand All @@ -111,17 +116,8 @@ describe('ReadOnlyNetworkStore', () => {
});

describe('set', () => {
it('throws if not passed a state parameter', async () => {
const store = setupReadOnlyNetworkStore();

await expect(
// @ts-expect-error Intentionally passing incorrect type
store.set(undefined),
).rejects.toThrow('MetaMask - updated state is missing');
});

it('sets the state', async () => {
const store = setupReadOnlyNetworkStore();
const store = new FixtureExtensionStore();

await store.set({
data: { appState: { test: true } },
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import log from 'loglevel';
import getFetchWithTimeout from '../../../../shared/modules/fetch-with-timeout';
import type {
MetaMaskStateType,
BaseStore,
MetaMaskStorageStructure,
} from './base-store';
import ExtensionStore from './extension-store';
import type { MetaMaskStorageStructure } from './base-store';

const fetchWithTimeout = getFetchWithTimeout();

Expand All @@ -13,33 +10,25 @@ const FIXTURE_SERVER_PORT = 12345;
const FIXTURE_SERVER_URL = `http://${FIXTURE_SERVER_HOST}:${FIXTURE_SERVER_PORT}/state.json`;

/**
* A read-only network-based storage wrapper
* Derived class of ExtensionStore that initializes the store using the fixture server.
*/
export default class ReadOnlyNetworkStore implements BaseStore {
export class FixtureExtensionStore extends ExtensionStore {
#initialized: boolean = false;

#initializing?: Promise<void>;

#state: MetaMaskStateType | null = null;

constructor() {
super();
this.#initializing = this.#init();
}

/**
* Declares this store as compatible with the current browser
*/
isSupported = true;

/**
* Initializes by loading state from the network
*/
async #init() {
try {
const response = await fetchWithTimeout(FIXTURE_SERVER_URL);

if (response.ok) {
this.#state = await response.json();
const state = await response.json();
await super.set(state);
} else {
log.debug(
`Received response with a status of ${response.status} ${response.statusText}`,
Expand All @@ -57,37 +46,23 @@ export default class ReadOnlyNetworkStore implements BaseStore {
}
}

/**
* Returns state
*/
async get() {
async get(): Promise<MetaMaskStorageStructure | null> {
if (!this.#initialized) {
await this.#initializing;
}
return this.#state;
return super.get();
}

/**
* Overwrite in-memory copy of state.
*
* @param data - The data to set
*/
async set(data: Required<MetaMaskStorageStructure>): Promise<void> {
if (!data) {
throw new Error('MetaMask - updated state is missing');
}
if (!this.#initialized) {
await this.#initializing;
}
this.#state = data;
return super.set(data);
}

/**
* Resets data to its initial state.
*/
async reset(): Promise<void> {
this.#initialized = false;
this.#state = null;
await super.reset();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Race condition in reset method

The reset method has a race condition where #initialized is set to false before creating a new #initializing promise. If get or set is called between setting #initialized = false and this.#initializing = this.#init(), they will await the old resolved promise instead of the new initialization, potentially accessing uninitialized or reset state.

Fix in Cursor Fix in Web

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this matters in practice, but can fix it if anyone feels strongly about it

this.#initializing = this.#init();
await this.#initializing;
}
Expand Down
1 change: 0 additions & 1 deletion app/scripts/lib/stores/persistence-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ jest.mock('./extension-store', () => {
return { set: mockStoreSet, get: mockStoreGet };
});
});
jest.mock('./read-only-network-store');
jest.mock('loglevel', () => ({
error: jest.fn(),
}));
Expand Down
6 changes: 4 additions & 2 deletions test/e2e/tests/metrics/errors.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ async function matchesSnapshot({
update?: boolean;
}): Promise<void> {
const snapshotPath = resolve(__dirname, `./state-snapshots/${snapshot}.json`);
console.log('snapshotPath', snapshotPath);
const rawSnapshotData = await fs.readFile(snapshotPath, {
encoding: 'utf-8',
});
Expand Down Expand Up @@ -803,6 +802,10 @@ describe('Sentry errors', function () {
async ({ driver, mockedEndpoint }) => {
await driver.navigate();
await new LoginPage(driver).checkPageIsLoaded();

// Wait for state to settle
await driver.delay(5_000);

// Erase `getSentryAppState` hook, simulating a "before initialization" state
await driver.executeScript(
'window.stateHooks.getSentryAppState = undefined',
Expand Down Expand Up @@ -1148,7 +1151,6 @@ describe('Sentry errors', function () {
const mockTextBody = (await mockedRequest.body.getText()).split('\n');
const mockJsonBody = JSON.parse(mockTextBody[2]);
const appState = mockJsonBody?.extra?.appState;
console.log('mockJsonBody', mockJsonBody);
const { extensionId, installType } = mockJsonBody.extra;
assert.deepStrictEqual(Object.keys(appState), [
'browser',
Expand Down
Loading
Loading