1563552 - Sync data choices checkboxes don't automatically update when the preference changes

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Raluca Popovici, Desktop QA]

[Preconditions:]

1. To activate the Form Autofill feature on a Beta build, go to about:config, search for the "extensions.formautofill.available" pref and modify it from "detect" to "on".
2. Go to browser Options, Privacy and Security section, Forms & Autofill section, click the "Saved Addresses..." button and add 1 or 2 address forms.

[Steps:]

1. Open 2FF profiles or two firefoxes on different machines (FF1 and FF2) and set up the same sync account on both of them.
2. From FF1 un-check the "Addresses" from about:preferences#sync
3. In FF1 Open the about:preferences#privacy and save a complete profile, afterward press Sync now from hamburger menu(top option).
4. In FF2 click on sync button and then check about:preferences#privacy / saved profiles.
5. In FF2 go to about:preferences#sync.

[Actual Result:]

You don't see the profile created at step 4 in FF1 but The "Addresses" is checked.

[Expected Result:]

The "Addresses" should be un-checked.

Comment 1

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

This appears to be true for all checkboxes on about:preferences#sync, and appears to be due to confusion between the "name" and "id" attributes. Specifically, these preferences are defined in the .xul as:

              <checkbox data-l10n-id="sync-engine-history"
                        preference="engine.history"/>

and in the .JS as

