Skip to content

Cleanup beforeunload handler after transaction is resolved#7333

Merged
Gudahtt merged 5 commits intodevelopfrom
e2e-bug-fixes
Oct 31, 2019
Merged

Cleanup beforeunload handler after transaction is resolved#7333
Gudahtt merged 5 commits intodevelopfrom
e2e-bug-fixes

Conversation

@Gudahtt
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt commented Oct 31, 2019

  • Cleanup beforeunload handler after transaction is resolved

    The notification window was updated to reject transactions upon close in Cancel transaction on close of notification window #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.

  • Fix indentation and remove redundant export

    The run-all.sh Bash script now uses consistent indentation, and is consistent about only re-exporting the Ganache arguments when they change.

  • Ensure transactions are completed before checking balance

    Various intermittent e2e test failures appear to be caused by React re-rendering the transaction list during the test, as the transaction goes from pending to confirmed. To avoid this race condition, the transaction is now explicitly looked for in the confirmed transaction list in each of the tests using this pattern.

  • Enable all e2e tests on Firefox

    The remaining tests that were disabled on Firefox now work correctly. Only a few timing adjustments were needed.

  • Update Firefox used in CI

    Firefox v70 is now used on CI instead of v68. This necessitated rewriting the function where the extension ID was obtained because the Firefox extensions page was redesigned.

@Gudahtt Gudahtt requested a review from tmashuang October 31, 2019 05:41
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [549bd05]

@Gudahtt Gudahtt changed the title Cleanup transaction rejection on window close Cleanup beforeunload handler after transaction is resolved Oct 31, 2019
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.
The `run-all.sh` Bash script now uses consistent indentation, and is
consistent about only re-exporting the Ganache arguments when they
change.
Various intermittent e2e test failures appear to be caused by React
re-rendering the transaction list during the test, as the transaction
goes from pending to confirmed. To avoid this race condition, the
transaction is now explicitly looked for in the confirmed transaction
list in each of the tests using this pattern.
The remaining tests that were disabled on Firefox now work correctly.
Only a few timing adjustments were needed.
Firefox v70 is now used on CI instead of v68. This necessitated
rewriting the function where the extension ID was obtained because the
Firefox extensions page was redesigned.
@Gudahtt Gudahtt dismissed a stale review via a1a92dd October 31, 2019 12:55
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [a1a92dd]

Copy link
Copy Markdown
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great PR

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [a1a92dd]

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [a1a92dd]

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [a1a92dd]

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [a1a92dd]

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [a1a92dd]

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [a1a92dd]

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [a1a92dd]

@Gudahtt Gudahtt merged commit fe28e0d into develop Oct 31, 2019
Gudahtt added a commit that referenced this pull request Nov 4, 2019
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
Gudahtt added a commit that referenced this pull request Nov 4, 2019
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
tmashuang pushed a commit that referenced this pull request Nov 4, 2019
* 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.

* Fix indentation and remove redundant export

The `run-all.sh` Bash script now uses consistent indentation, and is
consistent about only re-exporting the Ganache arguments when they
change.

* Ensure transactions are completed before checking balance

Various intermittent e2e test failures appear to be caused by React
re-rendering the transaction list during the test, as the transaction
goes from pending to confirmed. To avoid this race condition, the
transaction is now explicitly looked for in the confirmed transaction
list in each of the tests using this pattern.

* Enable all e2e tests on Firefox

The remaining tests that were disabled on Firefox now work correctly.
Only a few timing adjustments were needed.

* Update Firefox used in CI

Firefox v70 is now used on CI instead of v68. This necessitated
rewriting the function where the extension ID was obtained because the
Firefox extensions page was redesigned.
tmashuang pushed a commit that referenced this pull request Nov 4, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants