Page MenuHomePhabricator

[native] Have BackgroundIdentityLoginHandler wait until LogInHandler
ClosedPublic

Authored by ashoat on Mon, Jul 8, 10:27 AM.
Tags
None
Referenced Files
F2319718: D12698.id.diff
Mon, Jul 22, 9:21 PM
Unknown Object (File)
Fri, Jul 19, 2:32 PM
Unknown Object (File)
Thu, Jul 18, 12:55 AM
Unknown Object (File)
Tue, Jul 16, 7:42 PM
Unknown Object (File)
Tue, Jul 16, 2:14 PM
Unknown Object (File)
Mon, Jul 15, 5:15 PM
Unknown Object (File)
Mon, Jul 15, 3:06 PM
Unknown Object (File)
Mon, Jul 15, 10:38 AM
Subscribers
None

Details

Summary

This addresses a race condition described in ENG-8785, first investigated in ENG-8772.

Depends on D12697

Test Plan

I reproduced the issue by following these instructions, but making sure to log in with SIWE. Before this diff the issue reproduces; after the diff, it no longer reproduced

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added inline comments.
native/components/background-identity-login-handler.react.js
31–37 ↗(On Diff #42123)

This sounds confusing: we're checking if we are logged in to identity to know if we are ready to try the identity login.

This revision is now accepted and ready to land.Tue, Jul 9, 5:04 AM
native/components/background-identity-login-handler.react.js
31–37 ↗(On Diff #42123)

Hmm... I see your point. I think it all stems from the naming of isLoggedInToIdentityAndAuthoritativeKeyserver.

That name is based on a post-identity world, where currentUserInfo is set by identity (mostly), and the authoritative keyserver cookie is set in keyserverStore by the authoritative keyserver.

However, in a pre-identity world, both of these are set by the authoritative keyserver.

I'm going to land this for now, but I'm curious if you have a better suggested name for isLoggedInToIdentityAndAuthoritativeKeyserver that could improve the readability here.

native/components/background-identity-login-handler.react.js
31–37 ↗(On Diff #42123)

Not sure, we can consider using a different layer of abstraction and say hasCurrentUserInfoAndIsLoggedInAuthoritativeKeyserver - but it is really long. We can also consider mentioning that the user was logged in to identity. But overall, I don't see a name that is significantly better than this.