Sync settings chosen while creating a new sync account are ignored

VERIFIED FIXED in Firefox 30, Firefox OS v1.4

Status

()

Firefox
Sync
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: mail, Assigned: markh)

Tracking

26 Branch
Firefox 32
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox30+ verified, firefox31+ verified, firefox32 verified, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: p=5 s=it-32c-31a-30b.2 [qa!])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

- Setup Sync in Firefox
- Create a new Sync account
- During the account creation, change Sync settings (chose to not sync Add-ons and passwords)
- Finished creating sync account


Actual results:

In Firefox's settings dialog on the Sync tab, all sync options are selected (including previously deselected Add-ons and passwords.


Expected results:

Sync options, which are deselected during account creation, should be disabled after finishing account creation.

Comment 1

4 years ago
Yup, I got the same behavior here with the new sync in aurora.

Comment 2

4 years ago
Ditto, W7ProSP1x64, FF29.0.

Comment 3

4 years ago
What's the point of the Aurora and beta channel when a bug reported in the alpha phase makes it to the official release untouched...

Updated

4 years ago
Component: Untriaged → Sync
Sorry this didn't get noticed sooner - thanks for the bug triage YF.

Tracy, Ed, this sounds familiar to me (I think we had some similar issues early on), but comment 2 is a recent report. Have you seen this / know of any other bugs?
Flags: needinfo?(twalker)
Flags: needinfo?(edwong)
Keywords: qawanted
It does seem familiar, though I haven't found a related bug.  That said, this works correctly for me when creating an account on Mac.  However, it fails as reported on Win 8.1 Surface Pro tablet.  :-(
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(twalker)
Flags: needinfo?(edwong)
Flags: firefox-backlog+

Updated

4 years ago
Whiteboard: p=5

Updated

4 years ago
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Whiteboard: p=5 → p=5 s=it-32c-31a-30b.2 [qa?]
(Assignee)

Comment 6

4 years ago
Created attachment 8422163 [details] [diff] [review]
0001-Bug-961471-ensure-the-initial-sync-prefs-pane-writes.patch

The issue appears to be that we aren't using a <prefwindow> with type="child".  I'm guessing there was a reason for using <dialog>, so a fix is just to call the writePreferences method on the <prefpane>.  With this patch the preferences are correctly updated as the dialog is accepted.
Attachment #8422163 - Flags: review?(ttaubert)
Comment on attachment 8422163 [details] [diff] [review]
0001-Bug-961471-ensure-the-initial-sync-prefs-pane-writes.patch

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

(In reply to Mark Hammond [:markh] from comment #6)
> The issue appears to be that we aren't using a <prefwindow> with
> type="child".  I'm guessing there was a reason for using <dialog>

Good question, I honestly don't remember why it was chosen. The proposed fix however looks fine to me.

::: browser/base/content/sync/customize.js
@@ +19,5 @@
>  }
>  
>  addEventListener("dialogaccept", function () {
> +  let pane = document.getElementById("sync-customize-pane");
> +  pane.writePreferences(true);

Nit: We don't *need* to flush here, right?
Attachment #8422163 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 8

4 years ago
(In reply to Tim Taubert [:ttaubert] from comment #7)
> Nit: We don't *need* to flush here, right?

Dunno - ISTM that in theory we *never* need to flush.  I'm assuming the flush is to ensure a subsequent crash doesn't lose the pref changes, and I see a similar scenario applying here.  Please correct me if I'm wrong though :)
I don't think the chance of a crash here is higher than somewhere else though. If we always flush on write we could as well just remove the parameter but I think we probably don't want to flush and cause a disk write when accepting the dialog. The pref file can be written asynchronously - which I hope we do already.
(Assignee)

Comment 10

4 years ago
(In reply to Tim Taubert [:ttaubert] from comment #9)
> I don't think the chance of a crash here is higher than somewhere else
> though.

Yes - and prefwindow does it like this. IMO this is the same basic use-case as a prefwindow - so I'm not sure if you are saying that's a different use-case, or if prefwindow also shouldn't flush, or something else?
Well, if the prefwindow does it we can do it too. I don't really think it's that important :)
(Assignee)

Updated

4 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa?] → p=5 s=it-32c-31a-30b.2 [qa+]
https://hg.mozilla.org/mozilla-central/rev/74261c911d36
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
QA Contact: alexandra.lucinet
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0
Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0

Reproduced with Nightly 2014-05-13, verified as fixed with latest Nightly (Build ID: 20140516030204) on Windows 7 x64, Mac OS X 10.9.2 and Ubuntu 12.04 32bit.
Status: RESOLVED → VERIFIED
Keywords: qawanted
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa+] → p=5 s=it-32c-31a-30b.2 [qa!]
This one's a little embarrassing - we should uplift.
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → fixed
tracking-firefox30: --- → +
tracking-firefox31: --- → +
(Assignee)

Comment 16

4 years ago
Comment on attachment 8422163 [details] [diff] [review]
0001-Bug-961471-ensure-the-initial-sync-prefs-pane-writes.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FxA Sync
User impact if declined: User who chooses what to sync has their preferences ignored.
Testing completed (on m-c, etc.): Landed and verified on m-c
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None
Attachment #8422163 - Flags: approval-mozilla-beta?
Attachment #8422163 - Flags: approval-mozilla-aurora?
Attachment #8422163 - Flags: approval-mozilla-beta?
Attachment #8422163 - Flags: approval-mozilla-beta+
Attachment #8422163 - Flags: approval-mozilla-aurora?
Attachment #8422163 - Flags: approval-mozilla-aurora+
Marking as verified on Firefox 32 based on comment 14, and setting back [qa+] for verification on Beta and Aurora.
status-firefox32: fixed → verified
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa!] → p=5 s=it-32c-31a-30b.2 [qa+]
status-b2g-v1.4: --- → fixed
status-b2g-v2.0: --- → fixed

Comment 20

4 years ago
Verified as fixed on Firefox 30 beta 6 (Build ID: 20140520115057) and latest Aurora 31 (Build ID: 20140521004003) on Mac OS X 10.9.2, Windows 7 x64 and Ubuntu 12.04 x32:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0
status-firefox30: fixed → verified
status-firefox31: fixed → verified
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa+] → p=5 s=it-32c-31a-30b.2 [qa!]

Comment 21

4 years ago
Verified as fixed on Nighty32.0a1(Build ID:20140521030200) on Ubuntu(Desktop) 14.04 :
Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0
You need to log in before you can comment on or make changes to this bug.