Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: MetaMask app quits if dApp initiated transaction is rejected #12240

Closed
sleepytanya opened this issue Nov 8, 2024 · 4 comments · Fixed by #12311
Closed

[Bug]: MetaMask app quits if dApp initiated transaction is rejected #12240

sleepytanya opened this issue Nov 8, 2024 · 4 comments · Fixed by #12311
Labels
android Android specific issue regression-RC-7.34.0 Regression bug that was found in release candidate (RC) for release 7.34.0 release-7.37.0 Issue or pull request that will be included in release 7.37.0 team-mobile-platform type-bug Something isn't working

Comments

@sleepytanya
Copy link
Contributor

sleepytanya commented Nov 8, 2024

Describe the bug

DApp-initiated transactions can be confirmed successfully. However, every time when a transaction is rejected, the app unexpectedly quits. After quit the performance seems to be degraded.

Observed on Android devices only.

Expected behavior

Screenshots/Recordings

7.34.0 (1475):

reject.mov

7.34.1 (1480):

7_34_1.mov
Screenshot 2024-11-07 at 22 46 50

Steps to reproduce

  1. Connect to a dApp
  2. Start transaction
  3. Reject on Confirmation screen

Error messages or log output

No response

Detection stage

During release testing

Version

7.34.0 (1475), 7.34.1 (1480)

Build type

None

Device

Samsung S24+

Operating system

Android

Additional context

No response

Severity

No response

@sleepytanya sleepytanya added type-bug Something isn't working android Android specific issue regression-RC-7.34.0 Regression bug that was found in release candidate (RC) for release 7.34.0 labels Nov 8, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Nov 8, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Nov 8, 2024
@sleepytanya sleepytanya added team-mobile-platform team-confirmations Push issues to confirmations team labels Nov 8, 2024
@MetaMask MetaMask deleted a comment from Emmaward090 Nov 8, 2024
@pedronfigueiredo
Copy link
Contributor

No errors on the application code should cause the whole app to crash. If we pinpoint where the error is happening, the confirmations team can work on handling it, but it still would be preferable to address the underlying problem of crashing the UI separately.

@pedronfigueiredo pedronfigueiredo removed the team-confirmations Push issues to confirmations team label Nov 12, 2024
@Cal-L
Copy link
Contributor

Cal-L commented Nov 14, 2024

@pedronfigueiredo It is unclear what the culprit of the crash is at the moment until some debugging is done. Based on the description atm, the assumption is that the error is either related confirmations UI and/or other logic under the hood, which is why it was assigned to confirmations team. I can see that @sleepytanya Referenced #12228. @sleepytanya Since it's referenced, could you confirm if that fixed this issue? If not, could you assist in identifying the crash message? A simple way to do this is to connect your physical device and open Android studio. Once you run MM, the application should appear as an option in the Logcat, which will spew out logs including the error that occurs when it crashes. Feel free to reach out if you're having issues.

@sethkfman
Copy link
Contributor

This issue seems related to the crash we are seeing in production #12266

@sleepytanya
Copy link
Contributor Author

sleepytanya commented Nov 15, 2024

@Cal-L @sethkfman
The bug is present in latest main build meaning that #12228 didn't fix it.

logcat.log

Screen.Recording.2024-11-14.at.19.19.54.mov

github-merge-queue bot pushed a commit that referenced this issue Nov 15, 2024
## **Description**

Rejecting a transaction confirmation caused the app to crash.

This was due to Hermes not supporting the recursive nature of the error
`stack` during serialisation.

Resolution:

- Force the use of `7.0.1` of `@metamask/rpc-errors` since there are no
API breaking changes.
- Update the pre-existing patch to target this new version.

## **Related issues**

Fixes: #12240 

## **Manual testing steps**

See issue.

## **Screenshots/Recordings**

### **Before**

### **After**

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Nov 15, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Nov 15, 2024
@metamaskbot metamaskbot added the release-7.37.0 Issue or pull request that will be included in release 7.37.0 label Nov 15, 2024
runway-github bot added a commit that referenced this issue Nov 15, 2024
## **Description**

Rejecting a transaction confirmation caused the app to crash.

This was due to Hermes not supporting the recursive nature of the error
`stack` during serialisation.

Resolution:

- Force the use of `7.0.1` of `@metamask/rpc-errors` since there are no
API breaking changes.
- Update the pre-existing patch to target this new version.

## **Related issues**

Fixes: #12240 

## **Manual testing steps**

See issue.

## **Screenshots/Recordings**

### **Before**

### **After**

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
runway-github bot added a commit that referenced this issue Nov 15, 2024
## **Description**

Rejecting a transaction confirmation caused the app to crash.

This was due to Hermes not supporting the recursive nature of the error
`stack` during serialisation.

Resolution:

- Force the use of `7.0.1` of `@metamask/rpc-errors` since there are no
API breaking changes.
- Update the pre-existing patch to target this new version.

## **Related issues**

Fixes: #12240

## **Manual testing steps**

See issue.

## **Screenshots/Recordings**

### **Before**

### **After**

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
runway-github bot pushed a commit that referenced this issue Nov 15, 2024
## **Description**

Rejecting a transaction confirmation caused the app to crash.

This was due to Hermes not supporting the recursive nature of the error
`stack` during serialisation.

Resolution:

- Force the use of `7.0.1` of `@metamask/rpc-errors` since there are no
API breaking changes.
- Update the pre-existing patch to target this new version.

## **Related issues**

Fixes: #12240 

## **Manual testing steps**

See issue.

## **Screenshots/Recordings**

### **Before**

### **After**

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
sethkfman pushed a commit that referenced this issue Nov 15, 2024
- fix: transaction reject crash (#12311)

## **Description**

Rejecting a transaction confirmation caused the app to crash.

This was due to Hermes not supporting the recursive nature of the error
`stack` during serialisation.

Resolution:

- Force the use of `7.0.1` of `@metamask/rpc-errors` since there are no
API breaking changes.
- Update the pre-existing patch to target this new version.

## **Related issues**

Fixes: #12240 

## **Manual testing steps**

See issue.

## **Screenshots/Recordings**

### **Before**

### **After**

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding

Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling

guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
[8fa867d](8fa867d)

Co-authored-by: Matthew Walsh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android Android specific issue regression-RC-7.34.0 Regression bug that was found in release candidate (RC) for release 7.34.0 release-7.37.0 Issue or pull request that will be included in release 7.37.0 team-mobile-platform type-bug Something isn't working
Projects
Archived in project
Status: Fixed
Development

Successfully merging a pull request may close this issue.

5 participants