[M3] WIP Use the FxA Rust backend behind a pref #2
Conversation
We should bring in some stuff from that PR into this. using the m3 name sgtm |
services/fxaccounts/RustFxAccount.js
Outdated
| // There's a high chance the JS event loop will mess up everything, | ||
| // such as doing OP1 "write-state" AFTER OP2 "write-state". |
There was a problem hiding this comment.
Because of this, we might have to guard the RustFxAccount ops in a promise queue like we've done on other occasions on Desktop, what do you think @mhammond?
There was a problem hiding this comment.
Interesting. IIUC we do get queue-like behaviour from punt, which ensures that each operation at the rust level is executed serially and in order. I wonder if we can also convince it to guarantee calling these callbacks in order rather than having to add another layer of queueing here.
There was a problem hiding this comment.
Yeah I think it's a good idea, we can access the password manager from C++ I believe.
There was a problem hiding this comment.
(and even from Rust using XPCOM magic)
There was a problem hiding this comment.
Sorry I missed this - but yeah, I think the order of these callbacks being called can be deterministic, but as soon as the JS code in the callbacks start awaiting etc, then all bets are off. These notifications aren't async, which we faced in sync too, so we have a "promise queue" there, but that's more about ensuring they are all finished at certain points than about guaranteeing the order of them resolving. A strict ordering queue is tricky because at some points we explicitly chose to let a promise complete in the background - just grep for "background" in services/fxaccounts - often actual persisting of data is done in the background,
Note sure I'm answering the question, but I'm strongly sympathizing with the problem :)
|
<https://github.com/vladikoff> I'm using the fxa branch to spike on
logging, should I be using m3 instead?
for the spike `fxa` should be fine. depends on the changes we either can
send them to mozilla-central or to m3 after
…On Wed, May 20, 2020 at 5:41 PM lougeniaC64 ***@***.***> wrote:
@eoger <https://github.com/eoger> @vladikoff
<https://github.com/vladikoff> I'm using the fxa branch to spike on
logging, should I be using m3 instead?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA7N44LYEPLE7PJ5PU2FATRSRE7TANCNFSM4NGKJXNA>
.
|
| let { | ||
| key: { kid, k: kSync }, | ||
| } = await this._fxia.rustFxa.getAccessToken(SCOPE_OLD_SYNC); | ||
| let kXCS = kid.split("-")[1]; |
There was a problem hiding this comment.
Yikes! I understand why we're doing this, but I wonder if we could do the inverse refactoring here instead - rather than carving up the scoped-keys data held by the rust component so we can return it in the older format expected by desktop, could we refactor the desktop code that is calling this so that it instead works with scoped keys? I think @vladikoff's work to access sync via OAuth headed a bit in this direction already.
| const scopedKeys = {}; | ||
| if (this._fxia.rustFxa) { | ||
| for (const scope of scopes.split(" ")) { | ||
| const { key } = await this._fxia.rustFxa.getAccessToken(scope); |
There was a problem hiding this comment.
Hrm, if we don't already have an OAuth token with that scope, this is going to create one for no reason. I guess that's proably OK in practice because we'll usually have one already, but still, it's a shame.
| kSync, | ||
| kXCS, | ||
| kExtSync: null, // huuuuh | ||
| kExtKbHash: null, // yeah huuh. |
There was a problem hiding this comment.
It's OK, these keys will go away with the landing of the new webext-storage work, which causes us to stop using special keys and start using the same kSync as all the other synced datatypes. So the fact that we don't have them will not be a problem.
Bug 1630291 - rust backend pref and basic fxa login
remove extra fxa instance
This is a WIP patch that keeps the
FxAccountand friends API intact while using the Rust backend if theidentity.fxaccounts.useExperimentalRustClientpref is present.It's very much prototype quality, but I think it could be a good start. So far I've been able to show the profile info in the UI and trigger a sync. Device stuff is not working yet though.
How do you feel @mhammond @vladikoff @lougeniaC64 about contributing to this branch to achieve the M3 milestone? (maybe we could make PRs against this PR). (I didn't touch #1 since there seem to be rebase problems there)