378594 - Closing takes very long after deleting a livemark (places)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ria Klaassen (not reading all bugmail)]

Steps to reproduce:

- Open Firefox with enabled places with a new profile
- Wait for about 20 seconds while livemarks are loading
- Click Bookmarks menu and hover over Bookmarks Toolbar Folder.
- In Bookmarks Toolbar Folder delete the "Latest Headlines" livemark
- Close Firefox

Result: after that you'll experience a long closing procedure (for about 40 seconds here) while you'll see a parent.lock file in the profile.
The hang does not occur if you wait 12 seconds before closing Firefox.

In bug 338884 comment 2 I mentioned something similar but not quite the same for this bug is not about loading livemarks but about deleting livemarks just before closing.

Comment 1

[(not reading, please use [e-mail] instead)]

dietrich, given how livemarks uses the annotation service, perhaps this perforamce bug is improved by your fix for bug #381378?

Comment 2

[Ria Klaassen (not reading all bugmail)]

I tested a recent build and a 20 May build and both take about 20 seconds to close. On a 23 April places build it still takes 40 seconds.
So it has improved, but not by bug 381378. 

Comment 3

[Dietrich Ayala (:dietrich)]

finding owners for all the A6 blockers. can you please put a swag in the whiteboard? then we can review and load-balance at the next places meeting.

Comment 4

[Radek 'sysKin' Czyz]

The same applies if you delete other big folders. I believe the reason is identical to the reason why *reloading* live bookmarks took long (bug 379729) and this was fixed by attachment 266122 [details] [diff] [review].

Wherever delete loop is, it needs a begin transaction at the beginning and end transaction at end/throw.

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)
> The same applies if you delete other big folders. I believe the reason is
> identical to the reason why *reloading* live bookmarks took long (bug 379729)
> and this was fixed by attachment 266122 [details] [diff] [review].
> 
> Wherever delete loop is, it needs a begin transaction at the beginning and end
> transaction at end/throw.
> 

hm, bookmark folder removal is wrapped in a transaction. although it could be optimized a bit, possibly starting the transaction before removing the annotations:

http://lxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavBookmarks.cpp#1250

Comment 6

[Dietrich Ayala (:dietrich)]

this is not causing hangs, freezes or extreme-brokeness of critical features, so pushing to B1.

Comment 7

[Dan Mills [:thunder]]

I'm not likely to be able to work on these bugs anytime soon.

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: v1

wrap all the db activity in RemoveFolder in a single transaction.

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 282764 [details] [diff] [review]
v1

r=sspitzer, thanks dietrich

Comment 10

[Dietrich Ayala (:dietrich)]

Checked in. Ria, can you please see if the bug is still reproduce-able?

Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.121; previous revision: 1.120
done

Comment 11

[Hasham]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

Closes properly. Marking as verified.

Comment 12

[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