Preferences.addAll([
...
  { id: "engine.history", name: "services.sync.engine.history", type: "bool" },

When Preferences.observe() is called, this._all[data]; is referenced, where data is the preference name - however, this._all is indexed by id and not name - so for the prefs above, we are, effectively, doing this._all["services.sync.engine.history"] and fail to find it, because it is this._all["engine.history"] which we added to the map.

Best I can tell, these preferences are the only ones which define name, so I'm not sure if the best solution is to simply remove all support for name (and change the IDs of the sync pane to the full pref name), or to fix preferencesBindings.js so that the name attribute is used to add the element to the map.

Myk, this look like your change from bug 1379338 - WDYT?

(Fortunately the severity is fairly low, because everything is correct the next time about:prefs loads)

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to Mark Hammond [:markh] from comment #1)

When Preferences.observe() is called, this._all[data]; is referenced, where data is the preference name - however, this._all is indexed by id and not name - so for the prefs above, we are, effectively, doing this._all["services.sync.engine.history"] and fail to find it, because it is this._all["engine.history"] which we added to the map.

Best I can tell, these preferences are the only ones which define name, so I'm not sure if the best solution is to simply remove all support for name (and change the IDs of the sync pane to the full pref name), or to fix preferencesBindings.js so that the name attribute is used to add the element to the map.

Myk, this look like your change from bug 1379338 - WDYT?

I think removing the name attribute is the best solution. As you note, the "services.sync.*" prefs are the only ones that define name, and while that attribute presumably provided value at some point, it isn't clear how it does so today. My patch removed name from most of the prefs records (since name and id were identical for them), and I probably should've removed it from the rest at that point. Now is the second best time to do it!

Comment 3

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

Attachment: Bug 1563552 (part 1) - avoid using preferencesBindings 'name' attribute in the sync checkboxes. r?myk

Comment 4

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

Attachment: Bug 1563552 (part 2) - remove support for the 'name' attribute from preferencesBindings.js. r?myk

Depends on D38828

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce3ae99da523
(part 1) - avoid using preferencesBindings 'name' attribute in the sync checkboxes. r=myk,jaws
https://hg.mozilla.org/integration/autoland/rev/c7df8a9a00a4
(part 2) - remove support for the 'name' attribute from preferencesBindings.js. r=myk,jaws

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (Bug 1563552) for failures on browser_privacypane_2.js and browser_sanitizeDialog.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=c7df8a9a00a4558aa6f053f9c24ec81be159f3b6&searchStr=browser-chrome&tochange=7ebbbe62efadb26f66557a6781b3f49f7e2cd36a&selectedJob=258056964

Backout link: Backed out 2 changesets (Bug 1563552) for failures on browser_privacypane_2.js and browser_sanitizeDialog.js CLOSED TREE

Failures logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258056964&repo=autoland&lineNumber=3335

[task 2019-07-24T07:55:13.035Z] 07:55:13 INFO - Entering test bound bound runTestOnPrivacyPrefPane
[task 2019-07-24T07:55:13.035Z] 07:55:13 INFO - runTestOnPrivacyPrefPane entered
[task 2019-07-24T07:55:13.036Z] 07:55:13 INFO - Buffered messages logged at 07:55:12
[task 2019-07-24T07:55:13.037Z] 07:55:13 INFO - loaded about:preferences
[task 2019-07-24T07:55:13.038Z] 07:55:13 INFO - viewing privacy pane, executing testFunc
[task 2019-07-24T07:55:13.039Z] 07:55:13 INFO - Buffered messages finished
[task 2019-07-24T07:55:13.042Z] 07:55:13 INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_privacypane_2.js | Uncaught exception - at chrome://global/content/preferencesBindings.js:668 - TypeError: val.QueryInterface is not a function
[task 2019-07-24T07:55:13.042Z] 07:55:13 INFO - Stack trace:
[task 2019-07-24T07:55:13.043Z] 07:55:13 INFO - set valueFromPreferences@chrome://global/content/preferencesBindings.js:668:22
[task 2019-07-24T07:55:13.044Z] 07:55:13 INFO - set value@chrome://global/content/preferencesBindings.js:534:11
[task 2019-07-24T07:55:13.044Z] 07:55:13 INFO - reset_preferences@chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js:381:31
[task 2019-07-24T07:55:13.044Z] 07:55:13 INFO - runTestOnPrivacyPrefPane@chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js:16:9
[task 2019-07-24T07:55:13.045Z] 07:55:13 INFO - AsyncTester_execTest/<@chrome://mochikit/content/browser-test.js:1346:34
[task 2019-07-24T07:55:13.045Z] 07:55:13 INFO - asyncTester_execTest@chrome://mochikit/content/browser-test.js:1381:11
[task 2019-07-24T07:55:13.045Z] 07:55:13 INFO - nextTest/<@chrome://mochikit/content/browser-test.js:1209:14
[task 2019-07-24T07:55:13.046Z] 07:55:13 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:803:59
[task 2019-07-24T07:55:13.046Z] 07:55:13 INFO - Leaving test bound bound runTestOnPrivacyPrefPane
[task 2019-07-24T07:55:13.047Z] 07:55:13 INFO - GECKO(2158) | MEMORY STAT | vsize 20976045MB | residentFast 2071MB
[task 2019-07-24T07:55:13.047Z] 07:55:13 INFO - TEST-OK | browser/components/preferences/in-content/tests/browser_privacypane_2.js | took 14468ms
[task 2019-07-24T07:55:13.048Z] 07:55:13 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-07-24T07:55:13.049Z] 07:55:13 INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_privacypane_2.js | Found an unexpected tab at the end of test run: about:preferences#privacy -
[task 2019-07-24T07:55:13.166Z] 07:55:13 INFO - checking window state
[task 2019-07-24T07:55:13.208Z] 07:55:13 INFO - GECKO(2158) | JavaScript error: resource://testing-common/PromiseTestUtils.jsm, line 112: uncaught exception: Object
[task 2019-07-24T07:55:13.245Z] 07:55:13 INFO - TEST-START | browser/components/preferences/in-content/tests/browser_privacypane_3.js

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258056987&repo=autoland&lineNumber=2884

[task 2019-07-24T07:52:39.801Z] 07:52:39 INFO - TEST-PASS | browser/base/content/test/sanitize/browser_sanitizeDialog.js | timeSpan pref should be hour after accepting dialog with hour selected -
[task 2019-07-24T07:52:39.802Z] 07:52:39 INFO - TEST-PASS | browser/base/content/test/sanitize/browser_sanitizeDialog.js | history pref should be true after accepting dialog with history checkbox checked -
[task 2019-07-24T07:52:39.804Z] 07:52:39 INFO - TEST-PASS | browser/base/content/test/sanitize/browser_sanitizeDialog.js | downloads pref should be true after accepting dialog with history checkbox checked -
[task 2019-07-24T07:52:39.805Z] 07:52:39 INFO - Buffered messages logged at 07:49:39
[task 2019-07-24T07:52:39.806Z] 07:52:39 INFO - Longer timeout required, waiting longer... Remaining timeouts: 2
[task 2019-07-24T07:52:39.808Z] 07:52:39 INFO - Buffered messages logged at 07:51:09
[task 2019-07-24T07:52:39.810Z] 07:52:39 INFO - Longer timeout required, waiting longer... Remaining timeouts: 1
[task 2019-07-24T07:52:39.811Z] 07:52:39 INFO - Buffered messages finished
[task 2019-07-24T07:52:39.812Z] 07:52:39 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/sanitize/browser_sanitizeDialog.js | Test timed out -
[task 2019-07-24T07:52:39.818Z] 07:52:39 INFO - Console message: [JavaScript Error: "Unix error 2 during operation stat on file fakefile-0-minutes-ago (No such file or directory)" {file: "resource://gre/modules/DownloadCore.jsm" line: 124}]
[task 2019-07-24T07:52:39.819Z] 07:52:39 INFO - isPlaceholder@resource://gre/modules/DownloadCore.jsm:124:8
[task 2019-07-24T07:52:39.820Z] 07:52:39 INFO - asyncremoveData@resource://gre/modules/DownloadCore.jsm:2314:16
[task 2019-07-24T07:52:39.821Z] 07:52:39 INFO - removePartialData/this._promiseRemovePartialData<@resource://gre/modules/DownloadCore.jsm:878:28
[task 2019-07-24T07:52:39.822Z] 07:52:39 INFO - removePartialData@resource://gre/modules/DownloadCore.jsm:888:9
[task 2019-07-24T07:52:39.823Z] 07:52:39 INFO - finalize@resource://gre/modules/DownloadCore.jsm:1037:19
[task 2019-07-24T07:52:39.824Z] 07:52:39 INFO - blankSlate@chrome://mochitests/content/browser/browser/base/content/test/sanitize/browser_sanitizeDialog.js:901:20
[task 2019-07-24T07:52:39.826Z] 07:52:39 INFO - asyncinit/<@chrome://mochitests/content/browser/browser/base/content/test/sanitize/browser_sanitizeDialog.js:59:11
[task 2019-07-24T07:52:39.827Z] 07:52:39 INFO - nextTest@chrome://mochikit/content/browser-test.js:856:35
[task 2019-07-24T07:52:39.828Z] 07:52:39 INFO - timeoutFn@chrome://mochikit/content/browser-test.js:1467:18
[task 2019-07-24T07:52:39.829Z] 07:52:39 INFO - setTimeout handlerSimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:684:43
[task 2019-07-24T07:52:39.830Z] 07:52:39 INFO - timeoutFn@chrome://mochikit/content/browser-test.js:1435:52
[task 2019-07-24T07:52:39.831Z] 07:52:39 INFO - setTimeout handlerSimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:684:43
[task 2019-07-24T07:52:39.832Z] 07:52:39 INFO - timeoutFn@chrome://mochikit/content/browser-test.js:1435:52
[task 2019-07-24T07:52:39.833Z] 07:52:39 INFO - setTimeout handler*Tester_execTest@chrome://mochikit/content/browser-test.js:1414:80
[task 2019-07-24T07:52:39.834Z] 07:52:39 INFO - nextTest/<@chrome://mochikit/content/browser-test.js:1209:14

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e445ef2d906
(part 1) - avoid using preferencesBindings 'name' attribute in the sync checkboxes. r=myk,jaws
https://hg.mozilla.org/integration/autoland/rev/80ebc2bef53e
(part 2) - remove support for the 'name' attribute from preferencesBindings.js. r=myk,jaws

Comment 8

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/0e445ef2d906
https://hg.mozilla.org/mozilla-central/rev/80ebc2bef53e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Given how long this has been around, I'm thinking this fix can ride the trains. Let me know if you feel strongly otherwise.

Comment 10

[Raluca Popovici, Desktop QA]

Hi,
I verified the issue from the description on Win 10, MacOS 10.13 and Ubuntu 18.04, on Firefox Beta 70.0b11 and Nightly 71.0a1 (2019-10-03) and I can confirm the fix.

Additional note: During the testing I found out a different issue but I will come up with updates on another comment.

Comment 11

[Raluca Popovici, Desktop QA]

Prerequisites: You need an account with already some saved addresses.
Steps to reproduce:

1. Open two firefox instances(FF1 and FF2) on different OSes (I used Win 10 and MacOS 10.13 /Ubuntu 18.04) and log in with the same sync account on both of them.
2. In FF1(Win 10) open the about:preferences#privacy and check saved addresses. Observe that addresses from the prerequisites are displayed.
3. Go to FF2(MacOS 10.13/Ubuntu 18.04), sync account and open the about:preferences#privacy and verify saved addresses.

Actual Results:
There are no saved addresses displayed in FF2 (MacOS 10.13/Ubuntu 18.04).They were not imported from FF1.

Expected Results:
The same saved addresses from prerequisites are displayed in FF1 and FF2.

Note:

1. If I add a new address on FF1 (Win 10) and I sync both profiles(FF1 and FF2-Mac or Ubuntu) then this address is displayed in FF2, but only this one not the ones from the prerequisites.
2. If I open two profiles(FF1 and FF2) on Win 10, the same saved addresses from prerequisites are displayed in both profiles(FF1 and FF2).

Mark, please take a look over this and tell me what is your opinion. Should I log a bug or there already exists?
Thank you!

Comment 12

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

(In reply to Raluca from comment #11)

Mark, please take a look over this and tell me what is your opinion. Should I log a bug or there already exists?

I don't think there's a bug for addresses not syncing, so please file one. When you do, please be sure to attach logs (including "success" logs) from both profiles - see https://wiki.mozilla.org/CloudServices/Sync/File_a_desktop_bug for more details on this.

Thanks!