Last Comment Bug 415389 - All tags removed when moving a bookmark in the Bookmarks folder structure (cut & paste)
: All tags removed when moving a bookmark in the Bookmarks folder structure (cu...
Status: VERIFIED FIXED
: dataloss
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: P2 major with 2 votes (vote)
: Firefox 3
Assigned To: Dietrich Ayala (:dietrich)
:
Mentors:
: 419499 421697 (view as bug list)
Depends on: 384370
Blocks: 417228
  Show dependency treegraph
 
Reported: 2008-02-02 15:31 PST by Cyriel van 't End
Modified: 2011-08-27 06:33 PDT (History)
14 users (show)
mbeltzner: blocking‑firefox3+
mozaakash: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (7.62 KB, patch)
2008-03-17 14:59 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix (7.72 KB, patch)
2008-03-17 16:24 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix v3 (7.44 KB, patch)
2008-03-26 13:06 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix v4 (7.57 KB, patch)
2008-03-26 13:49 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Review
fix v5 (7.86 KB, patch)
2008-03-26 16:42 PDT, Dietrich Ayala (:dietrich)
asaf: review+
Details | Diff | Review

Description Cyriel van 't End 2008-02-02 15:31:11 PST
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) 2008-02-03 02:09:02 PST
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 2 [:Aleksej] 2008-02-26 13:35:13 PST
*** Bug 419499 has been marked as a duplicate of this bug. ***
Comment 3 Kevin Brosnan 2008-03-08 08:04:35 PST
*** Bug 421697 has been marked as a duplicate of this bug. ***
Comment 4 Dietrich Ayala (:dietrich) 2008-03-17 14:59:10 PDT
Created attachment 310080 [details] [diff] [review]
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) 2008-03-17 15:02:21 PDT
Comment on attachment 310080 [details] [diff] [review]
fix

not ready, need to adjust index values
Comment 6 Dietrich Ayala (:dietrich) 2008-03-17 16:24:00 PDT
Created attachment 310114 [details] [diff] [review]
fix
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-19 15:25:39 PDT
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) 2008-03-19 18:21:10 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-25 01:27:38 PDT
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) 2008-03-25 21:59:58 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-26 01:18:16 PDT
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) 2008-03-26 08:51:31 PDT
(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) 2008-03-26 13:06:22 PDT
Created attachment 311851 [details] [diff] [review]
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-26 13:12:55 PDT
I would rename aInlineTags to something more generic, there are few more places i would use that info. Maybe aIsUICommand?
Comment 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-26 13:14:29 PDT
+      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) 2008-03-26 13:49:55 PDT
Created attachment 311864 [details] [diff] [review]
fix v4

addresses comments 14 and 15.
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-26 14:22:07 PDT
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) 2008-03-26 16:42:16 PDT
Created attachment 311911 [details] [diff] [review]
fix v5

sorry, my misunderstanding. this adds filtering to properly handle that case.
Comment 19 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-26 17:20:08 PDT
Comment on attachment 311911 [details] [diff] [review]
fix v5

r=mano
Comment 20 Dietrich Ayala (:dietrich) 2008-03-26 18:59:11 PDT
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] 2008-04-29 13:32:44 PDT
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] 2009-01-25 16:28:01 PST
Test case https://litmus.mozilla.org/show_test.cgi?id=7478 has been created in litmus for regression testing.
Comment 23 Henrik Skupin (:whimboo) 2009-01-26 02:43:11 PST
(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] 2009-01-26 09:40:28 PST
Sure, created test case https://litmus.mozilla.org/show_test.cgi?id=7479 for rel3.0 regression testing
Comment 25 Gervase Markham [:gerv] 2009-11-26 06:43:21 PST
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 2011-08-26 23:02:47 PDT
Bug present in FF6 !
Comment 27 Cyriel van 't End 2011-08-27 03:22:03 PDT
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] 2011-08-27 06:33:38 PDT
another bug has already been filed on the new issue, correctly, this should never ever be reopened.

Note You need to log in before you can comment on or make changes to this bug.