Closed Bug 1153708 Opened 5 years ago Closed 4 years ago

Saving FxA profile data might clobber loginmanager data

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox38 --- unaffected
firefox38.0.5 --- unaffected
firefox39 --- verified
firefox40 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: markh, Assigned: markh)

References

Details

(Keywords: regression)

Attachments

(2 files)

The problem is:

* FxA starts up with MP locked, so only partial data is read.
* FxAccountsProfile.jsm fetches the profile data, and calls setUserAccountData() with updated data.
* Now setUserAccountData has been set we've lost the data in the login manager as we haven't previously read it.

To repro, see bug 1146346 comment 20, but when the "Sync Options" pane is open (which triggers the profile code).

See also bug 1150344, which describes how we might fix this.
FTR, this test change demonstrates the underlying problem.
Points: --- → 5
Flags: qe-verify+
Flags: firefox-backlog+
See Also: → 1156752
I opened bug 1157529 for the more general solution, but it's not clear this is going to land in time to fix this bug. Gavin suggested that we probably need to disable the profile saving code or otherwise work around the problem until a better fix is found.

Chris/Ryan/Zaach, can you guys have a chat about this and decide on what you think is the best way forward?

(NI all of you, but really it's just a single NI ;)
Flags: needinfo?(zack.carter)
Flags: needinfo?(rfkelly)
Flags: needinfo?(ckarlof)
How about we cache profile data in memory only for now? That way it won't hit the server every time the pref pane renders.
Flags: needinfo?(zack.carter)
I'll defer completely to Zach's instincts here.  Caching in memory seems like a sensible compromise in the short term.
Flags: needinfo?(rfkelly)
Assignee: nobody → zack.carter
As discussed on IRC
Assignee: zack.carter → mhammond
Status: NEW → ASSIGNED
Flags: needinfo?(ckarlof)
Attachment #8601247 - Flags: review?(zack.carter)
Iteration: --- → 40.3 - 11 May
Comment on attachment 8601247 [details] [diff] [review]
0004-Bug-1153708-put-the-FxA-profile-fetch-behind-the-sam.patch

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

Great! Thanks Mark.
Attachment #8601247 - Flags: review?(zack.carter) → review+
https://hg.mozilla.org/mozilla-central/rev/541073e6d227
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8601247 [details] [diff] [review]
0004-Bug-1153708-put-the-FxA-profile-fetch-behind-the-sam.patch

Approval Request Comment
[Feature/regressing bug #]: 1139657
[User impact if declined]: Master-password users will always be asked to sign in to sync
[Describe test coverage new/current, TreeHerder]: current pass
[Risks and why]: Low - user-facing part of this feature is pref'd off
[String/UUID change made/needed]: None
Attachment #8601247 - Flags: approval-mozilla-aurora?
Keywords: regression
Comment on attachment 8601247 [details] [diff] [review]
0004-Bug-1153708-put-the-FxA-profile-fetch-behind-the-sam.patch

Approved for aurora so that we don't ship 39 with this regression.
Attachment #8601247 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Florin, once this lands on 39 can your team verify the fix ? Thanks!
Flags: needinfo?(florin.mezei)
Assigning this to Catalin for verification.
Flags: needinfo?(florin.mezei)
QA Contact: catalin.varga
Verified as fixed using:

FF 39.0b6
Build Id: 20150615125213
Mac Os X 10.9.5
Flags: qe-verify+
Product: Core → Firefox
Target Milestone: mozilla40 → Firefox 40
You need to log in before you can comment on or make changes to this bug.