486481 - Weird behavior when syncing the bookmark with tags

Status: RESOLVED FIXED

(Cloud Services :: General, defect)

Description

[mishail.mishail]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 (.NET CLR 3.5.30729)
Build Identifier: Weave 0.3

When I sync bookmark with tags it appears incorrectly at the other PC

Reproducible: Always

Steps to Reproduce:
Reproduced on clean/new profile with default Firefox's bookmarks only on 2 PCs

1. Install weave 0.3
2. Make an initial sync (bookmarks - only default ones from Firefox installation). Both PCs have bookmarks as displayed at home_before.png and home_tags_before.png
3. Add any bookmark with tags at 1st PC. In the example I used Weave URL from AMO with tags "firefox, weave, addon"
4. Make a sync

Actual Results:  
The bookmark is correctly added only at 1st PC (see home_after.png). On the 2nd PC bookmarks are "broken",  see work_after.png

Expected Results:  

The same bookmarks at both PCs (as displayed at home_after.png)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 (.NET CLR 3.5.30729)

At both PCs

Comment 1

[mishail.mishail]

Attachment: The default bookmarks for new profile

Comment 2

[mishail.mishail]

Attachment: Empty "recent tags" for the fresh profile

Comment 3

[mishail.mishail]

Attachment: Home PC. After sync with newly added bookmark with tags. Correct behavior

Comment 4

[mishail.mishail]

Attachment: Work PC. After the sync with newly added bookmark with tags from Home PC. ERROR

Comment 5

[mishail.mishail]

Attachment: "weave" folder from Home PC. Logs. Snapshots.

Comment 6

[mishail.mishail]

Attachment: "weave" folder from Work PC. Logs. Snapshots.

Comment 7

[mishail.mishail]

