Closed Bug 1268761 Opened 3 years ago Closed 3 years ago

Have the client know when it needs to re-register the device due to additional fields being added.

Categories

(Firefox :: Firefox Accounts, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
firefox48 + verified
firefox49 --- fixed

People

(Reporter: markh, Assigned: eoger)

References

Details

Attachments

(1 file, 2 obsolete files)

Periodically we will add new fields to the device registration info we send to the FxA servers. When Firefox updates to a new version, it's possible that the new version will register additional fields to support features in that version (eg, we added push support this way) - but because the client has already registered the device it doesn't know that it should re-register the device with those new fields.

The simplest approach would use some kind of versioning and cause the new registration to happen immediately after the new version starts, but this means the server is going to be under heavy load each time a new release with new fields comes out. This might be OK if we put some additional work in to ensure the "backoff" semantics work correctly (and relatively silently - I think currently we will write an error log) and the server team puts effort into ensuring the server sends these responses before the servers have melted.

There might be other approaches we could explore by trying to make such registration lazy or deferring the registration in a way that attempts to balance the load over time, but these seem risky - eg, it would be hard to explain that, say, push features aren't working for many people due to this, and in this example there's no obvious "trigger" the client can use to say "now would be a good time to register as a push message might be coming soon".

I think we need to get something into 48 for this, or all 47 clients who have already registered will fail to tell the server about their push endpoints as they upgrade to 48.

Thoughts?
Flags: firefox-backlog+
> but this means the server is going to be under heavy load each time a new release with new fields
>  comes out. This might be OK if we put some additional work in to ensure the "backoff" semantics
> work correctly (and relatively silently - I think currently we will write an error log) and
> the server team puts effort into ensuring the server sends these responses before the servers have melted.

TBH this seems like the right tradeoff to me - investing heavily in doing lazy registration may be a good long-term goal but let's do something simple and understandable to start with.  Giving us the ability to send error-log-free backoffs from the server side and have them respected by the client, sounds like a nice place to be.
See Also: → 1269673
(In reply to Ryan Kelly [:rfkelly] from comment #1)
> TBH this seems like the right tradeoff to me

Sounds great, thanks. At this stage I'm *not* opening a bug for "don't report an error on 503" as TBH I'm not sure that's desirable yet - we don't get many complaints about "unexpected error files", were happy to have 46 create them on every Sync, and they may be useful in some cases. I did open bug 1268761 to add better support for retryAfter.

So it sounds like this bug can just implement comment 0 and ignore any mitigation against heavy load on new releases. It remains P1 as 48 needs it for push to work correctly.
Attached patch bug-1268761.patch (obsolete) — Splinter Review
I used a pref to implement this, but I don't mind redoing it using signedInUser.json instead if someone opinionated cares about this.
Assignee: nobody → edouard.oger
Status: NEW → ASSIGNED
Attachment #8748434 - Flags: review?(markh)
Comment on attachment 8748434 [details] [diff] [review]
bug-1268761.patch

Review of attachment 8748434 [details] [diff] [review]:
-----------------------------------------------------------------

I know I said in IRC that I didn't have a firm opinion on this, but looking at the patch I think it would be much clearer saving that to the user data than to prefs - it seems like that could be done without any new functions.

::: services/fxaccounts/FxAccounts.jsm
@@ +351,5 @@
>    // The timeout (in ms) we use to poll for a verified mail for the first 2 mins.
>    VERIFICATION_POLL_TIMEOUT_INITIAL: 15000, // 15 seconds
>    // And how often we poll after the first 2 mins.
>    VERIFICATION_POLL_TIMEOUT_SUBSEQUENT: 30000, // 30 seconds.
> +  DEVICE_REGISTRATION_VERSION: 1,

We should comment what this is for.

@@ +1525,5 @@
>  
>        log.debug("registering new device details");
>        return this.fxAccountsClient.registerDevice(
>          signedInUser.sessionToken, deviceName, this._getDeviceType(), deviceOptions);
> +    }).then(device => {

We should put an obvious comment as the first line of _registerOrUpdateDevice() saying something like: NOTE: if you change what we send the FxA server on device registration, be sure to update DEVICE_REGISTRATION_VERSION.

::: services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js
@@ +453,5 @@
>    do_check_eq(spy.count, 0);
>    do_check_eq(result, "foo's device id");
>  });
>  
> +add_task(function* test_getDeviceId_with_device_id_and_no_stale_flag_and_registration_version_outdated_invokes_device_registration() {

I think we can shorten the name :)
Attachment #8748434 - Flags: review?(markh)
Attached patch bug-1268761.patch (obsolete) — Splinter Review
You were totally right Mark, it's so much cleaner now.
Attachment #8748434 - Attachment is obsolete: true
Attachment #8748783 - Flags: review?(markh)
Comment on attachment 8748783 [details] [diff] [review]
bug-1268761.patch

Review of attachment 8748783 [details] [diff] [review]:
-----------------------------------------------------------------

What do you think about taking this one step further and replacing isDeviceStale with this flag? ie, maybe whenever we set isDeviceState to true we just instead set deviceRegistrationVersion to null (ie, delete it)?
I'm all for it, the semantics of the 2 variables are similar, we can simplify this.
Attachment #8748783 - Attachment is obsolete: true
Attachment #8748783 - Flags: review?(markh)
Attachment #8749405 - Flags: review?(markh)
Comment on attachment 8749405 [details] [diff] [review]
bug-1268761.patch

Review of attachment 8749405 [details] [diff] [review]:
-----------------------------------------------------------------

I think that's great, thanks!
Attachment #8749405 - Flags: review?(markh) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/25d777f7efb3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Edouard, can you please request aurora uplift in a week or so?
Flags: needinfo?(edouard.oger)
Sure!
Comment on attachment 8749405 [details] [diff] [review]
bug-1268761.patch

Approval Request Comment
[Feature/regressing bug #]: 1268761
[User impact if declined]: We will have to delay the new sync related push notifications patches, which means the user won't be able to get a new notification when a device joins his/her sync account.
[Describe test coverage new/current, TreeHerder]: Tests added/modified in test_accounts_device_registration.js, all tests green of course.
[Risks and why]: Low risk, the patch was merged in central a week ago now without any reported issues.
[String/UUID change made/needed]: None
Flags: needinfo?(edouard.oger)
Attachment #8749405 - Flags: approval-mozilla-aurora?
Comment on attachment 8749405 [details] [diff] [review]
bug-1268761.patch

Adds support for sync accounts for 48, includes some tests.
This looks like something we should verify on aurora 48 once everything has landed.
Attachment #8749405 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking for 48, since comment 1 explains that sync users on 48 will need this for push to be properly supported.
Flags: qe-verify+
Mark or Edouard, can you describe how to test this for QE ? or is your team planning to test it out? 

You  mention other patches still waiting to land. Are they already on m-c?
Flags: needinfo?(markh)
Flags: needinfo?(edouard.oger)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16)
> Mark or Edouard, can you describe how to test this for QE ? or is your team
> planning to test it out? 

I think we could come up with a test plan to ensure that push is working for Sync - I think the simplest might be to use 47 to create and verify an account, upgrade that profile to 48, then use a second device to delete the account and ensure that 48 sees the deletion immediately via push.

Edouard, can you please come up with a reproducible test plan along those lines? (Or other lines if you can think of a simpler one :)

> You  mention other patches still waiting to land. Are they already on m-c?

That was primarily bug 1267760, which we will land on central and don't anticipate requesting uplift on.
Flags: needinfo?(markh)
We can't figure out how to make an easy test plan. Is that okay if we do the verification ourselves?
Flags: needinfo?(edouard.oger)
Hi Edouard,
Have you managed to verify this issue on your side? Is there anything QE can help with?
Flags: needinfo?(edouard.oger)
I can confirm that the patch fixed the bug on Firefox 48 beta.
Flags: needinfo?(edouard.oger)
VERIFIED FIXED based on comment #21.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Core → Firefox
Target Milestone: mozilla49 → Firefox 49
You need to log in before you can comment on or make changes to this bug.