429379 - Deleting large live bookmark makes Firefox unresponsive for several seconds.

Status: VERIFIED FIXED

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

Description

[eleven81]

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

Deleting large live bookmark makes Firefox unresponsive for several seconds.

Reproducible: Always

Steps to Reproduce:
1. Navigate to http://www.thejournal.com/the/rss/
2. Click the button to subscribe to this feed using Live Bookmarks to the Bookmarks Toolbar.
3. Allow the feed to populate.
4. Right click the Live Bookmark, select Delete.
Actual Results:  
The browser hangs for 45 seconds at least, accepting no input.  Once the delete operation completes, the browser resumes normal functionality.

Expected Results:  
The Live Bookmark should disappear from the bookmark toolbar quickly, and the user interface should not hang.

Deleting the Live Bookmark should not take 45 seconds or longer.  The user interface should not hang.  If there is really some operation that needs so long to complete, it should be started and run in the background, not locking up the UI.

Comment 1

[Simon Bünzli]

That looks like a _very_ inefficient SQL query. On my system it took a full 2 minutes to delete what was added in a matter of seconds.

Comment 2

[eleven81]

I revisited this today with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050706 Minefield/3.0pre and noticed that there is a very large amount of disk i/o that occurs during the time Firefox is hanging, waiting for the live bookmark to delete.

I am not sure exactly what is being accessed or deleted.  It seems like Firefox is looking at the Live Bookmark like a directory full of files saved on the hard drive, instead of as a list of links cached someplace in memory for the current session.

Comment 3

[eleven81]

I revisited this again today, this time with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081117 Minefield/3.1b2pre and noticed a new dialog that I had not before seen.  The dialog appeared a total of seven times for me, each time asking whether or not to stop a script.  Each time I clicked continue until the live bookmark had been deleted.  The text of that dialog follows:

A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.

Script: chrome://browser/content/places/toolbar.xml:553

Comment 4

[Marco Bonardo [:mak]]

easily reproducible on a clean profile, we should take a look if we can gain something on this test case for 3.1 since it could help also other delete cases.

Comment 5

[Marco Bonardo [:mak]]

Attachment: wip 0.1

there are 2 bottlenecks here, the former is due to removeFolder, that has to remove all childrens (and luckily this is a bit faster now, still not perfect though, but we can track this later in bug 428459).

The latter and much bigger bottleneck is due to the transactionService, even if this is a livemark we save all of its children. Well i think we should not, children of a livemark are known to change, so there's no point in saving something we can reload directly from the feed.

This patch adds a removeLivemarkTransaction, and with this the big livemark removal takes 1-2 seconds. On undo we recreate the livemark and reload its contents from the feed, re-registering it with livemark service (something we were not doing before, so that's another bug.)

asking a first-pass review, i have to write a test for this before going on.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 349564 [details] [diff] [review]
wip 0.1


>+function placesRemoveLivemarkTransaction(aFolderId) {
>+  this.redoTransaction = this.doTransaction;
>+  this._id = aFolderId;
>+  this._title = PlacesUtils.bookmarks.getItemTitle(this._id);
>+  this._container = PlacesUtils.bookmarks.getFolderIdForItem(this._id);
>+  var annos = PlacesUtils.getAnnotationsForItem(this._id);
>+  // Exclude livemark related annotations, those will be recreated automatically
>+  this._annotations = annos.filter(function(aValue, aIndex, aArray) {
>+      return !aValue.name.match(/^livemark/);
>+    });

please match exactly on the annotations you intend to exclude, as you might match annotations from extensions otherwise.

r=me for the approach.

Comment 7

[Marco Bonardo [:mak]]

mh, i did that because we have a lot of different annotations there, and if we add more annotations in the service we will also have to update the transaction manager. Hwv i don't see other solutions if we don't want to force extension developers to use a different annotation root.

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.1

addes a test for this and cleaned up a bit transaction manager livemarks tests, plus addressed previous comment.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 352098 [details] [diff] [review]
patch v1.1

oops wrong patch

Comment 10

[Marco Bonardo [:mak]]

Attachment: correct patch v1.0

Sorry this is the correct one

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 352099 [details] [diff] [review]
correct patch v1.0

>+  // Testing remove livemark
>+  // Set an annotation an check that we don't lose it on undo

nit: s/an/and/

r=me

Comment 12

[Marco Bonardo [:mak]]

Attachment: correct patch v1.1

addressed nit

Comment 13

[Marco Bonardo [:mak]]

pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/a8b303937581

Comment 14

[Marco Bonardo [:mak]]

askin in litmus because maybe the big livemark posted here could be useful for manual testing

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 352171 [details] [diff] [review]
correct patch v1.1

asking approval 1.9.1. this is making livemarks delete really faster (and general all folder deletes that contain livemarks). Low risk since come with a complete unit test.

Comment 16

[Marco Bonardo [:mak]]

Attachment: followup

if we try to load a not existant livemark uri we leak loadgroup and other things

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 352231 [details] [diff] [review]
followup

i'll move the issue to a new bug since i think it's a livemark service bug.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 352171 [details] [diff] [review]
correct patch v1.1

a191=beltzner

Comment 19

[Marco Bonardo [:mak]]

pushed to 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/58e38927aaca

Comment 20

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.1b3pre) Gecko/20090123 Shiretoko/3.1b3pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090123 Minefield/3.2a1pre

deleting takes 2-4 seconds now.

Comment 22

[Tracy Walker [:tracy]]

added https://litmus.mozilla.org/show_test.cgi?id=7540 to litmus

Comment 23

[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