From dbd14d796c4cc1f0d42fbe58a096c3e41d5590a5 Mon Sep 17 00:00:00 2001
From: Mark Stacey <markjstacey@gmail.com>
Date: Mon, 4 Nov 2019 14:03:57 -0400
Subject: [PATCH] Clear `beforeunload` handler after button is pressed (#7346)

On the signature request and transaction confirmation notification
pages, the closure of the notification UI implies that the request has
been rejected. However, this rejection is being submitted even when the
window is closed as a result of the user explicitly confirming or
rejecting. In practice, I suspect this has no effect because the
transaction, after being explicitly confirmed or rejected, has already
been moved out of a pending state. But just in case there is some
present or future edge case that might be affected, the `beforeunload`
handler is now removed once the user has explicitly made a choice.

This mistake was introduced recently in #7333
---
 .../app/signature-request-original.js         | 37 ++++++++------
 .../confirm-transaction-base.component.js     | 48 +++++++++++--------
 2 files changed, 51 insertions(+), 34 deletions(-)

diff --git a/ui/app/components/app/signature-request-original.js b/ui/app/components/app/signature-request-original.js
index 0a9a435930f8..60b910eb0700 100644
--- a/ui/app/components/app/signature-request-original.js
+++ b/ui/app/components/app/signature-request-original.js
@@ -103,31 +103,36 @@ function SignatureRequest (props) {
   }
 }
 
-SignatureRequest.prototype.componentDidMount = function () {
+SignatureRequest.prototype._beforeUnload = (event) => {
   const { clearConfirmTransaction, cancel } = this.props
   const { metricsEvent } = this.context
+  metricsEvent({
+    eventOpts: {
+      category: 'Transactions',
+      action: 'Sign Request',
+      name: 'Cancel Sig Request Via Notification Close',
+    },
+  })
+  clearConfirmTransaction()
+  cancel(event)
+}
+
+SignatureRequest.prototype._removeBeforeUnload = () => {
   if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
-    this._onBeforeUnload = event => {
-      metricsEvent({
-        eventOpts: {
-          category: 'Transactions',
-          action: 'Sign Request',
-          name: 'Cancel Sig Request Via Notification Close',
-        },
-      })
-      clearConfirmTransaction()
-      cancel(event)
-    }
-    window.addEventListener('beforeunload', this._onBeforeUnload)
+    window.removeEventListener('beforeunload', this._beforeUnload)
   }
 }
 
-SignatureRequest.prototype.componentWillUnmount = function () {
+SignatureRequest.prototype.componentDidMount = function () {
   if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
-    window.removeEventListener('beforeunload', this._onBeforeUnload)
+    window.addEventListener('beforeunload', this._beforeUnload)
   }
 }
 
+SignatureRequest.prototype.componentWillUnmount = function () {
+  this._removeBeforeUnload()
+}
+
 SignatureRequest.prototype.renderHeader = function () {
   return h('div.request-signature__header', [
 
@@ -297,6 +302,7 @@ SignatureRequest.prototype.renderFooter = function () {
       large: true,
       className: 'request-signature__footer__cancel-button',
       onClick: event => {
+        this._removeBeforeUnload()
         cancel(event).then(() => {
           this.context.metricsEvent({
             eventOpts: {
@@ -315,6 +321,7 @@ SignatureRequest.prototype.renderFooter = function () {
       large: true,
       className: 'request-signature__footer__sign-button',
       onClick: event => {
+        this._removeBeforeUnload()
         sign(event).then(() => {
           this.context.metricsEvent({
             eventOpts: {
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
index 0e81dfe6eff6..25725381d600 100644
--- a/ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js
+++ b/ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js
@@ -387,7 +387,8 @@ export default class ConfirmTransactionBase extends Component {
 
     showRejectTransactionsConfirmationModal({
       unapprovedTxCount,
-      async onSubmit () {
+      onSubmit: async () => {
+        this._removeBeforeUnload()
         await cancelAllTransactions()
         clearConfirmTransaction()
         history.push(DEFAULT_ROUTE)
@@ -409,6 +410,7 @@ export default class ConfirmTransactionBase extends Component {
       updateCustomNonce,
     } = this.props
 
+    this._removeBeforeUnload()
     metricsEvent({
       eventOpts: {
         category: 'Transactions',
@@ -458,6 +460,7 @@ export default class ConfirmTransactionBase extends Component {
       submitting: true,
       submitError: null,
     }, () => {
+      this._removeBeforeUnload()
       metricsEvent({
         eventOpts: {
           category: 'Transactions',
@@ -568,8 +571,30 @@ export default class ConfirmTransactionBase extends Component {
     }
   }
 
+  _beforeUnload = () => {
+    const { txData: { origin, id } = {}, cancelTransaction } = this.props
+    const { metricsEvent } = this.context
+    metricsEvent({
+      eventOpts: {
+        category: 'Transactions',
+        action: 'Confirm Screen',
+        name: 'Cancel Tx Via Notification Close',
+      },
+      customVariables: {
+        origin,
+      },
+    })
+    cancelTransaction({ id })
+  }
+
+  _removeBeforeUnload = () => {
+    if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
+      window.removeEventListener('beforeunload', this._beforeUnload)
+    }
+  }
+
   componentDidMount () {
-    const { toAddress, txData: { origin, id } = {}, cancelTransaction, getNextNonce, tryReverseResolveAddress } = this.props
+    const { toAddress, txData: { origin } = {}, getNextNonce, tryReverseResolveAddress } = this.props
     const { metricsEvent } = this.context
     metricsEvent({
       eventOpts: {
@@ -583,20 +608,7 @@ export default class ConfirmTransactionBase extends Component {
     })
 
     if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
-      this._onBeforeUnload = () => {
-        metricsEvent({
-          eventOpts: {
-            category: 'Transactions',
-            action: 'Confirm Screen',
-            name: 'Cancel Tx Via Notification Close',
-          },
-          customVariables: {
-            origin,
-          },
-        })
-        cancelTransaction({ id })
-      }
-      window.addEventListener('beforeunload', this._onBeforeUnload)
+      window.addEventListener('beforeunload', this._beforeUnload)
     }
 
     getNextNonce()
@@ -604,9 +616,7 @@ export default class ConfirmTransactionBase extends Component {
   }
 
   componentWillUnmount () {
-    if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
-      window.removeEventListener('beforeunload', this._onBeforeUnload)
-    }
+    this._removeBeforeUnload()
   }
 
   render () {