1335538 - Request volume to profile.accounts.firefox.com has doubled starting in Firefox 51

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Sean McArthur [:seanmonstar]]

We've noticed an increased number of requests coming from Firefox starting with the rollout of Firefox 51. We can certainly handle the load, and it may not be an actual bug, but since we had to increase the number of EC2 instances to handle this, it's worth checking for the exact reason of **why**.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

I'm guessing this must be bug 1319639. We now will attempt to fetch the profile on any sync error. I suspect that some users are seeing sync errors on every sync, and while we throttle the number of requests to at most once per 2 minutes, this alone may account for that increase - users who do *not* see a sync error will typically only fetch the profile when a new window is created or when sync prefs are opened.

We could optimize the code to avoid this.

Comment 2

[Lina Butler (ex-Mozilla)]

Does bug 1323102 help here?

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Kit Cambridge [:kitcambridge] from comment #2)
> Does bug 1323102 help here?

I expect that will cause the same request volume, but with most of them returning 304 instead of the profile body. I've no idea how much that would impact server load, but I suspect not that much.

Comment 4

[Edouard Oger [:eoger]]

Attachment: Bug 1335538 - Don't fetch the FxA profile on every UI update in browser-fxaccounts.

Review commit: https://reviewboard.mozilla.org/r/108788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108788/

Comment 5

[Edouard Oger [:eoger]]

Here's a quick fix. Manual testing looked good (even with cases like switching accounts in the same window), but I'm thinking we should maybe ask softvision for a thorough test of the hamburger menu UI if this lands just to be sure we didn't break anything.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8832553 [details]
Bug 1335538 - Don't fetch the FxA profile on every UI update in browser-fxaccounts.

https://reviewboard.mozilla.org/r/108788/#review110122

Yeah, this looks safe, but I agree it makes sense to have softvision give this a workout. It also makes sense to coordinate with the FxA folks to try and verify that the number of requests on Nightly does drop after this lands (although that *might* be tricky as the volume of Nightly users hitting this might be too low to get conclusive data)

Thanks for jumping on this!

::: browser/base/content/browser-fxaccounts.js:144
(Diff revision 1)
>    },
>  
>    observe(subject, topic, data) {
>      switch (topic) {
> -      case this.FxAccountsCommon.ONPROFILE_IMAGE_CHANGE_NOTIFICATION:
> -        this.updateUI();
> +      case this.FxAccountsCommon.ON_PROFILE_CHANGE_NOTIFICATION:
> +        this.profileFetched = false;

I think we should probably initialize this variable.

Comment 7

[Edouard Oger [:eoger]]

Comment on attachment 8832553 [details]
Bug 1335538 - Don't fetch the FxA profile on every UI update in browser-fxaccounts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108788/diff/1-2/

Comment 8

[Edouard Oger [:eoger]]

Comment on attachment 8832553 [details]
Bug 1335538 - Don't fetch the FxA profile on every UI update in browser-fxaccounts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108788/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58967ce93315
Don't fetch the FxA profile on every UI update in browser-fxaccounts. r=markh

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/58967ce93315

Comment 11

[Lina Butler (ex-Mozilla)]

From conversation with jrgm, it might be worth uplifting this if traffic doesn't level off.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Kit Cambridge [:kitcambridge] from comment #11)
> From conversation with jrgm, it might be worth uplifting this if traffic
> doesn't level off.

John, can you please try and verify this does have the impact we hope so we can request uplift?

Comment 13

[Jon Buckley [:jbuck]]

:markh We've seen a 40% drop in traffic so far today (February 6th) from Firefox Nightly users compared to February 2nd. Lets get this patch uplifted to aurora and beta to verify with larger channel populations.

:lizzard needinfo so you can track this

Comment 14

[Edouard Oger [:eoger]]

This is not ready for uplift, we found a bug with this patch.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this for 51 onward. Let us know when you are ready to uplift fixes! Thanks.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like when you are ready, we should also ask SV to do exploratory testing (as mentioned in comment 5)
FYI Ioana.

Comment 17

[Edouard Oger [:eoger]]

Both patches landed in Nightly, can we do the testing?

Comment 18

[Mark Hammond [:markh] [:mhammond]]

needinfo me and Edouard to make sure it doesn't fall off our radar - the earlier we can get this uplifted the better!

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Hi Kanchan, given this is a desktop bug I thought I should get it on your radar - we are hoping to uplift this patch ASAP.

Comment 20

[Mark Hammond [:markh] [:mhammond]]

I think the testing we need here is:

* On Nightly, verify that the hamburger menu always shows the correct profile information (ie, profile image and display name or email address if no display name is set).

* Verify using the browser's devtools "network" tab that there are only requests to profile.accounts.firefox.com are never made closer than 2 minutes apart from each other. Note that on 54 and 53, we expect some of these requests to return a 304, whereas once this hits 52 we will always see a 200 response - that is expected.

At that point we will request uplift if this bug and bug 1337026 to 53 and 52, at which point it would be great to re-verify the behaviour on both - we expect 53 to behave the same as 54, whereas 52 is slightly riskier as another bug which landed on 53 will not be on 52 - it *should* still work fine, but this change means additional testing is worthwhile.

Please let me know if you need any more information.

Comment 21

[Kanchan Kumari QA]

Thanks  Mark! This fix seems to be working fine on the latest Nightly. 

UI looks good and shows correct profile information. I tested it by making several profile changes.
Also, I monitored the network traffic through developers tool but never saw requests to profile.accounts.firefox.com at a rate faster that 2 minutes. In fact, in some of my testing, I didn't see even a single profile request going out. Tested by making changes to firefox profile using bout:preferences#sync, multiple devices log on for the same Fx account and making profile changes on one,executing 'Services.obs.notifyObservers(null, "weave:service:sync:error", null)' in browser console etc.

With the observed behavior, we can conclude that the bug fix is spot on.

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8832553 [details]
Bug 1335538 - Don't fetch the FxA profile on every UI update in browser-fxaccounts.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: We should work hard to get this into 52
User impact if declined: Increased volume to profile.accounts.firefix.com
Fix Landed on Version: 54
Risk to taking this patch (and alternatives if risky): Fairly low risk, limited to the Sync UI, and even then, limited to the user's profile image not being shown.
String or UUID changes made by this patch:  None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: 51
[User impact if declined]: As above
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, as described in the bug.
[List of other uplifts needed for the feature/fix]: Bug 1337026
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Fairly low risk, limited to the Sync UI, and even then, limited to the user's profile image not being shown.
[String changes made/needed]: None

Comment 23

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 8832553 [details]
Bug 1335538 - Don't fetch the FxA profile on every UI update in browser-fxaccounts.

avoid unnecessary FxA network traffic, for aurora53 and beta52

Clearing the esr52 flag since that'll be synced from beta.

Comment 24

[Carsten Book [:Tomcat]]

This hit conflicts on aurora - Edouard could you take a look ? thanks

grafting 387768:58967ce93315 "Bug 1335538 - Don't fetch the FxA profile on every UI update in browser-fxaccounts. r=markh"
merging browser/base/content/browser-fxaccounts.js
merging browser/base/content/test/general/browser_fxaccounts.js
warning: conflicts while merging browser/base/content/browser-fxaccounts.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/base/content/test/general/browser_fxaccounts.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 25

[Edouard Oger [:eoger]]

Attachment: bug-1335538-aurora.patch

Here's the rebased patch for Aurora

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6e42f39aa98

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/70c542383655

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/70c542383655

Comment 29

[Kanchan Kumari QA]

I have tested this bug fix on Aurora 53, Beta 52 and ESR 52 and found it to be working fine.