Cleanup beforeunload handler after transaction is resolved

The notification window was updated to reject transactions upon close in #6340. A handler that rejects the transaction was added to `window.onbeforeunload`, and it was cleared in `actions.js` if it was confirmed or rejected. However, the `onbeforeunload` handler remained uncleared if the transaction was resolved in another window. This results in the transaction being rejected when the notification window closes, even long after the transaction is submitted and confirmed. This has been the cause of many problems with the Firefox e2e tests. Instead the `onbeforeunload` handler is cleared in the `componentWillUnmount` lifecycle function, alongside where it's set in the first place. This ensures that it's correctly unset regardless of how the transaction was resolved, and it better matches user expectations.
MetaMask · Oct 31, 2019 · 48277c2 · 48277c2
1 parent ab0eae1
commit 48277c2
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 15 deletions.
diff --git a/ui/app/components/app/signature-request.js b/ui/app/components/app/signature-request.js
@@ -107,7 +107,7 @@ SignatureRequest.prototype.componentDidMount = function () {
   const { clearConfirmTransaction, cancel } = this.props
   const { metricsEvent } = this.context
   if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
-    window.onbeforeunload = event => {
+    this._onBeforeUnload = event => {
       metricsEvent({
         eventOpts: {
           category: 'Transactions',
@@ -118,6 +118,13 @@ SignatureRequest.prototype.componentDidMount = function () {
       clearConfirmTransaction()
       cancel(event)
     }
+    window.addEventListener('beforeunload', this._onBeforeUnload)
+  }
+}
+
+SignatureRequest.prototype.componentWillUnmount = function () {
+  if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
+    window.removeEventListener('beforeunload', this._onBeforeUnload)
   }
 }
 

diff --git a/ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js b/ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js
@@ -581,7 +581,7 @@ export default class ConfirmTransactionBase extends Component {
     })
 
     if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
-      window.onbeforeunload = () => {
+      this._onBeforeUnload = () => {
         metricsEvent({
           eventOpts: {
             category: 'Transactions',
@@ -594,11 +594,18 @@ export default class ConfirmTransactionBase extends Component {
         })
         cancelTransaction({ id })
       }
+      window.addEventListener('beforeunload', this._onBeforeUnload)
     }
 
     getNextNonce()
   }
 
+  componentWillUnmount () {
+    if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
+      window.removeEventListener('beforeunload', this._onBeforeUnload)
+    }
+  }
+
   render () {
     const {
       isTxReprice,

diff --git a/...st-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.component.js b/...st-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.component.js
@@ -50,7 +50,7 @@ export default class ImportWithSeedPhrase extends PureComponent {
   }
 
   componentWillMount () {
-    window.onbeforeunload = () => this.context.metricsEvent({
+    this._onBeforeUnload = () => this.context.metricsEvent({
       eventOpts: {
         category: 'Onboarding',
         action: 'Import Seed Phrase',
@@ -61,6 +61,11 @@ export default class ImportWithSeedPhrase extends PureComponent {
         errorMessage: this.state.seedPhraseError,
       },
     })
+    window.addEventListener('beforeunload', this._onBeforeUnload)
+  }
+
+  componentWillUnmount () {
+    window.removeEventListener('beforeunload', this._onBeforeUnload)
   }
 
   handleSeedPhraseChange (seedPhrase) {

diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js
@@ -856,8 +856,6 @@ function signMsg (msgData) {
   log.debug('action - signMsg')
   return (dispatch) => {
     dispatch(actions.showLoadingIndication())
-    window.onbeforeunload = null
-
     return new Promise((resolve, reject) => {
       log.debug(`actions calling background.signMessage`)
       background.signMessage(msgData, (err, newState) => {
@@ -884,7 +882,6 @@ function signPersonalMsg (msgData) {
   log.debug('action - signPersonalMsg')
   return (dispatch) => {
     dispatch(actions.showLoadingIndication())
-    window.onbeforeunload = null
     return new Promise((resolve, reject) => {
       log.debug(`actions calling background.signPersonalMessage`)
       background.signPersonalMessage(msgData, (err, newState) => {
@@ -911,7 +908,6 @@ function signTypedMsg (msgData) {
   log.debug('action - signTypedMsg')
   return (dispatch) => {
     dispatch(actions.showLoadingIndication())
-    window.onbeforeunload = null
     return new Promise((resolve, reject) => {
       log.debug(`actions calling background.signTypedMessage`)
       background.signTypedMessage(msgData, (err, newState) => {
@@ -1124,7 +1120,6 @@ function sendTx (txData) {
   log.info(`actions - sendTx: ${JSON.stringify(txData.txParams)}`)
   return (dispatch, getState) => {
     log.debug(`actions calling background.approveTransaction`)
-    window.onbeforeunload = null
     background.approveTransaction(txData.id, (err) => {
       if (err) {
         dispatch(actions.txError(err))
@@ -1201,7 +1196,6 @@ function updateAndApproveTx (txData) {
   return (dispatch) => {
     log.debug(`actions calling background.updateAndApproveTx`)
     dispatch(actions.showLoadingIndication())
-    window.onbeforeunload = null
     return new Promise((resolve, reject) => {
       background.updateAndApproveTransaction(txData, err => {
         dispatch(actions.updateTransactionParams(txData.id, txData.txParams))
@@ -1260,7 +1254,6 @@ function txError (err) {
 function cancelMsg (msgData) {
   return (dispatch) => {
     dispatch(actions.showLoadingIndication())
-    window.onbeforeunload = null
     return new Promise((resolve, reject) => {
       log.debug(`background.cancelMessage`)
       background.cancelMessage(msgData.id, (err, newState) => {
@@ -1283,7 +1276,6 @@ function cancelMsg (msgData) {
 function cancelPersonalMsg (msgData) {
   return (dispatch) => {
     dispatch(actions.showLoadingIndication())
-    window.onbeforeunload = null
     return new Promise((resolve, reject) => {
       const id = msgData.id
       background.cancelPersonalMessage(id, (err, newState) => {
@@ -1306,7 +1298,6 @@ function cancelPersonalMsg (msgData) {
 function cancelTypedMsg (msgData) {
   return (dispatch) => {
     dispatch(actions.showLoadingIndication())
-    window.onbeforeunload = null
     return new Promise((resolve, reject) => {
       const id = msgData.id
       background.cancelTypedMessage(id, (err, newState) => {
@@ -1330,7 +1321,6 @@ function cancelTx (txData) {
   return (dispatch) => {
     log.debug(`background.cancelTransaction`)
     dispatch(actions.showLoadingIndication())
-    window.onbeforeunload = null
     return new Promise((resolve, reject) => {
       background.cancelTransaction(txData.id, err => {
         if (err) {
@@ -1360,7 +1350,6 @@ function cancelTx (txData) {
  */
 function cancelTxs (txDataList) {
   return async (dispatch) => {
-    window.onbeforeunload = null
     dispatch(actions.showLoadingIndication())
     const txIds = txDataList.map(({id}) => id)
     const cancellations = txIds.map((id) => new Promise((resolve, reject) => {
@@ -1744,7 +1733,6 @@ function addTokens (tokens) {
 function removeSuggestedTokens () {
   return (dispatch) => {
     dispatch(actions.showLoadingIndication())
-    window.onbeforeunload = null
     return new Promise((resolve) => {
       background.removeSuggestedTokens((err, suggestedTokens) => {
         dispatch(actions.hideLoadingIndication())