Prevent automatic rejection of confirmations (#13194)

* Prevent automatic rejection of confirmations

Confirmations are now only automatically rejected if a user explicitly
closes the notification window. If we close the window programmatically
because there are no notifications left to show, nothing gets rejected.

This partially avoids a race condition where a confirmation gets
rejected automatically without the user having seen the confirmation
first. This could happen if the confirmation was processed just as the
notification window was being closed.

It's still possible for a confirmation that the user has never seen to
get rejected as a result of the user closing the window. But at least
now it's no longer possible for a confirmation to get rejected in this
manner after the user resolves the last confirmation in the queue.

* Fix bug that prevented automatic closure detection

All windows were being detected as explicit window closures,
essentially just as they were previously, because this variable was
cleared too soon.

* Re-open popup when necessary

After the window is automatically closed, a confirmation may have been
queued up while the window was closing. If so, the popup is now re-
opened.

app/scripts/background.js

@@ -218,6 +218,7 @@ function setupController(initState, initLangCode) {
     initLangCode,
     // platform specific api
     platform,
+    notificationManager,
     extension,
     getRequestAccountTabIds: () => {
       return requestAccountTabIds;
@@ -455,6 +456,15 @@ function setupController(initState, initLangCode) {
    */
   function updateBadge() {
     let label = '';
+    const count = getUnapprovedTransactionCount();
+    if (count) {
+      label = String(count);
+    }
+    extension.browserAction.setBadgeText({ text: label });
+    extension.browserAction.setBadgeBackgroundColor({ color: '#037DD6' });
+  }
+
+  function getUnapprovedTransactionCount() {
     const unapprovedTxCount = controller.txController.getUnapprovedTxCount();
     const { unapprovedMsgCount } = controller.messageManager;
     const { unapprovedPersonalMsgCount } = controller.personalMessageManager;
@@ -466,25 +476,27 @@ function setupController(initState, initLangCode) {
     const pendingApprovalCount = controller.approvalController.getTotalApprovalCount();
     const waitingForUnlockCount =
       controller.appStateController.waitingForUnlock.length;
-    const count =
+    return (
       unapprovedTxCount +
       unapprovedMsgCount +
       unapprovedPersonalMsgCount +
       unapprovedDecryptMsgCount +
       unapprovedEncryptionPublicKeyMsgCount +
       unapprovedTypedMessagesCount +
       pendingApprovalCount +
-      waitingForUnlockCount;
-    if (count) {
-      label = String(count);
-    }
-    extension.browserAction.setBadgeText({ text: label });
-    extension.browserAction.setBadgeBackgroundColor({ color: '#037DD6' });
+      waitingForUnlockCount
+    );
   }
 
   notificationManager.on(
     NOTIFICATION_MANAGER_EVENTS.POPUP_CLOSED,
-    rejectUnapprovedNotifications,
+    ({ automaticallyClosed }) => {
+      if (!automaticallyClosed) {
+        rejectUnapprovedNotifications();
+      } else if (getUnapprovedTransactionCount() > 0) {
+        triggerUi();
+      }
+    },
   );
 
   function rejectUnapprovedNotifications() {

app/scripts/lib/notification-manager.js

@@ -22,6 +22,16 @@ export default class NotificationManager extends EventEmitter {
     this.platform.addOnRemovedListener(this._onWindowClosed.bind(this));
   }
 
+  /**
+   * Mark the notification popup as having been automatically closed.
+   *
+   * This lets us differentiate between the cases where we close the
+   * notification popup v.s. when the user closes the popup window directly.
+   */
+  markAsAutomaticallyClosed() {
+    this._popupAutomaticallyClosed = true;
+  }
+
   /**
    * Either brings an existing MetaMask notification window into focus, or creates a new notification window. New
    * notification windows are given a 'popup' type.
@@ -72,7 +82,10 @@ export default class NotificationManager extends EventEmitter {
   _onWindowClosed(windowId) {
     if (windowId === this._popupId) {
       this._popupId = undefined;
-      this.emit(NOTIFICATION_MANAGER_EVENTS.POPUP_CLOSED);
+      this.emit(NOTIFICATION_MANAGER_EVENTS.POPUP_CLOSED, {
+        automaticallyClosed: this._popupAutomaticallyClosed,
+      });
+      this._popupAutomaticallyClosed = undefined;
     }
   }
 

app/scripts/metamask-controller.js

@@ -131,6 +131,7 @@ export default class MetamaskController extends EventEmitter {
     this.opts = opts;
     this.extension = opts.extension;
     this.platform = opts.platform;
+    this.notificationManager = opts.notificationManager;
     const initState = opts.initState || {};
     const version = this.platform.getVersion();
     this.recordFirstTimeInfo(initState);
@@ -1053,6 +1054,8 @@ export default class MetamaskController extends EventEmitter {
       safelistPhishingDomain: this.safelistPhishingDomain.bind(this),
       getRequestAccountTabIds: this.getRequestAccountTabIds,
       getOpenMetamaskTabsIds: this.getOpenMetamaskTabsIds,
+      markNotificationPopupAsAutomaticallyClosed: () =>
+        this.notificationManager.markAsAutomaticallyClosed(),
 
       // primary HD keyring management
       addNewAccount: this.addNewAccount.bind(this),

ui/pages/confirm-add-suggested-token/confirm-add-suggested-token.component.js

@@ -3,8 +3,6 @@ import PropTypes from 'prop-types';
 import Button from '../../components/ui/button';
 import Identicon from '../../components/ui/identicon';
 import TokenBalance from '../../components/ui/token-balance';
-import { getEnvironmentType } from '../../../app/scripts/lib/util';
-import { ENVIRONMENT_TYPE_NOTIFICATION } from '../../../shared/constants/app';
 import { isEqualCaseInsensitive } from '../../helpers/utils/util';
 
 export default class ConfirmAddSuggestedToken extends Component {
@@ -40,11 +38,8 @@ export default class ConfirmAddSuggestedToken extends Component {
     if (suggestedAssets.length > 0) {
       return;
     }
-    if (getEnvironmentType() === ENVIRONMENT_TYPE_NOTIFICATION) {
-      global.platform.closeCurrentWindow();
-    } else {
-      history.push(mostRecentOverviewPage);
-    }
+
+    history.push(mostRecentOverviewPage);
   }
 
   getTokenName(name, symbol) {

ui/pages/home/home.component.js

@@ -46,6 +46,18 @@ const LEGACY_WEB3_URL =
 const INFURA_BLOCKAGE_URL =
   'https://metamask.zendesk.com/hc/en-us/articles/360059386712';
 
+function shouldCloseNotificationPopup({
+  isNotification,
+  totalUnapprovedCount,
+  isSigningQRHardwareTransaction,
+}) {
+  return (
+    isNotification &&
+    totalUnapprovedCount === 0 &&
+    !isSigningQRHardwareTransaction
+  );
+}
+
 export default class Home extends PureComponent {
   static contextTypes = {
     t: PropTypes.func,
@@ -68,6 +80,8 @@ export default class Home extends PureComponent {
     setShowRestorePromptToFalse: PropTypes.func,
     threeBoxLastUpdated: PropTypes.number,
     firstPermissionsRequestId: PropTypes.string,
+    // This prop is used in the `shouldCloseNotificationPopup` function
+    // eslint-disable-next-line react/no-unused-prop-types
     totalUnapprovedCount: PropTypes.number.isRequired,
     setConnectedStatusPopoverHasBeenShown: PropTypes.func,
     connectedStatusPopoverHasBeenShown: PropTypes.bool,
@@ -91,39 +105,37 @@ export default class Home extends PureComponent {
     seedPhraseBackedUp: PropTypes.bool.isRequired,
     newNetworkAdded: PropTypes.string,
     setNewNetworkAdded: PropTypes.func.isRequired,
+    // This prop is used in the `shouldCloseNotificationPopup` function
+    // eslint-disable-next-line react/no-unused-prop-types
     isSigningQRHardwareTransaction: PropTypes.bool.isRequired,
     newCollectibleAddedMessage: PropTypes.string,
     setNewCollectibleAddedMessage: PropTypes.func.isRequired,
+    closeNotificationPopup: PropTypes.func.isRequired,
   };
 
   state = {
     canShowBlockageNotification: true,
-    closing: false,
+    notificationClosing: false,
     redirecting: false,
   };
 
   constructor(props) {
     super(props);
 
     const {
+      closeNotificationPopup,
       firstPermissionsRequestId,
       haveSwapsQuotes,
       isNotification,
-      isSigningQRHardwareTransaction,
       showAwaitingSwapScreen,
       suggestedAssets = [],
       swapsFetchParams,
-      totalUnapprovedCount,
       unconfirmedTransactionsCount,
     } = this.props;
 
-    if (
-      isNotification &&
-      totalUnapprovedCount === 0 &&
-      !isSigningQRHardwareTransaction
-    ) {
-      this.state.closing = true;
-      global.platform.closeCurrentWindow();
+    if (shouldCloseNotificationPopup(props)) {
+      this.state.notificationClosing = true;
+      closeNotificationPopup();
     } else if (
       firstPermissionsRequestId ||
       unconfirmedTransactionsCount > 0 ||
@@ -141,21 +153,13 @@ export default class Home extends PureComponent {
       history,
       isNotification,
       suggestedAssets = [],
-      totalUnapprovedCount,
       unconfirmedTransactionsCount,
       haveSwapsQuotes,
       showAwaitingSwapScreen,
       swapsFetchParams,
       pendingConfirmations,
-      isSigningQRHardwareTransaction,
     } = this.props;
-    if (
-      isNotification &&
-      totalUnapprovedCount === 0 &&
-      !isSigningQRHardwareTransaction
-    ) {
-      global.platform.closeCurrentWindow();
-    } else if (!isNotification && showAwaitingSwapScreen) {
+    if (!isNotification && showAwaitingSwapScreen) {
       history.push(AWAITING_SWAP_ROUTE);
     } else if (!isNotification && haveSwapsQuotes) {
       history.push(VIEW_QUOTE_ROUTE);
@@ -176,18 +180,33 @@ export default class Home extends PureComponent {
     this.checkStatusAndNavigate();
   }
 
-  componentDidUpdate() {
+  static getDerivedStateFromProps(props) {
+    if (shouldCloseNotificationPopup(props)) {
+      return { notificationClosing: true };
+    }
+    return null;
+  }
+
+  componentDidUpdate(_prevProps, prevState) {
     const {
+      closeNotificationPopup,
       setupThreeBox,
       showRestorePrompt,
       threeBoxLastUpdated,
       threeBoxSynced,
       isNotification,
     } = this.props;
+    const { notificationClosing } = this.state;
 
-    isNotification && this.checkStatusAndNavigate();
-
-    if (threeBoxSynced && showRestorePrompt && threeBoxLastUpdated === null) {
+    if (notificationClosing && !prevState.notificationClosing) {
+      closeNotificationPopup();
+    } else if (isNotification) {
+      this.checkStatusAndNavigate();
+    } else if (
+      threeBoxSynced &&
+      showRestorePrompt &&
+      threeBoxLastUpdated === null
+    ) {
       setupThreeBox();
     }
   }
@@ -428,7 +447,7 @@ export default class Home extends PureComponent {
 
     if (forgottenPassword) {
       return <Redirect to={{ pathname: RESTORE_VAULT_ROUTE }} />;
-    } else if (this.state.closing || this.state.redirecting) {
+    } else if (this.state.notificationClosing || this.state.redirecting) {
       return null;
     }
 

ui/pages/home/home.container.js

@@ -22,6 +22,7 @@ import {
 } from '../../selectors';
 
 import {
+  closeNotificationPopup,
   restoreFromThreeBox,
   turnThreeBoxSyncingOn,
   getThreeBoxLastUpdated,
@@ -127,6 +128,7 @@ const mapStateToProps = (state) => {
 };
 
 const mapDispatchToProps = (dispatch) => ({
+  closeNotificationPopup: () => closeNotificationPopup(),
   turnThreeBoxSyncingOn: () => dispatch(turnThreeBoxSyncingOn()),
   setupThreeBox: () => {
     dispatch(getThreeBoxLastUpdated()).then((lastUpdated) => {

ui/store/actions.js

@@ -955,7 +955,7 @@ export function cancelTxs(txDataList) {
       });
     } finally {
       if (getEnvironmentType() === ENVIRONMENT_TYPE_NOTIFICATION) {
-        global.platform.closeCurrentWindow();
+        closeNotificationPopup();
       } else {
         dispatch(hideLoadingIndication());
       }
@@ -1775,7 +1775,7 @@ export function closeCurrentNotificationWindow() {
       getEnvironmentType() === ENVIRONMENT_TYPE_NOTIFICATION &&
       !hasUnconfirmedTransactions(getState())
     ) {
-      global.platform.closeCurrentWindow();
+      closeNotificationPopup();
     }
   };
 }
@@ -3007,6 +3007,11 @@ export function getGasFeeTimeEstimate(maxPriorityFeePerGas, maxFeePerGas) {
   );
 }
 
+export async function closeNotificationPopup() {
+  await promisifiedBackground.markNotificationPopupAsAutomaticallyClosed();
+  global.platform.closeCurrentWindow();
+}
+
 // MetaMetrics
 /**
  * @typedef {import('../../shared/constants/metametrics').MetaMetricsEventPayload} MetaMetricsEventPayload