I guess it could be related to "smart bookmarks" (Recently bookmarked and Recent tags) in the Bookmark menu. Those are bookmarks with places query as URL (http://forums.mozillazine.org/viewtopic.php?p=3260477), but they are displayed as "folders" to user.

It seems that Weave treat them as plain folders too, and tries to update their content.

Comment 8

[mishail.mishail]

One more observation. Issue seems to be related to "Recent tags" query and happens when new tag appears in the query's results. It's displayed as a new folder. May be Weave should check the parent's type for folder and skip those folders with RESULT_TYPE_QUERY as parent?

Comment 9

[mishail.mishail]

Attachment: Check fot "tags" root

I've added checking of "tags" folder (nsINavBookmarksService.tagsFolder) as one of the roots, besides with  "menu", "toolbar", "unfiled".

It seems to be working for me... I will test on clean profile

Comment 10

[mishail.mishail]

BTW, should weave check "places" (placesRoot) also?

Comment 11

[mishail.mishail]

Attachment: Bookmarks.js with checks for root and tags

Comment 12

[mishail.mishail]

Comment on attachment 370634 [details]
Check fot "tags" root

Doesn't fix

Comment 13

[mishail.mishail]

Comment on attachment 370641 [details]
Bookmarks.js with checks for root and tags

Doesn't fix. I give up

Comment 14

[Ed Lee :Mardak]

It seems like we're syncing the tags folder perhaps? Firefox internally stores tags as bookmarks, so we might accidentally be syncing them like any other bookmark folder.

But some reason I don't see any of this strangeness on my regular weave profile. On a new test profile, I do see a lot of strangeness like multiple synced bookmarks for a bookmark tagged multiple times.

Comment 15

[Ed Lee :Mardak]

Mikhail, could you attach a patch containing the changes you've made instead of the whole file?

Did you grab the source or are you modifying things from the extension .xpi?

Comment 16

[Ed Lee :Mardak]

Attachment: v1

mishail: I've made your changes into a patch, but what did you mean by it doesn't work? I just synced with a fresh profile and I don't see tags folders appearing on the other profile with your patch.

I still see duplicate bookmarks though.. but that might be a separate issue.

Comment 17

[Dan Mills [:thunder]]

The correct thing to do here is not to try to sync the contents of the tags toplevel folder.  Instead, we should *ignore* it.

In Weave, tags are stored as a field inside each bookmark record.  In places, they appear as pseudo-bookmarks under tag folders.  If we sync these pseudo bookmarks they will end up duplicated on the other end.

The fix is in the tracker, I think.  If the parent or grandparent of the item just changed is the tags folder, simply ignore it.

Comment 18

[mishail.mishail]

Attachment: Check the parent and grand parent in Tracker

Could someone review?

Comment 19

[mishail.mishail]

Comment on attachment 371010 [details] [diff] [review]
Check the parent and grand parent in Tracker

Wrong format

Comment 20

[mishail.mishail]

Attachment: Check the parent and grand parent in Tracker. v2

New version (hg diff -p -U 8 used)

Comment 21

[mishail.mishail]

BTW, there are no checks either for Livemarks or for Tags in BMT_onItemMoved. Is that correct?

Comment 22

[Dan Mills [:thunder]]

(In reply to comment #20)
> Created an attachment (id=371016) [details]
> Check the parent and grand parent in Tracker. v2
> 
> New version (hg diff -p -U 8 used)

Thanks, I much prefer it like that.

It still has some of your previous additions, btw.  I'll review the rest shortly.

(In reply to comment #21)
> BTW, there are no checks either for Livemarks or for Tags in BMT_onItemMoved.
> Is that correct?

I think for livemarks it is correct.  If you move a livemark out of the livemark container it would mean you intend to keep it, so we should treat it like a normal bookmark.  Though, I guess if you move it within the container (presumably accidentally) we should ignore it.

For tags, that should theoretically never happen, since those items are never moved, only created and removed internally by Places.

Comment 23

[mishail.mishail]

Attachment: Call untagURI on bookmark before removing that.

New version of patch. Added untagURI in BStore_remove. It seems that _bms.removeItem could leave empty tags...

Comment 24

[mishail.mishail]

(In reply to comment #22)
> It still has some of your previous additions, btw.

If you are talkign about those ones from https://bugzilla.mozilla.org/attachment.cgi?id=370850&action=diff, then I think they are needed... I tried to remove all changes listed in https://bugzilla.mozilla.org/attachment.cgi?id=370850&action=diff from https://bugzilla.mozilla.org/attachment.cgi?id=371016&action=diff and got incorrect sync with some strange follders in the Bookmarks menu.

Comment 25

[Dan Mills [:thunder]]

Comment on attachment 371025 [details] [diff] [review]
Call untagURI on bookmark before removing that.

This patch looks good to me.

Comment 26

[Ed Lee :Mardak]

Comment on attachment 371025 [details] [diff] [review]
Call untagURI on bookmark before removing that.

>+    // Skipping for tags    
>+    if ((folder == this._bms.tagsFolder) || 
>+        (this._bms.getFolderIdForItem(folder) == this._bms.tagsFolder)){
Why do you check both if the folder is the tags folder or if it's in the tags folder?

Comment 27

[mishail.mishail]

(In reply to comment #26)

> Why do you check both if the folder is the tags folder or if it's in the tags
> folder?

As per Dan:

>The fix is in the tracker, I think.  If the parent or grandparent of the item
>just changed is the tags folder, simply ignore it.

Comment 28

[Ed Lee :Mardak]

Ah right. Because each tag has its own id with the parent set to an actual tag with a name which is contained in the tags folder.

Is there a particular reason why you added the placesRoot checks?

Comment 29

[mishail.mishail]

(In reply to comment #28)
> Is there a particular reason why you added the placesRoot checks?

Well, it's listed in the "moz_bookmarks_roots" along with "menu", "toolbar", "tags" and "unfiled". So I decided to add that check.

I didn't test behavior with tagsFolder checks without placesRoot checks.

However in the comment #26 I said that I had observed wrong folders in the bookmarks menu afters sync. There were "History" and "All Tags" folders afters sync, which are children of placesRoot

Comment 30

[mishail.mishail]

>>However in the comment #26 I said that I

Sorry, in the comment #24

Comment 31

[mishail.mishail]

If patch is OK then could someone check in that?

Comment 32

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/5bfdbac91166

Add the remaining special top level folders and ignore them for certain behavior like getAllIDs and wipe. Have the tracker ignore changes to things in the tags folder.