Closed
Bug 1268761
Opened 8 years ago
Closed 8 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)
Firefox
Firefox Accounts
Tracking
()
VERIFIED
FIXED
Firefox 49
People
(Reporter: markh, Assigned: eoger)
References
Details
Attachments
(1 file, 2 obsolete files)
21.44 KB,
patch
|
markh
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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+
Comment 1•8 years ago
|
||
> 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.
Reporter | ||
Comment 2•8 years ago
|
||
(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.
Assignee | ||
Comment 3•8 years ago
|
||
I used a pref to implement this, but I don't mind redoing it using signedInUser.json instead if someone opinionated cares about this.
Reporter | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
You were totally right Mark, it's so much cleaner now.
Attachment #8748434 -
Attachment is obsolete: true
Attachment #8748783 -
Flags: review?(markh)
Reporter | ||
Comment 6•8 years ago
|
||
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)?
Assignee | ||
Comment 7•8 years ago
|
||
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)
Reporter | ||
Comment 8•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/25d777f7efb3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Reporter | ||
Comment 11•8 years ago
|
||
Edouard, can you please request aurora uplift in a week or so?
Flags: needinfo?(edouard.oger)
Assignee | ||
Comment 12•8 years ago
|
||
Sure!
Assignee | ||
Comment 13•8 years ago
|
||
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 14•8 years ago
|
||
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+
Comment 15•8 years ago
|
||
Tracking for 48, since comment 1 explains that sync users on 48 will need this for push to be properly supported.
tracking-firefox48:
--- → +
Flags: qe-verify+
Comment 16•8 years ago
|
||
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)
Reporter | ||
Comment 17•8 years ago
|
||
(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)
Assignee | ||
Comment 18•8 years ago
|
||
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)
Comment 19•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1a617887fb7
Comment 20•8 years ago
|
||
Hi Edouard, Have you managed to verify this issue on your side? Is there anything QE can help with?
Flags: needinfo?(edouard.oger)
Assignee | ||
Comment 21•8 years ago
|
||
I can confirm that the patch fixed the bug on Firefox 48 beta.
Flags: needinfo?(edouard.oger)
Comment 22•8 years ago
|
||
VERIFIED FIXED based on comment #21.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•8 years ago
|
Updated•6 years ago
|
Product: Core → Firefox
Updated•6 years ago
|
Target Milestone: mozilla49 → Firefox 49
You need to log in
before you can comment on or make changes to this bug.
Description
•