Page MenuHomePhabricator

[lib][native][web] Wait until database deletion before rejecting login/registration attempt
ClosedPublic

Authored by ashoat on Tue, Apr 30, 11:37 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, May 13, 1:00 AM
Unknown Object (File)
Sat, May 11, 11:34 PM
Unknown Object (File)
Wed, May 8, 12:45 AM
Unknown Object (File)
Tue, May 7, 5:16 AM
Unknown Object (File)
Sun, May 5, 7:57 PM
Unknown Object (File)
Sun, May 5, 4:20 PM
Unknown Object (File)
Fri, May 3, 11:14 PM
Unknown Object (File)
Tue, Apr 30, 2:22 PM
Subscribers

Details

Summary

In the prior diff, we introduce some logic for resetting login state when authoritative keyserver auth fails during login or registration.

However, we don't wait for the login state to be reset before letting the user retry. That can lead to TASK_CANCELLED errors when we attempt ops on SQLite while it is in the process of being cleared (initially described on Linear).

To avoid this, we won't let the user retry until the database deletion is complete. During this period the UI will continue to show the login or registration as being in progress.

Note – would normally put @tomek and @inka on this review, but since they're out for the rest of this week, I'm asking @varun to review.

Depends on D11845

Test Plan
  1. Replicate the test plan in the parent diff (D11845)
  2. Repeatedly press the registration button while it's spinning, so that when it stops spinning it is immediately triggered
  3. Confirm that there is no TASK_CANCELLED error during identity auth, and no TASK_CANCELLED error is printed to the logs

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/data/sqlite-data-handler.js
46 ↗(On Diff #39711)

I searched for other calls to clearSensitiveData. There was only one other one, which was for resetting the database in wipeAndExit. I don't think that's relevant here (we don't need to wait for that to happen, since we don't expect it to happen)

web/shared-worker/sqlite-data-handler.js
46 ↗(On Diff #39711)

I searched for other calls to sharedWorker.init with clearDatabase: true. There was only one other one, which was for resetting the database if some of our SQLite ops fail. I don't think that's relevant here (we don't need to wait for that to happen, since we don't expect it to happen)

This revision is now accepted and ready to land.Wed, May 1, 10:21 AM