From d359429f0473c3e73a6d14bf8d7b37cb301865cc Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Wed, 4 Aug 2021 16:53:13 -0500 Subject: [PATCH] Stop GasFeeController polling when pop closes (#11746) * Stop GasFeeController polling when pop closes * Stop estimate gas polling on window unload * lint + comments * Improve client closed logic * lint * Add back _beforeUnload on unmount in gas-modal-page-container * Add full check and call onClientClosed method for notifcation environment * Add gas pollingToken tracking to appStateController and use to disconnect polling for each environment type * remove unused method * move controller manipulation logic from background.js to metamask-controller, disaggregate methods * add beforeunload handling to reset gas polling tokens from root of send page * cleanup, lint and address feedback * clear appState gasPollingTokens when all instances of all env types are closed, fix pollingTokenType arg from onEnvironmentTypeClosed call in metamask-controller * mock new methods to fix tests * final bit of cleanup + comments Co-authored-by: Dan Miller --- app/scripts/background.js | 37 ++++++++++++++-- app/scripts/controllers/app-state.js | 37 ++++++++++++++++ app/scripts/metamask-controller.js | 44 ++++++++++++++++++- shared/constants/app.js | 6 +++ ...gas-modal-page-container-component.test.js | 1 + .../gas-modal-page-container.component.js | 13 +++++- ui/ducks/send/send.js | 7 ++- ui/hooks/useGasFeeEstimates.test.js | 2 + ui/hooks/useSafeGasEstimatePolling.js | 30 +++++++++---- .../confirm-transaction-base.component.js | 19 ++++++-- .../confirm-transaction.component.js | 20 +++++++-- ui/pages/send/send.js | 13 ++++-- ui/store/actions.js | 19 +++++++- 13 files changed, 221 insertions(+), 27 deletions(-) diff --git a/app/scripts/background.js b/app/scripts/background.js index bf7dd24cc..56044213c 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -302,6 +302,24 @@ function setupController(initState, initLangCode) { ); }; + const onCloseEnvironmentInstances = (isClientOpen, environmentType) => { + // if all instances of metamask are closed we call a method on the controller to stop gasFeeController polling + if (isClientOpen === false) { + controller.onClientClosed(); + // otherwise we want to only remove the polling tokens for the environment type that has closed + } else { + // in the case of fullscreen environment a user might have multiple tabs open so we don't want to disconnect all of + // its corresponding polling tokens unless all tabs are closed. + if ( + environmentType === ENVIRONMENT_TYPE_FULLSCREEN && + Boolean(Object.keys(openMetamaskTabsIDs).length) + ) { + return; + } + controller.onEnvironmentTypeClosed(environmentType); + } + }; + /** * A runtime.Port object, as provided by the browser: * @see https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/Port @@ -330,10 +348,11 @@ function setupController(initState, initLangCode) { if (processName === ENVIRONMENT_TYPE_POPUP) { popupIsOpen = true; - endOfStream(portStream, () => { popupIsOpen = false; - controller.isClientOpen = isClientOpenStatus(); + const isClientOpen = isClientOpenStatus(); + controller.isClientOpen = isClientOpen; + onCloseEnvironmentInstances(isClientOpen, ENVIRONMENT_TYPE_POPUP); }); } @@ -342,7 +361,12 @@ function setupController(initState, initLangCode) { endOfStream(portStream, () => { notificationIsOpen = false; - controller.isClientOpen = isClientOpenStatus(); + const isClientOpen = isClientOpenStatus(); + controller.isClientOpen = isClientOpen; + onCloseEnvironmentInstances( + isClientOpen, + ENVIRONMENT_TYPE_NOTIFICATION, + ); }); } @@ -352,7 +376,12 @@ function setupController(initState, initLangCode) { endOfStream(portStream, () => { delete openMetamaskTabsIDs[tabId]; - controller.isClientOpen = isClientOpenStatus(); + const isClientOpen = isClientOpenStatus(); + controller.isClientOpen = isClientOpen; + onCloseEnvironmentInstances( + isClientOpen, + ENVIRONMENT_TYPE_FULLSCREEN, + ); }); } } else { diff --git a/app/scripts/controllers/app-state.js b/app/scripts/controllers/app-state.js index 9916e5d4f..84ee14394 100644 --- a/app/scripts/controllers/app-state.js +++ b/app/scripts/controllers/app-state.js @@ -25,6 +25,9 @@ export default class AppStateController extends EventEmitter { connectedStatusPopoverHasBeenShown: true, defaultHomeActiveTabName: null, browserEnvironment: {}, + popupGasPollTokens: [], + notificationGasPollTokens: [], + fullScreenGasPollTokens: [], recoveryPhraseReminderHasBeenShown: false, recoveryPhraseReminderLastShown: new Date().getTime(), ...initState, @@ -191,4 +194,38 @@ export default class AppStateController extends EventEmitter { setBrowserEnvironment(os, browser) { this.store.updateState({ browserEnvironment: { os, browser } }); } + + /** + * Adds a pollingToken for a given environmentType + * @returns {void} + */ + addPollingToken(pollingToken, pollingTokenType) { + const prevState = this.store.getState()[pollingTokenType]; + this.store.updateState({ + [pollingTokenType]: [...prevState, pollingToken], + }); + } + + /** + * removes a pollingToken for a given environmentType + * @returns {void} + */ + removePollingToken(pollingToken, pollingTokenType) { + const prevState = this.store.getState()[pollingTokenType]; + this.store.updateState({ + [pollingTokenType]: prevState.filter((token) => token !== pollingToken), + }); + } + + /** + * clears all pollingTokens + * @returns {void} + */ + clearPollingTokens() { + this.store.updateState({ + popupGasPollTokens: [], + notificationGasPollTokens: [], + fullScreenGasPollTokens: [], + }); + } } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 99eff2ab3..f1c2cba85 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -32,6 +32,7 @@ import { MAINNET_CHAIN_ID } from '../../shared/constants/network'; import { UI_NOTIFICATIONS } from '../../shared/notifications'; import { toChecksumHexAddress } from '../../shared/modules/hexstring-utils'; import { MILLISECOND } from '../../shared/constants/time'; +import { POLLING_TOKEN_ENVIRONMENT_TYPES } from '../../shared/constants/app'; import { hexToDecimal } from '../../ui/helpers/utils/conversions.util'; import ComposableObservableStore from './lib/ComposableObservableStore'; @@ -1130,6 +1131,16 @@ export default class MetamaskController extends EventEmitter { this.gasFeeController.getTimeEstimate, this.gasFeeController, ), + + addPollingTokenToAppState: nodeify( + this.appStateController.addPollingToken, + this.appStateController, + ), + + removePollingTokenFromAppState: nodeify( + this.appStateController.removePollingToken, + this.appStateController, + ), }; } @@ -2969,7 +2980,6 @@ export default class MetamaskController extends EventEmitter { /* eslint-disable accessor-pairs */ /** * A method for recording whether the MetaMask user interface is open or not. - * @private * @param {boolean} open */ set isClientOpen(open) { @@ -2978,6 +2988,38 @@ export default class MetamaskController extends EventEmitter { } /* eslint-enable accessor-pairs */ + /** + * A method that is called by the background when all instances of metamask are closed. + * Currently used to stop polling in the gasFeeController. + */ + onClientClosed() { + try { + this.gasFeeController.stopPolling(); + this.appStateController.clearPollingTokens(); + } catch (error) { + console.error(error); + } + } + + /** + * A method that is called by the background when a particular environment type is closed (fullscreen, popup, notification). + * Currently used to stop polling in the gasFeeController for only that environement type + */ + onEnvironmentTypeClosed(environmentType) { + const appStatePollingTokenType = + POLLING_TOKEN_ENVIRONMENT_TYPES[environmentType]; + const pollingTokensToDisconnect = this.appStateController.store.getState()[ + appStatePollingTokenType + ]; + pollingTokensToDisconnect.forEach((pollingToken) => { + this.gasFeeController.disconnectPoller(pollingToken); + this.appStateController.removePollingToken( + pollingToken, + appStatePollingTokenType, + ); + }); + } + /** * Adds a domain to the PhishingController safelist * @param {string} hostname - the domain to safelist diff --git a/shared/constants/app.js b/shared/constants/app.js index 7e1b66572..a278a14bd 100644 --- a/shared/constants/app.js +++ b/shared/constants/app.js @@ -30,3 +30,9 @@ export const MESSAGE_TYPE = { ADD_ETHEREUM_CHAIN: 'wallet_addEthereumChain', SWITCH_ETHEREUM_CHAIN: 'wallet_switchEthereumChain', }; + +export const POLLING_TOKEN_ENVIRONMENT_TYPES = { + [ENVIRONMENT_TYPE_POPUP]: 'popupGasPollTokens', + [ENVIRONMENT_TYPE_NOTIFICATION]: 'notificationGasPollTokens', + [ENVIRONMENT_TYPE_FULLSCREEN]: 'fullScreenGasPollTokens', +}; diff --git a/ui/components/app/gas-customization/gas-modal-page-container/gas-modal-page-container-component.test.js b/ui/components/app/gas-customization/gas-modal-page-container/gas-modal-page-container-component.test.js index 5b8f5c665..185f1b3a7 100644 --- a/ui/components/app/gas-customization/gas-modal-page-container/gas-modal-page-container-component.test.js +++ b/ui/components/app/gas-customization/gas-modal-page-container/gas-modal-page-container-component.test.js @@ -13,6 +13,7 @@ jest.mock('../../../../store/actions', () => ({ getGasFeeEstimatesAndStartPolling: jest .fn() .mockImplementation(() => Promise.resolve()), + addPollingTokenToAppState: jest.fn(), })); const propsMethodSpies = { diff --git a/ui/components/app/gas-customization/gas-modal-page-container/gas-modal-page-container.component.js b/ui/components/app/gas-customization/gas-modal-page-container/gas-modal-page-container.component.js index aaa28a056..9bf9ccfa9 100644 --- a/ui/components/app/gas-customization/gas-modal-page-container/gas-modal-page-container.component.js +++ b/ui/components/app/gas-customization/gas-modal-page-container/gas-modal-page-container.component.js @@ -5,6 +5,8 @@ import { Tabs, Tab } from '../../../ui/tabs'; import { disconnectGasFeeEstimatePoller, getGasFeeEstimatesAndStartPolling, + addPollingTokenToAppState, + removePollingTokenFromAppState, } from '../../../../store/actions'; import AdvancedTabContent from './advanced-tab-content'; import BasicTabContent from './basic-tab-content'; @@ -52,18 +54,27 @@ export default class GasModalPageContainer extends Component { this._isMounted = true; getGasFeeEstimatesAndStartPolling().then((pollingToken) => { if (this._isMounted) { + addPollingTokenToAppState(pollingToken); this.setState({ pollingToken }); } else { disconnectGasFeeEstimatePoller(pollingToken); + removePollingTokenFromAppState(pollingToken); } }); + window.addEventListener('beforeunload', this._beforeUnload); } - componentWillUnmount() { + _beforeUnload = () => { this._isMounted = false; if (this.state.pollingToken) { disconnectGasFeeEstimatePoller(this.state.pollingToken); + removePollingTokenFromAppState(this.state.pollingToken); } + }; + + componentWillUnmount() { + this._beforeUnload(); + window.removeEventListener('beforeunload', this._beforeUnload); } renderBasicTabContent(gasPriceButtonGroupProps) { diff --git a/ui/ducks/send/send.js b/ui/ducks/send/send.js index 5c9badd91..2d58b7137 100644 --- a/ui/ducks/send/send.js +++ b/ui/ducks/send/send.js @@ -50,6 +50,8 @@ import { showLoadingIndication, updateTokenType, updateTransaction, + addPollingTokenToAppState, + removePollingTokenFromAppState, } from '../../store/actions'; import { setCustomGasLimit } from '../gas/gas.duck'; import { @@ -84,7 +86,6 @@ import { import { CHAIN_ID_TO_GAS_LIMIT_BUFFER_MAP } from '../../../shared/constants/network'; import { ETH, GWEI } from '../../helpers/constants/common'; import { TRANSACTION_ENVELOPE_TYPES } from '../../../shared/constants/transaction'; - // typedefs /** * @typedef {import('@reduxjs/toolkit').PayloadAction} PayloadAction @@ -437,6 +438,9 @@ export const initializeSendState = createAsyncThunk( // Instruct the background process that polling for gas prices should begin gasEstimatePollToken = await getGasFeeEstimatesAndStartPolling(); + + addPollingTokenToAppState(gasEstimatePollToken); + const { metamask: { gasFeeEstimates, gasEstimateType }, } = thunkApi.getState(); @@ -1298,6 +1302,7 @@ export function resetSendState() { await disconnectGasFeeEstimatePoller( state[name].gas.gasEstimatePollToken, ); + removePollingTokenFromAppState(state[name].gas.gasEstimatePollToken); } }; } diff --git a/ui/hooks/useGasFeeEstimates.test.js b/ui/hooks/useGasFeeEstimates.test.js index 030806667..0af584e1c 100644 --- a/ui/hooks/useGasFeeEstimates.test.js +++ b/ui/hooks/useGasFeeEstimates.test.js @@ -16,6 +16,8 @@ import { useGasFeeEstimates } from './useGasFeeEstimates'; jest.mock('../store/actions', () => ({ disconnectGasFeeEstimatePoller: jest.fn(), getGasFeeEstimatesAndStartPolling: jest.fn(), + addPollingTokenToAppState: jest.fn(), + removePollingTokenFromAppState: jest.fn(), })); jest.mock('react-redux', () => { diff --git a/ui/hooks/useSafeGasEstimatePolling.js b/ui/hooks/useSafeGasEstimatePolling.js index a5b803eb7..156f805e6 100644 --- a/ui/hooks/useSafeGasEstimatePolling.js +++ b/ui/hooks/useSafeGasEstimatePolling.js @@ -2,6 +2,8 @@ import { useEffect } from 'react'; import { disconnectGasFeeEstimatePoller, getGasFeeEstimatesAndStartPolling, + addPollingTokenToAppState, + removePollingTokenFromAppState, } from '../store/actions'; /** @@ -16,18 +18,30 @@ export function useSafeGasEstimatePolling() { useEffect(() => { let active = true; let pollToken; - getGasFeeEstimatesAndStartPolling().then((newPollToken) => { - if (active) { - pollToken = newPollToken; - } else { - disconnectGasFeeEstimatePoller(newPollToken); - } - }); - return () => { + + const cleanup = () => { active = false; if (pollToken) { disconnectGasFeeEstimatePoller(pollToken); + removePollingTokenFromAppState(pollToken); } }; + + getGasFeeEstimatesAndStartPolling().then((newPollToken) => { + if (active) { + pollToken = newPollToken; + addPollingTokenToAppState(pollToken); + } else { + disconnectGasFeeEstimatePoller(newPollToken); + removePollingTokenFromAppState(pollToken); + } + }); + + window.addEventListener('beforeunload', cleanup); + + return () => { + cleanup(); + window.removeEventListener('beforeunload', cleanup); + }; }, []); } diff --git a/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js index f162b1706..164573169 100644 --- a/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js @@ -40,6 +40,8 @@ import { COLORS } from '../../helpers/constants/design-system'; import { disconnectGasFeeEstimatePoller, getGasFeeEstimatesAndStartPolling, + addPollingTokenToAppState, + removePollingTokenFromAppState, } from '../../store/actions'; export default class ConfirmTransactionBase extends Component { @@ -679,10 +681,19 @@ export default class ConfirmTransactionBase extends Component { cancelTransaction({ id }); }; + _beforeUnloadForGasPolling = () => { + this._isMounted = false; + if (this.state.pollingToken) { + disconnectGasFeeEstimatePoller(this.state.pollingToken); + removePollingTokenFromAppState(this.state.pollingToken); + } + }; + _removeBeforeUnload = () => { if (getEnvironmentType() === ENVIRONMENT_TYPE_NOTIFICATION) { window.removeEventListener('beforeunload', this._beforeUnload); } + window.removeEventListener('beforeunload', this._beforeUnloadForGasPolling); }; componentDidMount() { @@ -723,18 +734,18 @@ export default class ConfirmTransactionBase extends Component { */ getGasFeeEstimatesAndStartPolling().then((pollingToken) => { if (this._isMounted) { + addPollingTokenToAppState(pollingToken); this.setState({ pollingToken }); } else { disconnectGasFeeEstimatePoller(pollingToken); + removePollingTokenFromAppState(this.state.pollingToken); } }); + window.addEventListener('beforeunload', this._beforeUnloadForGasPolling); } componentWillUnmount() { - this._isMounted = false; - if (this.state.pollingToken) { - disconnectGasFeeEstimatePoller(this.state.pollingToken); - } + this._beforeUnloadForGasPolling(); this._removeBeforeUnload(); } diff --git a/ui/pages/confirm-transaction/confirm-transaction.component.js b/ui/pages/confirm-transaction/confirm-transaction.component.js index 2988739e6..d28904544 100644 --- a/ui/pages/confirm-transaction/confirm-transaction.component.js +++ b/ui/pages/confirm-transaction/confirm-transaction.component.js @@ -28,6 +28,8 @@ import { import { disconnectGasFeeEstimatePoller, getGasFeeEstimatesAndStartPolling, + addPollingTokenToAppState, + removePollingTokenFromAppState, } from '../../store/actions'; import ConfTx from './conf-tx'; @@ -57,6 +59,14 @@ export default class ConfirmTransaction extends Component { this.state = {}; } + _beforeUnload = () => { + this._isMounted = false; + if (this.state.pollingToken) { + disconnectGasFeeEstimatePoller(this.state.pollingToken); + removePollingTokenFromAppState(this.state.pollingToken); + } + }; + componentDidMount() { this._isMounted = true; const { @@ -75,11 +85,15 @@ export default class ConfirmTransaction extends Component { getGasFeeEstimatesAndStartPolling().then((pollingToken) => { if (this._isMounted) { this.setState({ pollingToken }); + addPollingTokenToAppState(pollingToken); } else { disconnectGasFeeEstimatePoller(pollingToken); + removePollingTokenFromAppState(pollingToken); } }); + window.addEventListener('beforeunload', this._beforeUnload); + if (!totalUnapprovedCount && !sendTo) { history.replace(mostRecentOverviewPage); return; @@ -96,10 +110,8 @@ export default class ConfirmTransaction extends Component { } componentWillUnmount() { - this._isMounted = false; - if (this.state.pollingToken) { - disconnectGasFeeEstimatePoller(this.state.pollingToken); - } + this._beforeUnload(); + window.removeEventListener('beforeunload', this._beforeUnload); } componentDidUpdate(prevProps) { diff --git a/ui/pages/send/send.js b/ui/pages/send/send.js index 1e908d9de..3c5515867 100644 --- a/ui/pages/send/send.js +++ b/ui/pages/send/send.js @@ -1,4 +1,4 @@ -import React, { useEffect } from 'react'; +import React, { useEffect, useCallback } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { useHistory, useLocation } from 'react-router-dom'; import { @@ -47,11 +47,17 @@ export default function SendTransactionScreen() { }); const dispatch = useDispatch(); + + const cleanup = useCallback(() => { + dispatch(resetSendState()); + }, [dispatch]); + useEffect(() => { if (chainId !== undefined) { dispatch(initializeSendState()); + window.addEventListener('beforeunload', cleanup); } - }, [chainId, dispatch]); + }, [chainId, dispatch, cleanup]); useEffect(() => { if (location.search === '?scan=true') { @@ -67,8 +73,9 @@ export default function SendTransactionScreen() { useEffect(() => { return () => { dispatch(resetSendState()); + window.removeEventListener('beforeunload', cleanup); }; - }, [dispatch]); + }, [dispatch, cleanup]); let content; diff --git a/ui/store/actions.js b/ui/store/actions.js index 64fe017fb..05001e99e 100644 --- a/ui/store/actions.js +++ b/ui/store/actions.js @@ -10,7 +10,10 @@ import { import { getMethodDataAsync } from '../helpers/utils/transactions.util'; import { getSymbolAndDecimals } from '../helpers/utils/token-util'; import switchDirection from '../helpers/utils/switch-direction'; -import { ENVIRONMENT_TYPE_NOTIFICATION } from '../../shared/constants/app'; +import { + ENVIRONMENT_TYPE_NOTIFICATION, + POLLING_TOKEN_ENVIRONMENT_TYPES, +} from '../../shared/constants/app'; import { hasUnconfirmedTransactions } from '../helpers/utils/confirm-tx.util'; import txHelper from '../helpers/utils/tx-helper'; import { getEnvironmentType, addHexPrefix } from '../../app/scripts/lib/util'; @@ -2787,6 +2790,20 @@ export function disconnectGasFeeEstimatePoller(pollToken) { return promisifiedBackground.disconnectGasFeeEstimatePoller(pollToken); } +export async function addPollingTokenToAppState(pollingToken) { + return promisifiedBackground.addPollingTokenToAppState( + pollingToken, + POLLING_TOKEN_ENVIRONMENT_TYPES[getEnvironmentType()], + ); +} + +export async function removePollingTokenFromAppState(pollingToken) { + return promisifiedBackground.removePollingTokenFromAppState( + pollingToken, + POLLING_TOKEN_ENVIRONMENT_TYPES[getEnvironmentType()], + ); +} + export function getGasFeeTimeEstimate(maxPriorityFeePerGas, maxFeePerGas) { return promisifiedBackground.getGasFeeTimeEstimate( maxPriorityFeePerGas,