623418 - Bookmark sync: don't record children in annotation, use single SQL query

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

In the attempt of solving the erroneous bookmark ordering problem, we were digging through the logs and bookmark backups (JSON) of a particular user and noticed _reorderChildren was failing because the it was referring to GUIDs that didn't exist. In fact, they didn't exist anywhere as a bookmark, just within the children annotation of that particular folder.

After investigating some more, we found that we're not updating the children annotation correctly in all cases, e.g. when the local folder that we're duping to has more children than upstream.

I think we should not bother with the annotation. It was meant as an optimization, but a) we don't actually know that it *is* an optimization and b) there are only two hard problems in computer science: cache invalidation and naming things.

If we're concerned about performance, we can use a single SQL query to compute the list of child GUIDs for a record.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): don't record children in an annotation

Get rid of the annotation and just fetch the children every time we create a folder record. (This may be a bit more expensive, but Part 2 should fix that.)

All crossweave tests pass again with this patch applied.

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): use a single SQL query to compute children

Use a single SQL query to compute the GUIDs of a folder's children. This will be pretty fast on trunk as Places assigns GUIDs to all items as they're created and we can just query the GUID column.

On 3.5/3.6 we have to JOIN the annotations table as usual. Also there might be "holes" in the GUID columns because Sync assigns the GUIDs here (usually during tracking) and Sync might not always done that by the time we sync (e.g. first sync). That's why we need to make sure we assign GUIDs to items that don't have one yet. This isn't a problem, we would have to do this anyway, it just won't be as fast as on trunk.

Comment 3

[Philipp von Weitershausen [:philikon]]

I'm pretty confident this fixes the reordering problems people have reported. While going through the code to produce the patches I discovered some more cases where we didn't try hard enough to invalidate the annotation (we wouldn't clean it up when users stopped using Sync, for instance).

On that basis, this bug should definitely block betaN and we should land it in 1.6.x too.

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 502364 [details] [diff] [review]
Part 2 (v1): use a single SQL query to compute children

Would be good to get sdwilsh to sign off here, as well.

sdwilsh, I owe you a whiskey.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 502364 [details] [diff] [review]
Part 2 (v1): use a single SQL query to compute children

>+      stmt = this._getStmt(
>+        "SELECT b.id AS item_id, " +
>+          "(SELECT id FROM moz_anno_attributes WHERE name = '" + GUID_ANNO + "') AS name_id," +
>+          "a.content AS guid " +
>+        "FROM moz_bookmarks b " +
>+        "LEFT JOIN moz_items_annos a ON a.item_id = b.id "+
nit: space before +

So glad this is a bazillion times easier on trunk now :)

r=sdwilsh

(In reply to comment #4)
> sdwilsh, I owe you a whiskey.
rowan's creek!

Comment 6

[Philipp von Weitershausen [:philikon]]

1.6.x:
https://hg.mozilla.org/services/fx-sync/rev/662c757bf3b9
https://hg.mozilla.org/services/fx-sync/rev/67e7e8b0c2b8

default:
https://hg.mozilla.org/services/fx-sync/rev/f0c52f7988ad
https://hg.mozilla.org/services/fx-sync/rev/6c67efeb94f3

Comment 7

[Tracy Walker [:tracy]]

what is the test case here; steps to repro?

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to comment #7)
> what is the test case here; steps to repro?

Here are two potential failure modes that I could think of:

* Have Sync set up and syncing with another profile. Choose "Stop Using This Account", add a few bookmarks, make a few changes, etc., set up Sync again on the profile, connecting to the same account.

* Set up Sync 1.5.1 syncing two profiles, lots of bookmarks. Upgrade one of them to 1.6, have it reupload everything. Make a few changes to the bookmarks on the other profile before you upgrade it to 1.6.

In both cases you may see some bookmarks being reordered. (It's a random thing so the more bookmarks you have the more chance you should have to see this.) This should be fixed now.

Comment 9

[Tracy Walker [:tracy]]

(In reply to comment #8)
> (In reply to comment #7)
> > what is the test case here; steps to repro?
> 
> Here are two potential failure modes that I could think of:
> 
> * Have Sync set up and syncing with another profile. Choose "Stop Using This
> Account", add a few bookmarks, make a few changes, etc., set up Sync again on
> the profile, connecting to the same account.
> 
OK, so I know I had been able to repro that in the past.

This is what I did to test again with 1.6.2:
1) while accumulating and adding bookmarks to the BM toolbar, sync three clients.
2) Stop using one of the clients. Then, on that client, add a "test" folder to the BM toolbar and move all the BM toolbar bookmarks into that folder.  
3) Add the client back to the same account as before.  Sync.

Tested results:  All the bookmarks are removed from the "test" folder and put back on the toolbar. Note: this is different from what I had previously been able to repro.  Before 1.6.2, I'd get a full "test" folder AND everything on the toolbar.  Is the new result what is expected?

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to comment #9)
> Is the new result what is expected?

Yes.

Comment 11

[Tracy Walker [:tracy]]

ok, verified then