Skip to content

EC-149 Fix crash when trying to Focus on Lock page#1879

Merged
fedemkr merged 1 commit intomasterfrom
bug/fix-crash-becomefirstresponder
Jul 5, 2022
Merged

EC-149 Fix crash when trying to Focus on Lock page#1879
fedemkr merged 1 commit intomasterfrom
bug/fix-crash-becomefirstresponder

Conversation

@fedemkr
Copy link
Member

@fedemkr fedemkr commented Apr 13, 2022

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

Fix crash produced when focussing the pin/master password entry on the lock page after choosing the fallback method on a failed biometric scan.
I couldn't reproduced this but there are a lot of crashes reported on AppCenter so just fixed it where it's appearing to crash.
Also improved the code so that the ViewModel have fewer direct accesses to the View/Page.

Code changes

  • LockPage.xaml.cs: Improved the code making the SecretEntry centralizing the logic with the PinLock to get the correct entry. Also perform the logic of Focusing on the Page invoking it on the main thread to avoid the crash.
  • LockPageViewModel.cs: Moved the focus action to the page, given that the VM should not do this directly on MVVM. Also added WeakEventManager so we don't have to worry about memory leaks.

Testing requirements

I couldn't reproduce the issue but given the crash report it would be like:

  1. Enter you account
  2. Configure biometrics
  3. Configure pin
  4. Lock the account
  5. Try to unlock with fail biometric (i.e. don't use the correct one)
  6. Try again until the fallback appears (Master Password / Pin)
  7. Choose the fallback
  8. Then it should crash

Also it could be like that but with the automatic prompting of biometrics when minimizing the app and starting it again.

Btw, if no biometrics is setup the pin/master password entry should be automatically focused (i.e. should work as before) and if biometrics are configured and the fallback is selected the same should happen, i.e. it should automatically focus the pin/master password.

Before you submit

  • I have added unit tests where it makes sense to do so (encouraged but not required)
  • This change requires a documentation update (notify the documentation team)
  • This change has particular deployment requirements (notify the DevOps team)

…improved the code so there are fewer direct access from the VM to the View
@fedemkr fedemkr requested a review from a team April 13, 2022 21:52
@fedemkr fedemkr changed the title Fix crash when trying to Focus on Lock page EC-149 Fix crash when trying to Focus on Lock page Apr 13, 2022
@fedemkr fedemkr enabled auto-merge (squash) April 14, 2022 12:50
@fedemkr fedemkr requested a review from a team July 5, 2022 16:38
@fedemkr fedemkr merged commit dbc1e5e into master Jul 5, 2022
@fedemkr fedemkr deleted the bug/fix-crash-becomefirstresponder branch July 5, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants