Closed Bug 623418 Opened 13 years ago Closed 13 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: philikon, Assigned: philikon)

References

Details

(Whiteboard: [has patch][hardblocker])

Attachments

(2 files)

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.
Depends on: 617834
Blocks: 617834
No longer depends on: 617834
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.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #502362 - Flags: review?(mconnor)
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.
Attachment #502364 - Flags: review?(mconnor)
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.
blocking2.0: --- → ?
Whiteboard: [has patch][needs review mconnor]
Attachment #502362 - Flags: review?(mconnor) → review+
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.
Attachment #502364 - Flags: review?(sdwilsh)
Attachment #502364 - Flags: review?(mconnor)
Attachment #502364 - Flags: review+
blocking2.0: ? → betaN+
Whiteboard: [has patch][needs review mconnor] → [has patch][needs review mconnor][hardblocker]
Whiteboard: [has patch][needs review mconnor][hardblocker] → [has patch][needs review sdwilsh][hardblocker]
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!
Attachment #502364 - Flags: review?(sdwilsh) → review+
Whiteboard: [has patch][needs review sdwilsh][hardblocker] → [has patch][hardblocker]
Blocks: 625407
Blocks: 624982
Blocks: 545647
what is the test case here; steps to repro?
(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.
(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?
(In reply to comment #9)
> Is the new result what is expected?

Yes.
ok, verified then
Status: RESOLVED → VERIFIED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.