Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: seanmonstar, Assigned: eoger)

Tracking

51 Branch
Firefox 54
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51+ affected, firefox52+ verified, firefox53+ verified, firefox54+ verified, firefox-esr52 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
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**.

Updated

6 months ago
Summary: Increase by double requests to profile.accounts.firefox.com starting in Firefox 51 → Request volume to profile.accounts.firefox.com has doubled starting in Firefox 51

Comment 1

6 months ago
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.
Does bug 1323102 help here?

Comment 3

6 months ago
(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 hidden (mozreview-request)
(Assignee)

Comment 5

6 months ago
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.
(Assignee)

Updated

6 months ago
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1

Comment 6

6 months ago
mozreview-review
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.
Attachment #8832553 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

6 months ago
Pushed by eoger@mozilla.com:
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

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58967ce93315
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
From conversation with jrgm, it might be worth uplifting this if traffic doesn't level off.
(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?
Flags: needinfo?(jrgm)

Comment 13

6 months ago
: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
Flags: needinfo?(jrgm) → needinfo?(lhenry)
(Assignee)

Comment 14

6 months ago
This is not ready for uplift, we found a bug with this patch.
Depends on: 1337026
Tracking this for 51 onward. Let us know when you are ready to uplift fixes! Thanks.
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox51: --- → +
tracking-firefox52: --- → +
tracking-firefox53: --- → +
tracking-firefox54: --- → +
Flags: needinfo?(lhenry)
Sounds like when you are ready, we should also ask SV to do exploratory testing (as mentioned in comment 5)
FYI Ioana.
Flags: needinfo?(chiorean.ioana)

Updated

5 months ago
Flags: needinfo?(chiorean.ioana) → needinfo?(ioana.chiorean)
QA Contact: ioana.chiorean
(Assignee)

Comment 17

5 months ago
Both patches landed in Nightly, can we do the testing?
needinfo me and Edouard to make sure it doesn't fall off our radar - the earlier we can get this uplifted the better!
Flags: needinfo?(markh)
Flags: needinfo?(eoger)
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.
Flags: needinfo?(markh) → needinfo?(kkumari)
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

5 months ago
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.
status-firefox54: fixed → verified
Flags: needinfo?(kkumari)
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
Flags: needinfo?(ioana.chiorean)
Flags: needinfo?(eoger)
Attachment #8832553 - Flags: approval-mozilla-esr52?
Attachment #8832553 - Flags: approval-mozilla-beta?
Attachment #8832553 - Flags: approval-mozilla-aurora?
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.
Attachment #8832553 - Flags: approval-mozilla-esr52?
Attachment #8832553 - Flags: approval-mozilla-beta?
Attachment #8832553 - Flags: approval-mozilla-beta+
Attachment #8832553 - Flags: approval-mozilla-aurora?
Attachment #8832553 - Flags: approval-mozilla-aurora+
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')
Flags: needinfo?(eoger)
(Assignee)

Comment 25

5 months ago
Created attachment 8838071 [details] [diff] [review]
bug-1335538-aurora.patch

Here's the rebased patch for Aurora
Flags: needinfo?(eoger)

Comment 26

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a6e42f39aa98
status-firefox53: affected → fixed
Flags: in-testsuite+

Comment 27

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/70c542383655
status-firefox52: affected → fixed

Comment 28

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/70c542383655
status-firefox-esr52: --- → fixed

Comment 29

5 months ago
I have tested this bug fix on Aurora 53, Beta 52 and ESR 52 and found it to be working fine.
status-firefox52: fixed → verified
status-firefox53: fixed → verified
status-firefox-esr52: fixed → verified
You need to log in before you can comment on or make changes to this bug.