Skip to content

Commit 108263d

Browse files
GudahttGudahtt
authored
fix: cp-12.15.0 Prevent fullscreen UI from opening every startup (#31332)
## **Description** Prevent tab from opening upon every browser startup. A [recent PR](#29826) updated the logic for how we determine whether the extension was just installed or not; we used to check state, but now we use the `runtime.onInstalled` event instead to store a flag in session storage indicating whether the extension was just installed. Unfortunately this PR had a couple of bugs; the condition using this flag was accidentally reversed, and the read from session storage was never successful because of a race condition (the write had not finished yet). To address both problems, the logic for detecting first install was refactored to use a global variable instead of session storage. Globals can be accessed and written synchronously, preventing any possibility of a race condition. The solution was complicated by the fact that MV3 builds must add an `onInstalled` listener in the root service worker module, otherwise the listener is never called (we tested this experimentally). It was also unclear whether the listener would always run before or after the `background.js` script loading, and we need to trigger an action in `background.js` on install. But we've accounted for both possibilities in this solution. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31332?quickstart=1) ## **Related issues** Fixes: #30924 ## **Manual testing steps** * Create a production-like build (`yarn dist` and `yarn dist:mv2`), or download the builds from the `metamaskbot` comment * Test that the window opens only on first install, not on later browser restarts. ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
1 parent 478ec43 commit 108263d

File tree

3 files changed

+75
-42
lines changed

3 files changed

+75
-42
lines changed

‎app/scripts/app-init.js‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ let scriptsLoadInitiated = false;
66
const { chrome } = globalThis;
77
const testMode = process.env.IN_TEST;
88

9+
/**
10+
* @type {globalThis.stateHooks}
11+
*/
12+
globalThis.stateHooks = globalThis.stateHooks || {};
13+
914
const loadTimeLogs = [];
1015
// eslint-disable-next-line import/unambiguous
1116
function tryImport(...fileNames) {
@@ -184,12 +189,32 @@ const registerInPageContentScript = async () => {
184189
}
185190
};
186191

187-
chrome.runtime.onInstalled.addListener(function (details) {
192+
/**
193+
* `onInstalled` event handler.
194+
*
195+
* On MV3 builds we must listen for this event in `app-init`, otherwise we found that the listener
196+
* is never called.
197+
* For MV2 builds, the listener is added in `background.js` instead.
198+
*
199+
* @param {chrome.runtime.InstalledDetails} details - Event details.
200+
*/
201+
function onInstalledListener(details) {
188202
if (details.reason === 'install') {
189-
chrome.storage.session.set({ isFirstTimeInstall: true });
190-
} else if (details.reason === 'update') {
191-
chrome.storage.session.set({ isFirstTimeInstall: false });
203+
// This condition is for when `background.js` was loaded before the `onInstalled` listener was
204+
// called.
205+
if (globalThis.stateHooks.metamaskTriggerOnInstall) {
206+
globalThis.stateHooks.metamaskTriggerOnInstall();
207+
// Delete just to clean up global namespace
208+
delete globalThis.stateHooks.metamaskTriggerOnInstall;
209+
// This condition is for when the `onInstalled` listener in `app-init` was called before
210+
// `background.js` was loaded.
211+
} else {
212+
globalThis.stateHooks.metamaskWasJustInstalled = true;
213+
}
214+
chrome.runtime.onInstalled.removeListener(onInstalledListener);
192215
}
193-
});
216+
}
217+
218+
chrome.runtime.onInstalled.addListener(onInstalledListener);
194219

195220
registerInPageContentScript();

‎app/scripts/background.js‎

Lines changed: 35 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -141,22 +141,35 @@ const PHISHING_WARNING_PAGE_TIMEOUT = ONE_SECOND_IN_MILLISECONDS;
141141
// Event emitter for state persistence
142142
export const statePersistenceEvents = new EventEmitter();
143143

144-
if (isFirefox) {
145-
browser.runtime.onInstalled.addListener(function (details) {
146-
if (details.reason === 'install') {
147-
browser.storage.session.set({ isFirstTimeInstall: true });
148-
} else if (details.reason === 'update') {
149-
browser.storage.session.set({ isFirstTimeInstall: false });
150-
}
151-
});
152-
} else if (!isManifestV3) {
153-
browser.runtime.onInstalled.addListener(function (details) {
144+
if (!isManifestV3) {
145+
/**
146+
* `onInstalled` event handler.
147+
*
148+
* On MV3 builds we must listen for this event in `app-init`, otherwise we found that the listener
149+
* is never called.
150+
* There is no `app-init` file on MV2 builds, so we add a listener here instead.
151+
*
152+
* @param {import('webextension-polyfill').Runtime.OnInstalledDetailsType} details - Event details.
153+
*/
154+
const onInstalledListener = (details) => {
154155
if (details.reason === 'install') {
155-
global.sessionStorage.setItem('isFirstTimeInstall', true);
156-
} else if (details.reason === 'update') {
157-
global.sessionStorage.setItem('isFirstTimeInstall', false);
156+
onInstall();
157+
browser.runtime.onInstalled.removeListener(onInstalledListener);
158158
}
159-
});
159+
};
160+
161+
browser.runtime.onInstalled.addListener(onInstalledListener);
162+
163+
// This condition is for when the `onInstalled` listener in `app-init` was called before
164+
// `background.js` was loaded.
165+
} else if (globalThis.stateHooks.metamaskWasJustInstalled) {
166+
onInstall();
167+
// Delete just to clean up global namespace
168+
delete globalThis.stateHooks.metamaskWasJustInstalled;
169+
// This condition is for when `background.js` was loaded before the `onInstalled` listener was
170+
// called.
171+
} else {
172+
globalThis.stateHooks.metamaskTriggerOnInstall = () => onInstall();
160173
}
161174

162175
/**
@@ -505,12 +518,6 @@ async function initialize() {
505518
await sendReadyMessageToTabs();
506519
log.info('MetaMask initialization complete.');
507520

508-
if (isManifestV3 || isFirefox) {
509-
browser.storage.session.set({ isFirstTimeInstall: false });
510-
} else {
511-
global.sessionStorage.setItem('isFirstTimeInstall', false);
512-
}
513-
514521
resolveInitialization();
515522
} catch (error) {
516523
rejectInitialization(error);
@@ -1286,24 +1293,15 @@ const addAppInstalledEvent = () => {
12861293
}, 500);
12871294
};
12881295

1289-
// On first install, open a new tab with MetaMask
1290-
async function onInstall() {
1291-
const sessionData =
1292-
isManifestV3 || isFirefox
1293-
? await browser.storage.session.get(['isFirstTimeInstall'])
1294-
: await global.sessionStorage.getItem('isFirstTimeInstall');
1295-
1296-
const isFirstTimeInstall = sessionData?.isFirstTimeInstall;
1297-
1298-
if (process.env.IN_TEST) {
1299-
addAppInstalledEvent();
1300-
} else if (!isFirstTimeInstall && !process.env.METAMASK_DEBUG) {
1301-
// If storeAlreadyExisted is true then this is a fresh installation
1302-
// and an app installed event should be tracked.
1303-
addAppInstalledEvent();
1296+
/**
1297+
* Trigger actions that should happen only upon initial install (e.g. open tab for onboarding).
1298+
*/
1299+
function onInstall() {
1300+
log.debug('First install detected');
1301+
addAppInstalledEvent();
1302+
if (!process.env.IN_TEST && !process.env.METAMASK_DEBUG) {
13041303
platform.openExtensionInBrowser();
13051304
}
1306-
onNavigateToTab();
13071305
}
13081306

13091307
function onNavigateToTab() {
@@ -1334,7 +1332,7 @@ function setupSentryGetStateGlobal(store) {
13341332
}
13351333

13361334
async function initBackground() {
1337-
await onInstall();
1335+
onNavigateToTab();
13381336
try {
13391337
await initialize();
13401338
if (process.env.IN_TEST) {

‎types/global.d.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,16 @@ type StateHooks = {
250250
metamaskGetState?: () => Promise<any>;
251251
throwTestBackgroundError?: (msg?: string) => Promise<void>;
252252
throwTestError?: (msg?: string) => void;
253+
/**
254+
* This is set in `app-init.js` to communicate that MetaMask was just installed, and is read in
255+
* `background.js`.
256+
*/
257+
metamaskWasJustInstalled?: boolean;
258+
/**
259+
* This is set in `background.js` so that `app-init.js` can trigger "on install" actions when
260+
* the `onInstalled` listener is called.
261+
*/
262+
metamaskTriggerOnInstall?: () => void;
253263
};
254264

255265
export declare global {

0 commit comments

Comments
 (0)