Closed Bug 627497 Opened 12 years ago Closed 11 years ago

Bookmark sync in Firefox 3.5/3.6: copied items need to get their own GUIDs

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: philikon, Assigned: rnewman)

References

Details

Attachments

(2 files)

When you copy a folder or bookmark in the Organize Bookmarks UI, annotations will be copied over the new item as well. That means on Firefox 3.5/3.6, the copies will have the same GUID. Confusion ensues.

So when a new bookmark is added and it already has a GUID on 3.5/3.6, it's probably a good idea to check whether there are two bookmarks with that GUID now. If so we want to assign a new GUID to the newer one. To fix people's existing databases, we want to extend idForGUID to look at all results on 3.5/3.6 (and also retrieve the created date) and do the same if we find more than one record with the same GUID.
Blocks: 621584
I have a patch for this in my local queue, which passes manual testing (read: poking at places.sqlite). Will attach when I have automated tests, or before if I run out of time.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
This one's pretty self-explanatory!

Applies on top of 

 Bug 627490
 Bug 626796
Attachment #506064 - Flags: review?(mconnor)
Whiteboard: [has patch][needs review mconnor]
Comment on attachment 506064 [details] [diff] [review]
Proposed patch. v1

>+      // Order results by lastModified so we can preserve the ID of the oldest bookmark.
>+      // Copying a record preserves its dateAdded, and only modifying the
>+      // bookmark alters its lastModified, so we also order by its item_id --
>+      // lowest wins ties. Of course, Places can still screw us by reassigning IDs...
>       stmt = this._getStmt(
>         "SELECT a.item_id AS item_id " +
>         "FROM moz_items_annos a " +
>         "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " +
>         "WHERE n.name = '" + GUID_ANNO + "' " +
>-        "AND a.content = :guid");
>+        "AND a.content = :guid " +
>+        "ORDER BY a.lastModified, a.item_id");

I had to make sure that a.lastModified actually gets set, but it seems that it does. Otherwise we should sort by moz_bookmarks.dateAdded which would require another JOIN. So, yay, good solution!

>+// Copying a bookmark in the UI also copies its annotations, including the GUID
>+// annotation in 3.x.
>+function test_copying() {

Maybe test_copying_avoid_duplicate_guids()?


This is a good part 1. We also need part 2 where we make sure that copied items are getting a new GUID right after being created. Otherwise we'll be tracking the wrong object. It might be as simple as checking that itemId doesn't already have a GUID assigned in tracker.onItemAdded() and if it does, assign a new one (need to check that new items don't already have a GUID on 3.5/3.6 when onItemAdded() is fired.)
Attachment #506064 - Flags: review?(mconnor) → review+
Whiteboard: [has patch][needs review mconnor] → [has patch][has review][needs part 2]
Doesn't block gecko 2.0 but is needed for the 1.6.x add-on.
Target Milestone: --- → 1.6
Here's my WIP for monkeypatching the copy function, just for future reference.

I don't know if this also tackles folders, because...

I also haven't tested it, because I still haven't figured out the appropriate Components incantation to get hold of PlacesUIUtils in 3.6, and I'm not going to spend the time to do so right now.

Here's the mxr link:

http://mxr.mozilla.org/mozilla1.9.2/source/browser/components/places/content/utils.js#85

mak had this to say:

  11:31:35 < mak> rnewman: so copy uses _getBookmarkItemCopyTransaction... drag&drop instead iirc use  wrapNode to convert in json

but I don't know if that applies to 3.x.
Pushed:

  default: http://hg.mozilla.org/services/fx-sync/rev/5476099741f5
  1.6.x:   http://hg.mozilla.org/services/fx-sync/rev/88e7acfb0ad4

Closing this; will file a followup for part 2.
Blocks: 627511
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 628788
Whiteboard: [has patch][has review][needs part 2]
copied a BM toolbar folder and pasted it to the BM menu.  renamed the folder.  sync to cloud, sync to client b. the folder doesn't appear on client b's BM menu.

reopening per irc with philikon.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
two scenarios with different results.  The  first may be the follow up bug

STR's 1)
With 1.6.3 installed on 3.6.13:
1) In the Library, Copy a folder that contains a few children from the BM toolbar
2) Paste the folder to another subfolder on the BM toolbar
3) rename the folder copy
4) sync to another client (tested with 1.6.3 on 3.5.16)

tested results:
the copied/renamed folder exist on client B, but its children do not

expected results: the children exist

Scenario 2)
With 1.6.3 installed on 3.6.13:
1) In the Library, Copy a folder that contains a few children from the BM toolbar
2) Paste the folder to the BM menu
3) rename the folder copy
4) sync to another client (tested with 1.6.3 on 3.5.16)

tested results:
The folder doesn't exist on client B

expected results: the folder and its children exist on client B
(In reply to comment #8)
> two scenarios with different results.  The  first may be the follow up bug

Looking now.
Tracy, I believe this is fixed by Bug 628788. Would you please re-verify when that lands somewhere you can run?
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
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.