1320324 - Sync error while resetting the add-on to its default values

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Vasilica Tamas, QA]

Attachment: category_tabs_for_google_keeptm-3.1.0-an+fx.xpi

[Prerequisites]:
- webextensions.storage.sync.enabled set to true
- create a new string setting identity.fxaccounts.autoconfig.uri and set it to https://stable.dev.lcip.org

[Affected versions]:
Firefox 53.0a1 (2016-11-24)
Firefox 52.0a2 (2016-11-22)


[Affected platforms]:
Windows 10 64-bit
Ubuntu 16.04 32-bit


[Steps to reproduce]:
1.Launch Firefox with a clean profile and set up the configuration.
2.Go to about:preferences#sync and login using a valid FxA account. 
3.Install the attached webextension.
4.Navigate to the add-on details tab from about:addons and uncheck some colors or change a few names from textboxes.
5.Force a sync.
6.Click on “Reset to default” button from the add-on details tab.
7.Force a sync.


[Expected Results]:
The default values are successfully synced.


[Actual Results]:
 - The values are reset but the following error is thrown in Browser Console after clicking on “Reset to default” button: Cannot send function call result: other side closed connection  ExtensionParent.jsm:469
 - The following error-sync-1480082060262.txt appears in about:sync-log after forcing the second sync: http://pastebin.com/VgP53DhG


[Additional notes]:
The default values are not synchronized to another Firefox profile.

Comment 1

[Ethan Glasser-Camp (:glasserc)]

Thanks for reporting. It looks like there's something obvious wrong with the code that handles deleted records/tombstones.

Comment 2

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1320324: deleted values are not handled correctly on sync,

No tests existed for this functionality either, so add some.

Review commit: https://reviewboard.mozilla.org/r/100560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100560/

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8821250 [details]
Bug 1320324: deleted values are not handled correctly on sync,

This looks good to me, but I don't think I'm the right person to review it.

Comment 4

[Mark Hammond [:markh]]

Comment on attachment 8821250 [details]
Bug 1320324: deleted values are not handled correctly on sync,

https://reviewboard.mozilla.org/r/100560/#review104446

LGTM!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ae4e620a16d
deleted values are not handled correctly on sync, r=markh

Comment 6

[Carsten Book [:Tomcat]]

backed out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=68025050&repo=autoland

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c2eac75f422
Backed out changeset 9ae4e620a16d for eslint failure

Comment 8

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8821250 [details]
Bug 1320324: deleted values are not handled correctly on sync,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100560/diff/1-2/

Comment 9

[Ethan Glasser-Camp (:glasserc)]

Ouch, sorry about that!

Comment 10

[Ethan Glasser-Camp (:glasserc)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16602b3b6810

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2a2e98c362d
deleted values are not handled correctly on sync, r=markh

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3fb11b63ef49
Fix ESLint error in test_ext_storage_sync.js.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/d2a2e98c362d
https://hg.mozilla.org/mozilla-central/rev/3fb11b63ef49

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Ethan, is it something that we want to uplift? Thanks

Comment 15

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8821250 [details]
Bug 1320324: deleted values are not handled correctly on sync,

Approval Request Comment
[Feature/Bug causing the regression]: chrome.storage.sync, specifically bug 1253740
[User impact if declined]: Syncing settings will not work correctly for the users which have enabled it
[Is this code covered by automated tests?]: Yes (see patch)
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: The change only affects users who have enabled the feature using the about:config flag, and only fixes one bug related to that feature
[String changes made/needed]: None

Comment 16

[Ethan Glasser-Camp (:glasserc)]

Yes, I'd like to uplift it to whatever you are willing to uplift it to.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8821250 [details]
Bug 1320324: deleted values are not handled correctly on sync,

fix issue with deleted values in sync, beta52+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a1baa5a308da

Comment 19

[Vasilica Tamas, QA]

Attachment: syncerror.txt

Tested this issue on Firefox 53.0b7 (20170327081421) under Windows 10 64-bit and Ubuntu 16.04 32-bit and noticed the following:

 - The following error appears in Browser Console when clicking on “Reset to default” button: Cannot send function call result: other side closed connection  ExtensionParent.jsm:554
 - The default values are successfully synced.
 - An error is encountered while syncing new values after “reset to default” step has already been performed. But, in spite of this, the new values are synced between profiles. I am attaching the sync-log file.

Could this error be related to Bug 1351678 or this is a different issue and must be tracked separately?

Comment 20

[Ethan Glasser-Camp (:glasserc)]

Sorry about the delay in getting to this. Yes, I think this is the same as bug 1351678.

Comment 21

[Ethan Glasser-Camp (:glasserc)]

Also, I don't know anything about "Cannot send function call result: other side closed connection  ExtensionParent.jsm:554". That isn't related to storage.sync code as far as I know.

Comment 22

[Vasilica Tamas, QA]

Tested again on Firefox 53 (20170413192749) and Firefox 55.0a1 (2017-04-24) under Windows 10 64-bit and Ubuntu 16.04 32-bit using a different webextension (https://addons-dev.allizom.org/en-US/firefox/addon/instant-multi-search/). This issue seems to be fixed and the default values are successfully synced.