Skip to content
This repository has been archived by the owner on Oct 18, 2018. It is now read-only.

Bug 964854 - cache account bundles #532

Closed
wants to merge 3 commits into from

Conversation

ncalexan
Copy link
Contributor

Such guesses, many work-arounds.

This is merely a stab in the dark, but if we are in fact seeing caching
errors, perhaps we're tickling them by writing twice when we could write
once.
…ccount bundles.

This should avoid reads from the Android Account user data store, which
we theorize is buggy.  It trades those reads for the complexity of
maintaining and invalidating an in-memory cache, which has the potential
to avoid races and cache corruption.

There is no reliable way to determine if an Account has been
removed (and subsequently re-added), so we clear the cache entirely when
any Firefox Account is added.  We do this at the authenticator level,
which should be more inclusive than doing it at the AndroidFxAccount
level.

I put the cache itself in AndroidFxAccount, since that is where we have
been storing things associated to the Android Account object; but it
could just as well go in the authenticator.
@rnewman
Copy link
Contributor

rnewman commented Feb 11, 2015

This all looks valid, but I think it leaves the largest and riskiest hole.

That is: unless the setUserData calls were racing (in which case anything else is unnecessary), we're simply hiding the problem with a memory cache. As soon as our process is killed, we'll hit the same symptoms we get now.

That might be worth having, but perhaps it's just needless complexity.

Are you planning a follow-up to figure out what to do when we get a (partially) empty bundle back, rather than just crashing? E.g., see what we do know, and transition to the appropriate state to re-capture credentials?

@ncalexan
Copy link
Contributor Author

On Tue, Feb 10, 2015 at 4:29 PM, Richard Newman notifications@github.com
wrote:

This all looks valid, but I think it leaves the largest and riskiest hole.

That is: unless the setUserData calls were racing (in which case anything
else is unnecessary), we're simply hiding the problem with a memory cache.
As soon as our process is killed, we'll hit the same symptoms we get now.

This is correct, but the point is that we believe Android's in memory
cache is the issue, and maintaining our own cache avoids hitting
Android's. We will still hit Android's cache on process start, but it
might be the case that Android itself needs to read from disk in order to
refresh its cache each process start.

That might be worth having, but perhaps it's just needless complexity.

Are you planning a follow-up to figure out what to do when we get a
(partially) empty bundle back, rather than just crashing? E.g., see what we
do know, and transition to the appropriate state to re-capture credentials?

This is more work than it seems because the Account state includes things
that we'd need to create valid account state, namely the server provided
opaque uid (which we don't actually use). We could transition some of the
state details into the Account user data and make it possible to go to
separated in this case. (I'm moving to this approach on iOS, where the
state details are really the state and not the account and the state.)

So my plan is to see if the band-aid helps (up-lifted to beta after a few
days) and re-assess. I could be plain wrong about Android caching issues :/

@ncalexan
Copy link
Contributor Author

Landed.

@ncalexan ncalexan closed this Feb 18, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants