May be related to Bug # 84920 (that's how it manifested for me -- really slow closing windows). I have been managing a moderate sized set of bookmarks (128K) recently, for the first time, in Mozilla's bookmark manager. Trying to track down why performance recently was sluggish I thought I'd look at file sizes in the profile directory. 'lo and behold my bookmarks file had burgeoned to 4MB! Further investigation showed that for some reason three particular bookmarks had been replicated over and over and over. Resultant bad bookmarks file and file after cleanup can be provided. I apologize that no further pre-bug or tracing information of any kind can be provided. This did not crash Mozilla at any time, but significantly reduced the user experience.

sounds like a duplicate of bug 80191

I have just closed bug 80191 as duplicate of bug 74969 which is already more than a month old, so this must be a new issue. Chris, what build/milestone are you using? Please *always* include this information in your bugreports as it will be worthless otherwise. Thanks.

I just saw this in a recent nightly build (Build 2001062504 win32). I find that it seems to happen after I "manage" a bookmark (i.e. drap and drop bookmarks, copy and paste bookmarks). I find that it sometimes make multiple copies in the bookmarks.html file when I copy and paste to the same folder multiple times, and sometimes the bookmarks.html file just grows on its own! After this happened Moz became rather unstable (i.e. crash&freeze at startup, at exit and randomly at bookmark access).

1) The bookmark manager duplicate entries in the bookmarks.html files when one ctrl-drag a bookmark entry. 2) ctrl-dragging bookmark folders duplicate entire folders too. 3) sometime ctrl-dragging cause weird duplicates like when I ctrl-drag a folder, the entries under the folder gets duplicated. 4) sometimes ctrl-dragging cause other bookmarks to duplicate thousands of times. I can reproduce 1 and 2 reliably, but 3 and 4 is very hard to duplicate.

Forgot to mentioned that I tested the above using build 2001062815 win32 0.9.2 branch

copy-paste also causes the same effect

I was not able to reproduce the duplicates from basic's comment. Linux, 2001062906 Moz0.9.2 branch

I am seeing #1 from _basic@yahoo.com's 2001-06-29 13:33 list, using a linux tip build from this evening. Here's how I reproduced it: 1. Choose bookmarks->Add bookmark 2. Bring up bookmark manager 3. Right click on the new bookmark, and choose copy 4. Right click, then paste. Notice: only 1 copy of the bookmark appears onscreen, but there's 2 in the bookmarks.html file. 5. Right click on the new bookmark, then delete. Notice: it's not on the screen, but there's still 2 in the bookmarks file. 6. Close bookmark manager 7. Open bookmark manager Notice: the bookmark is back, but only 1 entry in the file. 8. Delete the bookmark, and now it's really gone. Note: I have my new bookmark folder still at the default.

*** Bug 89770 has been marked as a duplicate of this bug. ***

*** Bug 88948 has been marked as a duplicate of this bug. ***

*** Bug 88910 has been marked as a duplicate of this bug. ***

Raising severity to critical since mozilla won't start once the bookmarks file has grown enough. Platform -> All as per the duplicates on Mac platform. I am changing the summary to ease spotting the bug and avoid duplicates. I have nominated the bug for 0.9.3 and nsCatFood since this is very annoying and may keep people from using mozilla.

nav triage team: This isn't going to make it for the mozilla0.9.3 bus, marking nsbeta1+, mozilla1.0 and p2

moving up to mozilla0.9.5, as it manifests as a perf problem.

*** Bug 94860 has been marked as a duplicate of this bug. ***

*** Bug 97367 has been marked as a duplicate of this bug. ***

Mass-moving lower-priority 0.9.5 bugs off to 0.9.6 to make way for remaining 0.9.4/eMojo bugs, and MachV planning, performance and feature work. If you disagree with any of these targets, please let me know.

This stopped me dead in my tracks today, although I only tried the steps to reproduce a few times, 3 days ago. Today, my bookmarks file somehow grew to 10 times the size it was yesterday, and is now 2.7MB, which causes the app to become unresponsive at startup (Win98). The extra bloat is a few bookmarks which appear to have been duplicated thousands of times each. I'm pretty sure they are the ones I used to repro this on 9/16. I didn't realize that this could happen suddenly, I thought it was very gradual, upon repeating steps to repro many times. Moving back to 095, nominating for nsbranch. Claudius, care to risk a bookmarks file on this, and opine as to whether it is worthy of plussing?

*** Bug 100699 has been marked as a duplicate of this bug. ***

This also caused my bookmarks file to be truncated, I lost thousands of bookmarks :`-(

Hmmm, now that it seems evident that the gross misbehavior starts out and looks rather less innocent (1 for 1 dupes after manipulating BM's) before suddenly becoming the huge performance mess I'd say yes, this bug is worth an nsbranch plus. here's a big BM file to play around with. You should take a look peter: http://mozilla.org/quality/browser/front-end/testcases/bookmarks/large-Bookmarks.html

Thanks Claudius, nsbranch+

investigating today.

I've not yet been able to reproduce the massive growth, but all steps to reproduce point towards the duplication of bookmarks in the HTML during copy- paste operations. (Ctrl+Drag is the same as a copy-paste) So I've patched those operations to prevent duplicates being created if a paste occurs into the same folder as the source, along with adding a comment saying that it's because of our nonexistent handling of duplicate bookmarks that is my suspected cause of this woe.

Can someone who can see this try this patch and let me know if it works? Or provide me with a faulty bookmarks file?

(I've verified that this patch removes the creation of excess bookmarks during pastes into the source folder)

Ben, could you tar up something for Claudius to test (unless he can handle the patch)? I'm not convinced that this only happens on copy/paste/ctrl-drag, since for me the growth occurred well after the last such op.

> I'm not convinced that this only happens on copy/paste/ctrl-drag, since for me > the growth occurred well after the last such op. I think copy/paste/ctrl-drag could have somehow triggered something that causes this. When I first discovered this problem I thought it was regression but after some testing I found out that if I don't copy/paste/ctrl-drag between different folders (and back and forth), it doesn't happen.

I agree, the copy/paste/ctrl-drag seems to be a necessary step, but the problem doesn't show up at that time.

The effect of a duplication into an existing folder is not user visible. It is my thinking that the effects are seen later. Updated .jar file containing the patch coming in a few minutes...

adding PDT for review

Ben, I'm not sure what you mean is not user-visible. The end effect of my bookmarks file growing by an order of magnitude, and the app failing to launch, is quite noticeable. Both of these happened 2 days after the operation. We need to get this bug prepped for landing, which means QA testing it, reviews, possibly even landing and baking on the trunk. Please drive this like you would drive Sylvia, or it won't make it into the branch.

This needs an ETA date in the status whiteboard today.

Comment on attachment 50448 [details] [diff] [review] patch to copy paste operations to prevent duplicate bookmarks r=jag

Chris - We need an sr=, so we can check this one in today - PDT.

Comment on attachment 50448 [details] [diff] [review] patch to copy paste operations to prevent duplicate bookmarks sr=waterson

what are the chances of getting this in for 0.9.4?

check it in - PDT+

0.9.5 is out the door. bumping TM up by one.

Fixes checked in to trunk and .9.4 branch.

So here's another diff. I don't think my previous patch was enough coverage. Tonight, I finally (involuntarily) reproduced this bug while working on some other bugs. Magic doubling bookmarks file and all. Not wanting to be beaten, I wrote this new patch. Explained: Many moons ago, rjc realized that duplicate bookmarks would be a PITA, so he added some code to the bookmark parser that prevented against adding duplicate bookmarks (to fix bug 51021). This code essentially did the following: for each bookmark "A" about to be added to a container "C", scan all the contents of "C" to see if "A" already exists. This was a O(n^2) algorithm and was removed to save some startup time. The current code has no mechanism for preventing against duplicate bookmarks, other than vigilance in the FE. As this bug shows, there's not been much vigilance in the FE ;) (I'm to blame). I'm now proposing a solution that can kill two birds with one stone. Here's what I've done in this patch: - added a back-arc out of bookmarks to their parent container. - added two special methods to nsIBookmarksService - insertInFolder and removeFromFolder. These methods insert a bookmark into a folder (preventing against duplicates, and ensuring the back arc is created) and remove a bookmark (and its back arc) respectively. The code in insertInFolder does this: when a bookmark "A" is about to be inserted into folder "C", 1) obtain a list of all its parents (RDF "atomizes" resources, so a single bookmark resource can be in multiple folders 2) iterate over this set of parents, comparing to "C" 3) if one matches, "A" already exists in "C", so don't add it again. This should be faster than the system used originally because bookmarks typically have fewer parents than folders have children (so the list to scan is smaller). It's still slower than it is now by nature of the fact that it is inserting more code, but it represents a critical safety measure, IMO. The other question is bloat. I am unsure as to how much is added by all these back arcs. I will investigate today/tomorrow. The patch does work, however, I created a rogue bookmark file with about a hundred copies of the same bookmark in a particular folder, and the code presented here only used the first, ignored the second, and then repaired the bookmark file when the application quit. After this patch goes in, the best followup would be to modify all FE code in js that inserts/removes items from folders and make them use the two methods I've added in this patch. This ensures that all insertion/removal goes through the same code and reduces the likelihood of error. The other bird killed by this, by the way, is a less important bug where a user is prevented from deleting bookmarks displayed in search results because as far as the view is concerned, the "parent" of the bookmark is the search result root, not the folder the bookmark is actually in. Without knowing what that folder is, it's impossible to delete (or perform other actions on) the bookmark from within the search view. A back arc imparts that knowledge. There are other potential benefits, such as reducing the coupling between the RDF data model and the view (treeview currently), where the FE code currently has to crawl around a content model to find parents, etc.

tingley has fixed nsIRDFContainer::IndexOf() so that it should now be O(1); see bug 104328. So perhaps we could just revert rjc's fix?

Reinstate rjc's old code. I'd still like to investigate the back-arc stuff (for the purpose of fixing other bugs) but since it's no longer a requirement of this bug, it can wait.

Removing the + for the time being, given that there might be another fix that better addresses this problem, which has not been addressed.

PDT-, would've loved to haved the last patch earlier. Sorry, but pls try and get it into the trunk, once waterson has checked in his patch for the performance issue. Let's stay where we are at right now.

So can I assume sr=waterson for this last patch?

Comment on attachment 53644 [details] [diff] [review] patch using IndexOf sr=waterson

tingley, ben was seeing some problems with hanging on startup when he attempted to alter attachment 53644 [details] [diff] [review] to use nsIRDFContainerUtils directly (for the branch). Could you do a quick sanity check and verify that this works correctly with the patch from bug 104328?

Sure, I'll take a look.

Thank goodness for Ben Goodger. There's an infinite loop in RDFContainerUtilsImpl::IndexOf(). The |continue| at http://lxr.mozilla.org/mozilla/source/rdf/base/src/nsRDFContainerUtils.cpp#533 should be a |break|. Making this change fixes the hang -- I'll include this in a new version of the patch on bug 104328. Is this code being called from anywhere else? I wouldn't have expected a bug like this to survive for so long.

I try ;)

Comment on attachment 53644 [details] [diff] [review] patch using IndexOf r=pchen

Paul Chen is now taking Bookmarks bugs. For your convenience, you can filter email notifications caused by this by searching for 'ilikegoats'.

Fix checked in.

*** Bug 105810 has been marked as a duplicate of this bug. ***

marking VERIFIED

*** Bug 101307 has been marked as a duplicate of this bug. ***