621584 - [meta] Bookmarks dataloss (duplicated, reordered, moved, lost, attributes changed, etc) caused by Sync

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect, P3)

Description

[Philipp von Weitershausen [:philikon]]

This is a meta bug tracking the various issues people are reporting with Sync 1.6 / Firefox 4.0b8.

Comment 1

[Mike Connor [:mconnor]]

Not going to block on a meta bug, please nominate individual bugs instead.

Comment 2

[Dave Dyer]

I did another test, just by re setting up sync.  The android device melted down due to the bookmarks it received, and subsequently the up-sync damaged my original bookmarks.

Comment 3

[Richard Newman [:rnewman]]

I leave my own machine with verbose logging enabled, just in case I spot this issue. It happens to me once a year or so. It just did.

An incremental fetch returned 257 modified bookmark records:

storage/bookmarks?newer=1403930192.75&full=1
=>
1403996503978	Sync.Engine.Bookmarks	INFO	Records: 257 applied, 257 successfully, 0 failed to apply, 0 newly failed to apply, 0 reconciled.

A bunch of folder records applied:

1403996502868	Sync.Store.Bookmarks	DEBUG	Applying record NRt8rIRDd4TQ
1403996502868	Sync.Store.Bookmarks	DEBUG	Local parent is 5lQz_glXhEtn
1403996502868	Sync.Store.Bookmarks	DEBUG	Record NRt8rIRDd4TQ is not an orphan.
1403996502869	Sync.Store.Bookmarks	DEBUG	Reparenting orphans  to 4980
1403996502874	Sync.Store.Bookmarks	DEBUG	Applying record O7v4Ols_TnMD
1403996502874	Sync.Store.Bookmarks	DEBUG	Local parent is SFIUyHkW7qPb
1403996502874	Sync.Store.Bookmarks	DEBUG	Record O7v4Ols_TnMD is not an orphan.

No orphans.

A bunch of non-folder records:

1403996503230	Sync.Store.Bookmarks	DEBUG	Applying record 7k3qvuFPy11P
1403996503230	Sync.Store.Bookmarks	DEBUG	Local parent is VF3wbfjLyJVz
1403996503230	Sync.Store.Bookmarks	DEBUG	Record 7k3qvuFPy11P is not an orphan.
1403996503233	Sync.Store.Bookmarks	DEBUG	Applying record 9itnzgJiL0Ok
1403996503233	Sync.Store.Bookmarks	DEBUG	Local parent is slf34ON7SOO-
1403996503234	Sync.Store.Bookmarks	DEBUG	Record 9itnzgJiL0Ok is not an orphan.
1403996503239	Sync.Store.Bookmarks	DEBUG	Applying record AyhmBi7Cv598
1403996503239	Sync.Store.Bookmarks	DEBUG	Local parent is mobile

and some special folders:

1403996503972	Sync.Store.Bookmarks	DEBUG	Applying record mobile
1403996503972	Sync.Store.Bookmarks	DEBUG	Processing special node: mobile
1403996503973	Sync.Store.Bookmarks	DEBUG	Applying record ngcvhzmHboB4
1403996503973	Sync.Store.Bookmarks	DEBUG	Local parent is B8WxDqcMMQqf
1403996503974	Sync.Store.Bookmarks	DEBUG	Record ngcvhzmHboB4 is not an orphan.
1403996503975	Sync.Store.Bookmarks	DEBUG	Reparenting orphans  to 5265
1403996503977	Sync.Store.Bookmarks	DEBUG	Applying record toolbar
1403996503977	Sync.Store.Bookmarks	DEBUG	Processing special node: toolbar

Those record applications resulted in reordering.


Interestingly, a client record had changed:

1403996501296	Sync.Engine.Clients	INFO	Records: 1 applied, 1 successfully, 0 failed to apply, 0 newly failed to apply, 0 reconciled.

so I think we can deduce that another client synced.



The immediately previous sync had failed for network reasons:

1403979724657	Sync.ErrorHandler	TRACE	Beginning stream copy to error-1403979724657.txt: 1403979724657
1403979724658	Sync.Service	DEBUG	Exception: Error: NS_ERROR_NET_TIMEOUT (resource://services-sync/resource.js:394:18) JS Stack trace: [e-mail]:414:12 < [e-mail]:540:14 < [e-mail]:77:16 < [e-mail]:1271:7 < [e-mail]:141:21 < [e-mail]:96:16 < [e-mail]:1261:12 < sync/<@service.js:1253:14 < [e-mail]:70:16 < [e-mail]:1241:5
1403979724658	Sync.ErrorHandler	TRACE	Notifying weave:ui:sync:finish. Status.login is success.login. Status.sync is error.login.reason.network


10 logins that I hadn't used recently were downloaded and applied -- e.g., landfill.bugzilla.org, gogoinflight.com (I haven't flown for months). This really raises a red flag.

One login even threw because we tried to insert when it already existed (Bug 1031855):

1403996513250	Sync.Store.Passwords	DEBUG	Adding record {5df990fc-d78a-4566-be16-72006853a39a} resulted in exception [Exception... "This login already exists.'This login already exists.' when calling method: [nsILoginManager::addLogin]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: resource://gre/modules/services-sync/engines/passwords.js :: PasswordStore__create :: line 229"  data: no] Stack trace: PasswordStore__create()@resource://gre/modules/services-sync/engines/passwords.js:229 < Store.prototype.applyIncoming()@resource://services-sync/engines.js:329 < Store.prototype.applyIncomingBatch()@resource://services-sync/engines.js:297 < doApplyBatch()@resource://services-sync/engines.js:951 < doApplyBatchAndPersistFailed()@resource://services-sync/engines.js:966 < SyncEngine.prototype._processIncoming()@resource://services-sync/engines.js:1070 < SyncEngine.prototype._sync()@resource://services-sync/engines.js:1481 < WrappedNotify()@resource://services-sync/util.js:141 < Engine.prototype.sync()@resource://services-sync/engines.js:655 < _syncEngine()@resource://services-sync/stages/enginesync.js:199 < sync()@resource://services-sync/stages/enginesync.js:149 < onNotify()@resource://gre/modules/services-sync/service.js:1271 < WrappedNotify()@resource://services-sync/util.js:141 < WrappedLock()@resource://services-sync/util.js:96 < _lockedSync()@resource://gre/modules/services-sync/service.js:1261 < sync/<()@resource://gre/modules/services-sync/service.js:1253 < WrappedCatch()@resource://services-sync/util.js:70 < sync()@resource://gre/modules/services-sync/service.js:1241 < <file:unknown>


That site was last visited *in December*, so it should already have been synced long ago. That it existed with a different GUID is also bizarre.

So this looks like another client touched a bunch of records on the server based on very old state, and this client applied them, with predictably unhappy results.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #3)

> An incremental fetch returned 257 modified bookmark records:

I should note that I have a little over 1000 bookmarks.

Comment 5

[John Hesling [:John99] (NeedInfo me)]

Was this intended only to track old sync problems, 
and is there any tracker bug, 
or need for a tracker bug relating to new sync ?

A very quick glance finds sync & bookmark related bugs: bug 950167, bug 978339, bug 974080, bug 1025989, bug 1046954, bug1050397, bug 1092722

Comment 6

[Richard Newman [:rnewman]]

The only thing changed in Sync is the auth layer, so this is the right bug for tracking bookmark issues.

Comment 7

[Chris Karlof [:ckarlof]]

Removing priorities from all our meta sync bugs.

Comment 8

[pedrogfrancisco]

I've duplicated folders for History, Downloads and Tags on Firefox Developer Edition.

I have not however on Firefox Nightly.

Is this expected?

Comment 9

[Cédric Bellegarde]

Hello, I think I know what is happening for bookmark sort broken by Android Sync.

I was able to reproduce issue while adding sync support for my personnal project: "A GNOME web browser"

I think is missing some code like this:
- When pushing a new record, it need to push parent with "children" array
- Take care of pusning parent record after all children are pushed

Example:
{'id': 'toolbar', 'type': 'folder', 'parentid': 'places', 'parentName': '', 'title': 'Barre personnelle', 'children': ['TE44RsH4ZPoL', 'XncDg9-VCj09', '7aogaBxve0hT', 'QrTnV2_3Sqg3', '_hRqIwWa2bEf', 'nm4iqlypoS4k', 'PDR9JW7OKzJF', '9J_r61iy3QGK', 'bvp3MOmXzD1F', 'Z1j4ThDYGB4V', 'tVz6m8kFgKbc', 'OjgmgPaI_qEA', 'P7I1IfNannnb', 'gMp1WKi47YFs']}

Comment 10

[Richard Newman [:rnewman]]

(In reply to Cédric Bellegarde from comment #9)

> I think is missing some code like this:
> - When pushing a new record, it need to push parent with "children" array
> - Take care of pusning parent record after all children are pushed

The sync clients on all platforms do indeed do this. There are a variety of interesting causes for this kind of issue, including partial writes (remedied by atomic batch uploads in Bug 1253051 and friends), partial reads (buffered downloads; Bug 1305563), client bugs (many solved by Bug 1258127), non-structural record application, extremely large folders (Bug 1321021), and many more.

Comment 11

[Cédric Bellegarde]

(In reply to Richard Newman [:rnewman] from comment #10)
> Take care of pusning parent record after all children are pushed
> 
> The sync clients on all platforms do indeed do this. There are a variety of
> interesting causes for this kind of issue, including partial writes
> (remedied by atomic batch uploads in Bug 1253051 and friends), partial reads
> (buffered downloads; Bug 1305563), client bugs (many solved by Bug 1258127),
> non-structural record application, extremely large folders (Bug 1321021),
> and many more.

I think this is really an issue because: in Eolie Web Browser, I do not use folders but only tags but I have to deal with Firefox folders.

Updating a bookmark entry without updating "parentName", "parentid" and parent record should not remove the bookmark from parent, only update the entry.

Comment 12

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

(In reply to Cédric Bellegarde from comment #11) 
> Updating a bookmark entry without updating "parentName", "parentid" and
> parent record should not remove the bookmark from parent, only update the
> entry.

I haven't much context here - but who exactly is updating a bookmark record without specifying those fields?

Comment 13

[Cédric Bellegarde]

>I haven't much context here - but who exactly is updating a bookmark record without specifying those fields?

I was wrong in my previous comment, this is not my issue.

My issue is that if you update a bookmark record without updating parent record (and it's children attribute), the bookmark position in parent is changed (by Firefox).

Comment 14

[Lina Butler [:lina]]

Cédric, is the bookmark record uploaded from a third-party client, or Firefox? Could you please post an example of the full JSON bookmark record that's being uploaded to the server? If you're changing the "parentid" in the bookmark, or omitting it entirely, Firefox will move it to "unfiled". JSON records need to specify all fields, including ones that haven't changed; we don't support "deltas" with only a subset of fields.

Comment 15

[Cédric Bellegarde]

As I sayed, I was wrong, I understand it's needed to set the parentid. And yes it's a third-party client.

No, the issue is that if you update a record with it's parentid but you need to update parent record (children attribute) to preserve order.

Comment 16

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

(In reply to Cédric Bellegarde from comment #15)
> No, the issue is that if you update a record with it's parentid but you need
> to update parent record (children attribute) to preserve order.

Yes - for better or worse, that's how bookmarks work. Clients must update both, so this is WONTFIX.