1632734 - fxAccounts.device public API methods should all use `withCurrentAccountState` internally

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, enhancement)

Description

[Ryan Kelly [:rfkelly]]

The FxAccounts implementation has a withCurrentAccountState helper method that's designed to guard against concurrent changes to the user's account state, but it's not used consistently. I think all public API methods should use this helper in order to prevent calling code from breaking invariants we assume about our account state.

As an example, suppose I call the fxAccounts.device.getLocalId() method, which is async. If there's a problem with my current device registration, then this method might end up making several calls to the FxA server to try to rectify it.

Now imagine that while that method call is in flight, the user signs out of their browser. What should happen here is that the in-flight promise rejects with an error about the user signing out of their account. What will actually happen is not clear, because the implementation of this method directly accesses _fxai.currentAccountState rather than using the withCurrentAccountState helper.

I intend to fix at least this one API, because I want to use it safely over in Bug 1604844. I would like this to be a nice mechanical refactor that I can apply to other public API methods while I'm in there, but we'll see how that works out in practice...

Comment 1

[Ryan Kelly [:rfkelly]]

I would like this to be a nice mechanical refactor that I can apply to other public API methods while I'm in there, but we'll see how that works out in practice...

Hahahaha no; I'm going to de-scope this bug to just the devices API, which is the one I need.

Comment 2

[Ryan Kelly [:rfkelly]]

Attachment: Bug 1632734 - refactor FxAccounts.device public methods to use `withCurrentAccountState`. r?eoger,markh

Comment 3

[Ryan Kelly [:rfkelly]]

The patch here is ready to go but I'm going to wait for the code-freeze before landing, given that our device-registration machinery and its interaction with push/send-tab has historically been fairly fragile. ni? myself to revisit this in a few days once the merge is complete.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56ce8aa215c0
refactor FxAccounts.device public methods to use `withCurrentAccountState`. r=eoger,markh

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/56ce8aa215c0