Page MenuHomePhabricator

[native] Have BackgroundIdentityLoginHandler wait until LogInHandler

Authored by ashoat on Mon, Jul 8, 10:27 AM.
Referenced Files
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



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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

tomek added inline comments.
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
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.

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.