Closed Bug 87864 Opened 23 years ago Closed 23 years ago

Hang: Bookmarks file grows huge, is truncated

Categories

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

Product:
SeaMonkey
Component:
Bookmarks & History
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.6

People

(Reporter: chris, Assigned: paulkchen)

References

(
URL
)

Details

(Whiteboard: [PDT-] [willconflict:wgate094+] [Fix checked into 094 branch 10.15])

Attachments

(4 files)

patch to copy paste operations to prevent duplicate bookmarks
1.90 KB, patch
jag+mozilla
: review+
waterson
: superreview+
Details | Diff | Splinter Review
jar file containing fix
686.56 KB, application/octet-stream
Details
another fix
9.79 KB, patch
Details | Diff | Splinter Review
patch using IndexOf
1.15 KB, patch
paulkchen
: review+
waterson
: superreview+
Details | Diff | Splinter Review 
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
OS: Linux → All
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.
Severity: major → critical
Keywords: mozilla0.9.3, nsCatFood
Hardware: PC → All
Summary: Bookmarks Manager severely duplicated bookmarks → bookmarks duplicated, bookmarks file grows
nav triage team:

This isn't going to make it for the mozilla0.9.3 bus, marking nsbeta1+,
mozilla1.0 and p2
Keywords: nsbeta1+
Priority: -- → P2
Target Milestone: --- → mozilla1.0
moving up to mozilla0.9.5, as it manifests as a perf problem.
Target Milestone: mozilla1.0 → mozilla0.9.5
*** 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.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
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?
Keywords: mozilla0.9.3 → nsbranch
Summary: bookmarks duplicated, bookmarks file grows → Hang: bookmarks duplicated, bookmarks file grows huge
Target Milestone: mozilla0.9.6 → mozilla0.9.5
*** 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 :`-(
Summary: Hang: bookmarks duplicated, bookmarks file grows huge → Hang: Bookmarks file grows huge, is truncated
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
URL: http://mozilla.org/quality/browser/fr...
Thanks Claudius, nsbranch+
Keywords: nsbranch → nsbranch+
investigating today.
Status: NEW → ASSIGNED
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
Whiteboard: PDT
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
Attachment #50448 - Flags: review+
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
Attachment #50448 - Flags: superreview+
what are the chances of getting this in for 0.9.4?
Whiteboard: PDT → [PDT]
check it in - PDT+
Whiteboard: [PDT] → [PDT+]
0.9.5 is out the door. bumping TM up by one.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Fixes checked in to trunk and .9.4 branch.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [PDT+] → [PDT+] {willconflict:wgate094+}
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.
Status: REOPENED → ASSIGNED
Removing the + for the time being, given that there might be another fix that
better addresses this problem, which has not been addressed.
Whiteboard: [PDT+] {willconflict:wgate094+} → [PDT] [willconflict:wgate094+] [Fix checked into 094 branch 10.15]
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.
Whiteboard: [PDT] [willconflict:wgate094+] [Fix checked into 094 branch 10.15] → [PDT-] [willconflict:wgate094+] [Fix checked into 094 branch 10.15]
So can I assume sr=waterson for this last patch?
Whiteboard: [PDT-] [willconflict:wgate094+] [Fix checked into 094 branch 10.15] → [PDT] [willconflict:wgate094+] [Fix checked into 094 branch 10.15]
Whiteboard: [PDT] [willconflict:wgate094+] [Fix checked into 094 branch 10.15] → [PDT-] [willconflict:wgate094+] [Fix checked into 094 branch 10.15]
Comment on attachment 53644 [details] [diff] [review]
patch using IndexOf

sr=waterson
Attachment #53644 - Flags: superreview+
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.
Depends on: 104328
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.
No longer depends on: 104328
Depends on: 104328
Comment on attachment 53644 [details] [diff] [review]
patch using IndexOf

r=pchen
Attachment #53644 - Flags: review+
Paul Chen is now taking Bookmarks bugs. For your convenience, you can filter 
email notifications caused by this by searching for 'ilikegoats'.
Assignee: ben → pchen
Status: ASSIGNED → NEW
Fix checked in.
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
*** Bug 105810 has been marked as a duplicate of this bug. ***
marking VERIFIED
Status: RESOLVED → VERIFIED
*** Bug 101307 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: