1278564 - Synchronization fails if all incoming bookmark records are value-only changes, resulting in no tree to merge

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, defect)

Description

[Aaron Train [:aaronmt]]

There are reports of Synchronization failing for users on v5.0 (Beta), including Maria. The users are seeing the error message 'Syncing has failed'.

Comment 1

[Aaron Train [:aaronmt]]

The troubleshooting link also leads to an article with no content.

Comment 2

[Aaron Train [:aaronmt]]

One failed state (of how many?) that is reproducible.

=> Bookmark any site
=> Sync Now on your device
=> Sync Now on desktop
=> Modify the page title on desktop
=> Sync Now on desktop
=> Sync Now on your device

This will yield 'Syncing has failed.'

Comment 3

[Richard Newman [:rnewman]]

I get "Sync is unavailable". The first time was after a long delay, so probably some progress was made syncing.

Comment 4

[Benson Wong [:mostlygeek]]

Just updated to v5.0.0 (10) and still having syncing issues

Comment 5

[Benson Wong [:mostlygeek]]

The message I get is: Syncing has failed. The link to troubleshoot still goes nowhere.

Comment 6

[Aaron Train [:aaronmt]]

(In reply to Aaron Train [:aaronmt] from comment #2)
> One failed state (of how many?) that is reproducible.
> 
> => Bookmark any site
> => Sync Now on your device
> => Sync Now on desktop
> => Modify the page title on desktop
> => Sync Now on desktop
> => Sync Now on your device
> 
> This will yield 'Syncing has failed.'

Can this case be fixed for 5.0?

Comment 7

[Stephan Leroux [:sleroux]]

> Can this case be fixed for 5.0?

I don't think so. I ran this scenario to see what kind of error was occurring and it looks like it's failing when merging bookmark changes. The reason being reported is the "Tree is unrooted" [1]. :rnewman might have some more insight as to why this is the case.

[1] https://github.com/mozilla/firefox-ios/blob/master/Sync/Synchronizers/Bookmarks/ThreeWayTreeMerger.swift#L1005

Comment 8

[Richard Newman [:rnewman]]

If the sequence in Comment 2 is accurate, then the steps will be as follows:

> > => Bookmark any site

This should automatically trigger a sync, uploading two records (the parent and the new record).

> > => Sync Now on your device

This should download and merge.

> > => Sync Now on desktop

This should do nothing.

> > => Modify the page title on desktop

This should upload a new version of the new record.

> > => Sync Now on desktop

This should do nothing.

> > => Sync Now on your device

This should download the single modified record, which will only be value-different from a known merged tree record, with no structural changes. That should be fine.


One of these things might be happening:

* The initial addition/merge is not resulting in a sane mirror state, and so indeed the newly changed record isn't overlaying on the borked mirror -- the bug is earlier in the flow.

* There's a bug in the tree extractor or overlayer. This bug is later in the flow.

* The changed server record has actually moved, and the server is now inconsistent. This would happen if you picked a new parent folder for it during a sync (Bug 1235269). This is a desktop bug.

It should be pretty easy to see what's happening if this repros.

Comment 9

[Richard Newman [:rnewman]]

Failing test:

https://github.com/mozilla/firefox-ios/pull/1927

Working on this today.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Pull req.

Comment 11

[Richard Newman [:rnewman]]

From the PR:



The issue here is that a value-only change doesn't result in a remote (or local) tree. There was a sanity check in the merger to make sure that we weren't called with nothing to do, because upstream callers should short-circuit in that case. That check turns out to be too aggressive.

This PR adds a test and makes it work by removing that check.

We also need to remove an observational test elsewhere that verified the sanity check itself, and thus would now fail.

Comment 12

[Stephan Leroux [:sleroux]]

Comment on attachment 8764179 [details] [review]
Pull req.

Nice catch! Simple patch and test passes. LGTM!

Comment 13

[Richard Newman [:rnewman]]

master: d4eb0e9
v5.x: 0cb44c3