961471 - Sync settings chosen while creating a new sync account are ignored

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Stefan Kleeschulte]

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

[kinwolf]

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

Comment 2

[zschuetz]

Ditto, W7ProSP1x64, FF29.0.

Comment 3

[kinwolf]

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...

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 5

[Tracy Walker [:tracy]]

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.  :-(

Comment 6

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

Attachment: 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.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 8

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

(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 :)

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 10

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

(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?

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Well, if the prefwindow does it we can do it too. I don't really think it's that important :)

Comment 12

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

https://hg.mozilla.org/integration/fx-team/rev/74261c911d36

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/74261c911d36

Comment 14

[Bogdan Maris, Desktop Test Engineering]

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.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

This one's a little embarrassing - we should uplift.

Comment 16

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

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9a5dcd5f9d37
https://hg.mozilla.org/releases/mozilla-beta/rev/09a9c1859b6b

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking as verified on Firefox 32 based on comment 14, and setting back [qa+] for verification on Beta and Aurora.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/09a9c1859b6b

Comment 20

[Ada [:adalucinet]]

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

Comment 21

[YaoMingWang]

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