Closed Bug 1351678 Opened 2 years ago Closed 2 years ago

Sync error after logging using an existing account on a profile with already stored data

Categories

(WebExtensions :: Untriaged, defect, P3)

defect

Tracking

(firefox52 unaffected, firefox53 wontfix, firefox54 fixed, firefox55 verified)

VERIFIED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- verified

People

(Reporter: vasilica.mihasca, Unassigned)

Details

(Whiteboard: triaged)

Attachments

(4 files)

[Affected versions]:
Firefox 55.0a1 (2017-03-26)
Firefox 54.0a2 (2017-03-26)
Firefox 53.0b7 (20170327081421)


[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.Install the attached webextenion.
3.Set a favourite colour in about:addons and click “Save” button.
4.Go to about:preferences#sync and login using an already used account.
5.Force a sync.

[Expected Results]:
Local data is kept and there are no errors encountered.

[Actual Results]:
- Local data is successfully kept but the following errors are thrown after the sync in completed: 
Warning: setting the doctor.lastRepairAdvance pref to the non-integer number 1490795925.77 converted it to the integer number 1490795925; to retain fractional precision, store non-integer numbers as strings.  Preferences.jsm:134
Sync encountered an error - see about:sync-log for the log file.  policies.js:731

[Additional notes]:
- Also at step 3, other errors appear in Browser Console:
TypeError: this._recipeManager is null[Learn More]  LoginManagerParent.jsm:73:9
Cannot send function call result: other side closed connection (call data: ({path:"storage.sync.set", args:[{colour:"3333333311111111"}]}))
Yeah, I definitely didn't consider this something we would handle. After all, we already show a dialog indicating that two people's data could get scrambled together when the second login happens. I think the correct way to handle it is to flush the local DB after the user clicks "yes" on that dialog, or maybe just reset sync status.
(In reply to Ethan Glasser-Camp (:glasserc) from comment #2)
> Yeah, I definitely didn't consider this something we would handle. After
> all, we already show a dialog indicating that two people's data could get
> scrambled together when the second login happens. I think the correct way to
> handle it is to flush the local DB after the user clicks "yes" on that
> dialog, or maybe just reset sync status.

I think you misunderstood this scenario. There are no 2 logins, only one and therefore no “Merge Warning” dialog is displayed.
Priority: -- → P3
Whiteboard: triaged
You're right, I misunderstood this scenario. Sorry for taking so long to get back to this. I'll try to have a patch together today.
Comment on attachment 8860118 [details]
Bug 1351678: Handle resolved conflicts correctly,

https://reviewboard.mozilla.org/r/132136/#review135488

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js:1151
(Diff revision 1)
> +      yield extensionStorageSync.syncAll();
> +      const remoteValue = (yield extensionStorageSync.get(extension, "remote-key", context))["remote-key"];
> +      equal(remoteValue, 8,
> +            "locally set value overrides remote value");
> +
> +      dump(`wtf ${JSON.stringify(calls)}\n`);

Probably want to remove this.
Attachment #8860118 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8860118 [details]
Bug 1351678: Handle resolved conflicts correctly,

https://reviewboard.mozilla.org/r/132136/#review135488

> Probably want to remove this.

Haha, oops! Sorry about that. I wish there was an eslint check for dump()..
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5053fcd89b01
Handle resolved conflicts correctly, r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5053fcd89b01
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attached file sync-logs.zip
I am still receiving the error-sync accompanied by a success-sync file on Firefox 55.0a1 (2017-05-01) under Windows 10 64-bit and Ubuntu 16.04 32-bit.

Any thoughts about this?
Flags: needinfo?(eglassercamp)
Sorry, I don't know anything about this failed sync. It appears to have something to do with Firefox Accounts information? The Extension-Storage part of the code doesn't appear to log any errors at all.
Flags: needinfo?(eglassercamp)
Vasilica, are you still seeing errors?  Maybe you can file a new bug. 

Ethan, looks like 54 is also affected. If you can help us verify this fix it might be good to uplift to beta 54.
Flags: needinfo?(vasilica.mihasca)
Flags: needinfo?(eglassercamp)
Shane, do you still work on Fx Accounts? Can you help find someone who might know how to help here?
Flags: needinfo?(stomlinson)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Vasilica, are you still seeing errors?  Maybe you can file a new bug. 

Checked again on Firefox 55.0a1 (2017-05-24) under Windows 10 64-bit and Ubuntu 16.04 32-bit and the sync was performed without errors. 
 
The following sync-error still appears on Firefox 54.0b10 (20170522172523) under both platforms: https://pastebin.com/qWx2hEYf
Status: RESOLVED → VERIFIED
Flags: needinfo?(vasilica.mihasca)
Hi :lizzard, 

I had a look at the referenced log and nothing looks particularly out of the ordinary from an FxA request P.O.V., but I have to admit I'm a bit out of my depth when it comes to knowing what the browser does.

My guess is that Mark Hammond would be able to direct you to the correct person/people, he works extensively with Sync. Shifting the ?ni to him.
Flags: needinfo?(stomlinson) → needinfo?(markh)
Comment on attachment 8860118 [details]
Bug 1351678: Handle resolved conflicts correctly,

Approval Request Comment
[Feature/Bug causing the regression]: chrome.storage.sync
[User impact if declined]: Syncing of chrome.storage.sync keyrings may fail for users with multiple devices. This won't cause any real data loss but is annoying.
[Is this code covered by automated tests?]: Yes.
[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]: None.
[Is the change risky?]: Not really.
[Why is the change risky/not risky?]: It's a targeted fix for a function that doesn't seem to be being used by extensions "in the wild".
[String changes made/needed]: None.
Flags: needinfo?(eglassercamp)
Attachment #8860118 - Flags: approval-mozilla-beta?
Comment on attachment 8860118 [details]
Bug 1351678: Handle resolved conflicts correctly,

Fix a sync error with multiple devices and was verified. Beta54+. Should be in 54 beta 12.
Attachment #8860118 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #16)
> The following sync-error still appears on Firefox 54.0b10 (20170522172523)
> under both platforms: https://pastebin.com/qWx2hEYf

The sync error log is due to the following entries:

> 1495700549267   Sync.Status DEBUG   Status for engine extension-storage: error.engine.reason.unknown_fail
> 1495700549267   Sync.Status DEBUG   Status.service: success.status_ok => error.sync.failed_partial
> 1495700549267   Sync.ErrorHandler   DEBUG   extension-storage failed: TypeError: conflict.remote is undefined (resource://gre/modules/ExtensionStorageSync.jsm:839:7) JS Stack trace: sync@ExtensionStorageSync.jsm:839:7 < waitForSyncCallback@async.js:98:7 < makeSpinningCallback/callback.wait@async.js:168:27 < promiseSpinningly@async.js:234:12 < _sync@extension-storage.js:44:12 < WrappedNotify@util.js:164:21 < sync@engines.js:720:5 < _syncEngine@enginesync.js:222:7 < sync@enginesync.js:169:15 < onNotify@service.js:1115:7 < WrappedNotify@util.js:164:21 < WrappedLock@util.js:120:16 < _lockedSync@service.js:1105:12 < sync/<@service.js:1097:7 < WrappedCatch@util.js:95:16 < sync@service.js:1086:5
> 1495700549269   Sync.Telemetry  TRACE   observed weave:engine:sync:error extension-storage

Ethan, can you confirm you patch should resolve this, or do we need a new bug?
Flags: needinfo?(markh) → needinfo?(eglassercamp)
Yes, this patch should resolve `conflict.remote is undefined`.

One of the logs Vasilica posted has an error starting like this:

> 1493715671035	FirefoxAccounts	ERROR	json parse error on response: [{"id":"01289872d160109697f095bbc447544e","lastAccessTime":null,"name":"vasilica.mihasca’s Nightly on P6079","type":"desktop","pushCallback":"https://updates.push.services.mozilla.com/wpush/v1/gAAAAABY_e8Kkl6arMesMx6PuGQ_jeoLrD-vNFMzEJSpviE1X7N7yO7I5-_D62Ma-DHwdt-7wXiCXxVBMXHXDWc2U1yWIQ5KKxZHrn7iFxk2VpzNNUURTdEEvo-LreHFLMvdQG5IprKt"...

This is the error I don't know anything about and should probably be in a new bug.
Flags: needinfo?(eglassercamp)
(In reply to Ethan Glasser-Camp (:glasserc) from comment #21)
> > 1493715671035	FirefoxAccounts	ERROR	json parse error on response: [{"id":"01289872d160109697f095bbc447544e","lastAccessTime":null,"name":"vasilica.mihasca’s Nightly on P6079","type":"desktop","pushCallback":"https://updates.push.services.mozilla.com/wpush/v1/gAAAAABY_e8Kkl6arMesMx6PuGQ_jeoLrD-vNFMzEJSpviE1X7N7yO7I5-_D62Ma-DHwdt-7wXiCXxVBMXHXDWc2U1yWIQ5KKxZHrn7iFxk2VpzNNUURTdEEvo-LreHFLMvdQG5IprKt"...
> 
> This is the error I don't know anything about and should probably be in a
> new bug.

Ah, I missed that - that is bug 1362099, already fixed.
(In reply to Ethan Glasser-Camp (:glasserc) from comment #18)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Ethan's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.