415389 - All tags removed when moving a bookmark in the Bookmarks folder structure (cut & paste)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Cyriel van 't End]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020204 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020204 Minefield/3.0b3pre

All tags associated with a bookmark are removed when moving an item from the root of the Bookmarks Menu folder to another folder.

Reproducible: Always

Steps to Reproduce:
1. Create a bookmark in the Bookmark Menu folder
2. Add some tags to the bookmark
3. Create another folder within the Bookmark Menu folder
4. Move the bookmark to the folder which was just created.
Actual Results:  
Bookmark is moved without the given tags

Expected Results:  
Bookmark is moved with the given tags

Experienced this issue when moving items using Cut and Paste. Not sure if this happens in other situations (Drag and drop doesn't seem to work in this build aswell)

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020223 Minefield/3.0b3pre

At the moment he only way to drag a bookmark into a folder is in the sidebar BTW. No problem migrating the tag.
The problem occurs only while trying to cut & paste. 

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: fix

de/serializes tags as a property of the bookmark. this has a few benefits:

1. complete partial serializations: tags are carried around w/ the item they describe.
2. backup size: currently we serialize the tag root, where a uri is serialized once for for each tag it has. this means that a bookmark with 3 tags is saved in the backup as 4 bookmarks, inflating the size of the backup, and slowing down import and export.
3. restore perf: instead of tagging the URI once for each tag, we can now make one call to the tag service for all a uri's tags. maybe a few less db queries, but certainly less xpconnect traversal.

notes:

1. adds exclude filtering for json export, which could be useful to extension consumers using these methods for sync, etc.

2. older backups since json landed will still work, since we still handle import of the tag root, if present.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 310080 [details] [diff] [review]
fix

not ready, need to adjust index values

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: fix

Comment 7

[Asaf Romano (gone)]

Comment on attachment 310114 [details] [diff] [review]
fix

Dietrich, doesn't this cause us to remove tags as you *redo* item removal? That can be fixed by only setting the tags property if you're removing the last bookmark for the bookmark's url.

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> (From update of attachment 310114 [details] [diff] [review])
> Dietrich, doesn't this cause us to remove tags as you *redo* item removal? That
> can be fixed by only setting the tags property if you're removing the last
> bookmark for the bookmark's url.
> 

No it doesn't cause tags to be removed when doing redo item removal. The child transactions (adding the tags) are executed yet again.

Comment 9

[Asaf Romano (gone)]

I still don't think it's right. We should export the tags-root *as-is* and don't re-tag after restoring from backup.

Comment 10

[Dietrich Ayala (:dietrich)]

Per irc, i looked at only serializing tags inline for cut. However, it suffers from the same problem: Annotations on tag items will be lost. Our current approach towards tags in the transaction layer has the same problem. Basically any tag removal via the tagging API incurs the loss of annotations on the "tag bookmarks".

I think we need to document that the tag root is "special internal use", and can be modified out-of-band, so extensions should not use it directly. Use the tagging API instead. And in 3.1 we can fix bug 424160, moving tags out of the bookmark tree.

Comment 11

[Asaf Romano (gone)]

Dietrich, this isn't just annotations, this is alsp a speed issue and may lead to different Tags Root in certain cases, I'm sure. I would really like not to change our current, pretty cheap, solution with something that heavy.

Comment 12

[Dietrich Ayala (:dietrich)]

(In reply to comment #11)
> Dietrich, this isn't just annotations, this is alsp a speed issue and may lead
> to different Tags Root in certain cases, I'm sure. I would really like not to
> change our current, pretty cheap, solution with something that heavy.
> 

I'm ok with backup/restore the whole root for now. My point is that regardless of how we fix this bug, we still need to discourage extension developers from annotating tag bookmarks, due to the dataloss problem.

How could this lead to a different tags root?

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

- always backup tag root
- always restore tag root if present
- only inline tags for copy/drag
- always restore inline tags if present

Comment 14

[Asaf Romano (gone)]

I would rename aInlineTags to something more generic, there are few more places i would use that info. Maybe aIsUICommand?

Comment 15

[Asaf Romano (gone)]

+      var tags = aData.tags.split(", ");
+      childTxns.push(this.ptm.tagURI(itemURL, tags));

We should filter the incoming tags list here, otherwise undoing this transaction will remove old tags as well.

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: fix v4

addresses comments 14 and 15.

Comment 17

[Asaf Romano (gone)]

I don't understand how's that addressing comment 15.

basically, here's my scenraio:
1. have two bookmarks a, b for the same uri, set with some tags
2. copy&paste b
3. undo paste b

Are the tags for a still set?

Comment 18

[Dietrich Ayala (:dietrich)]

Attachment: fix v5

sorry, my misunderstanding. this adds filtering to properly handle that case.

Comment 19

[Asaf Romano (gone)]

Comment on attachment 311911 [details] [diff] [review]
fix v5

r=mano

Comment 20

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.128; previous revision: 1.127
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.8; previous revision: 1.7
done

Comment 21

[Tracy Walker [:tracy]]

Verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/20080429006
Minefield/3.0pre

Comment 22

[Aakash Desai [:aakashd]]

Test case https://litmus.mozilla.org/show_test.cgi?id=7478 has been created in litmus for regression testing.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #22)
> Test case https://litmus.mozilla.org/show_test.cgi?id=7478 has been created in
> litmus for regression testing.

Aakash, could you please also add it to the 3.0 testrun, since it has been fixed for Firefox 3? It would be great.

Comment 24

[Aakash Desai [:aakashd]]

Sure, created test case https://litmus.mozilla.org/show_test.cgi?id=7479 for rel3.0 regression testing

Comment 25

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 26

[Dimitri]

Bug present in FF6 !

Comment 27

[Cyriel van 't End]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

This issue indeed seems to have been regressed into the latest release version of Firefox 6.

Comment 28

[Marco Bonardo [:mak]]

another bug has already been filed on the new issue, correctly, this should never ever be reopened.