718154 - [meta] Ensure correct handling of bookmark reconciling, moves, and such

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect, P1)

Description

[Richard Newman [:rnewman]]

We put a lot of effort into getting this mostly correct on desktop, and I'm convinced that there are huge flaws in the equivalent (very young, barely tested) code on Android.

Given the awful user impact of getting this wrong, ensuring correctness in this regard comes pretty much right after ensuring that backoff handling works.

The following scenarios come to mind:

* Repeated fresh-start syncs should be completely stable: no duplications or reorderings.

* Incoming folder has a new GUID. When reconciling to an existing folder, ensure that children have their parent GUID adjusted.

* Incoming folder and local folder both have new children. A merged children list should result, with some consistent order.

* Records yielded by Fennec should have a position set as needed. When the position of an item changes, other items will have their position changed, too. When the position changes, the parent record must be re-uploaded, because its children array will have changed.

* When an item is added to or removed from a folder, the changed parents should be uploaded, too.

* Bookmark folders should have a higher sortindex than bookmarks. Ideally upload them first for clarity.

* When we reconcile two records, I believe desktop always takes the remote GUID. We should follow this example if my memory is correct.

I'm sure there are more, but this will do as a starting point!

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

rnewman, it looks like Tony went through and nom'd all your bugs. We're triaging now and will make our best call on all of them, but please renom and comment if you think we've got something wrong.

Comment 2

[Richard Newman [:rnewman]]

(In reply to Brad Lassey [:blassey] from comment #1)
> rnewman, it looks like Tony went through and nom'd all your bugs. We're
> triaging now and will make our best call on all of them, but please renom
> and comment if you think we've got something wrong.

You got it. Thanks!

Comment 3

[Richard Newman [:rnewman]]

Switching this to me, given that Bug 718238 swelled to cover most of these.

I believe most of these scenarios are therefore fixed; any unusual edge cases I'm happy to file as follow-ups to get test coverage and implemented.

Will investigate.

Comment 4

[Tracy Walker [:tracy]]

Initial looks at this seem fine.  However, there are seemingly endless possible edge case scenarios.  Of which I am not even sure about what is expected.  One I just ran goes as follows:

1) ensure you have a Bookmark synced that lives in the Mobile Bookmarks folder on desktop.
2) On your mobile device, remove that bookmark.  Sync Now.
3) On your desktop, move the same bookmark out of the mobile bookmarks folder to the BM toolbar.  Sync Now.

Observed results:
Upon completion of the sync in step 3, the bookmark is removed from the desktop BM toolbar.

Expected Results:
I don't know if what was observed is correct or not.  I think it is? Does client track when a change was made to a bm?  Should the move that happened later in time override the earlier remove?  (I suppose that is a general Sync reconciling question, not mobile specific)

I can probably come up with these timing based testing situations all day long.  I'm not sure how "real world" they are though because of proximity and constantly forcing manual syncs.  For example, if I do a bunch of stuff on mobile. Then come home and start up my desktop FX, Instant Sync will more than likely take place just after start up and before I can actual get in and create these odd timing issues.

That said, there is value in delving into edge cases to find bugs that may possibly affect real users.  But based on what I've seen so far today, this is looking good.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Tracy Walker [:tracy] from comment #4)
> Initial looks at this seem fine.  However, there are seemingly endless
> possible edge case scenarios.

Indeed.

> I don't know if what was observed is correct or not.  I think it is?

As you note, that depends on the client's reconciling behavior (which is not specified as part of the Sync protocol).

In this case, the default behavior on the desktop is (according to the Firefox 13 code, at least) to remove the local record if the remote is deleted and the local record is not modified. Otherwise, the latest record wins. It's possible that the server time is sufficiently far ahead of your desktop that the timestamp was ahead, or it's possible that there's a bug on the desktop. (Or, of course, that I misunderstand its behavior.)

I'm mostly concerned with things being not being insane. That is, if you deleted a record and it got duplicated, or a different record was deleted, etc., then I'd regard that as a blocker. A record being deleted and that deletion syncing is probably acceptable.

That said, if you can reliably reproduce an old deletion overriding a newly modified record, I'd be happy to get a bug report with logs!

> (I suppose that is a general
> Sync reconciling question, not mobile specific)

Correct, but the logic between the two is unlikely to be 100% identical.

> That said, there is value in delving into edge cases to find bugs that may
> possibly affect real users.  But based on what I've seen so far today, this
> is looking good.

Glad to hear it! Thanks for testing.

Comment 6

[Richard Newman [:rnewman]]

Calling this a meta bug, because a spade is a spade.

The only outstanding work is Bug 724740.

The only outstanding known weakness is in a situation where a parent and all of its children are not encountered in the same session; positioning logic does not cross session boundaries, so if:

* a folder is seen in one Sync, and
* one of its children is not encountered until a subsequent sync, and
* the folder is not seen again in that subsequent sync (which it will be if a child is added or moved), then

that child might be mis-positioned.

An alternative presentation is very similar: if all children are not seen in one sync, then Android Sync will collapse the children of that folder and reupload the folder record. The desktop with all the children will reposition them, re-adding the ones that Android Sync didn't see, but possibly damaging the order. Android Sync will then see the folder again, returning to synchronization with the desktop, but with children out of order.

This is a very unlikely situation (you'd need more than five thousand bookmarks to encounter it), so I'm in no hurry to fix it.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Not blocking on meta bugs. If any blocker to this isn't marked blocking, please nom