132013 - Remove from list makes program unresponsive for too long.

Status: VERIFIED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Hixie (not reading bugmail)]

STEPS TO REPRODUCE
   1. Select seven downloads in the Download Manager window.
   2. Click "Remove From List".

ACTUAL RESULTS
   Takes several seconds.

EXPECTED RESULTS
   Takes no noticable time.

Comment 1

[sairuh (rarely reading bugmail)]

hixie, how many items did you have listed in the download manager? fwiw, when i
had 3 items it removed 'em quickly... let me try more...

Comment 2

[sairuh (rarely reading bugmail)]

okay, now i see this when i have 10 items in the dl mgr --it takes about 4-5sec
to remove them all. tested using 2002.03.20 comm bits on linux rh7.2, win2k and
mac 10.1.3.

Comment 3

[Peter Trudelle]

I've had this take minutes, especially if the list had to scroll.  The first
time it happened, I thought the program had gone nuts, and I killed it.  The
next time I went to lunch and all was well when I got back.

Comment 4

[Peter Trudelle]

Yup, CPU pegging makes this indistinguishable from hang.  nsbeta1+ ->1.0

Comment 5

[Blake Ross]

outliner should help this, and I have a fix to batch it also.

Comment 6

[Peter Trudelle]

ADT2

Comment 7

[Vadim Berezniker]

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

Comment 8

[sairuh (rarely reading bugmail)]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Blake Ross]

Attachment: patch

this will help

Comment 12

[Blake Ross]

Comment on attachment 82431 [details] [diff] [review]
patch

indicating gathered reviews (ben, hewitt on aim/irc)

Comment 13

[Blake Ross]

There are other speed improvements to be made, but this bug was about batching,
and I've checked it in on the trunk. marking fixed.

Comment 14

[Aleksey Nogin]

This commit added a warning on brad TBox:

+   8. xpfe/components/download-manager/src/nsDownloadManager.cpp:89 (See build
log excerpt)
+      Will be re-ordered to match declaration order
+87 nsDownloadManager::nsDownloadManager() : mCurrDownloads(nsnull),
+88                                          mBatches(0)
+89 {
+90   NS_INIT_ISUPPORTS();
+91 }
+

Comment 15

[scottputterman]

changing to adt1.0.0- for beta and adding [adt2 rtm]

Comment 16

[sairuh (rarely reading bugmail)]

vrfy'd fixed on the trunk. tested removing 10-15 entries in the dl mgr --took
less than a second.

linux rh7.2, 2002.05.07.10-trunk comm
mac 10.1.4, 2002.05.07.03-trunk comm
win2k, 2002.05.07.08-trunk comm

Comment 17

[R.K.Aa.]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Rob Stow]

Why has this bug been marked as resolved/fixed ?   It is very clearly still
there in Release Candidate 2 for Windows NT.   I currently have about 200 items
completed downloads listed in Download Manager, and if I select all of them and
click "Remove From List" Mozilla hangs completely.  I can leave Mozilla sitting
there for an hour and come back to find it still sitting there unchanged and
completely unresponsive.  When I use NT's "Task Manager" to kill Mozilla and
then restart Mozilla, all of the items will still be there in Download Manager.
  This is reproducible also in W2K.

Comment 20

[Vadim Berezniker]

Sigh. This was checked into the trunk and was not checked into the branch thus
it's not in RC2. 
If you want this, download a trunk build or just wait...

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Seeing that this is has adt1.0.0-, I don't think that it will ever be on the
branch...

Comment 22

[Jure Repinc (JLP)]

Is this really in the trunk? I have build 2002051408 and when I select 5 items
to be removed the removal takes a very long time (about 20s) and I have Athlon
1200MHz.

Comment 23

[Rob Stow]

RE Comment #20

"Sigh. This was checked into the trunk and was not checked into the branch thus
it's not in RC2. If you want this, download a trunk build or just wait..."

This bug was not fixed in the trunk build I downloaded the day before RC2
was announced - and that trunk build was about 3 days AFTER this bug was 
marked as fixed.

This bug is also not fixed in build 2002051508.

Comment 24

[Rob Stow]

(Using build 2002051508)
1.)  Got tired of waiting for a way to clean up download manager entries 
using Download Manager itself, so I simply tried deleting the file
downloads.rdf from my profile.   Works great - takes 2 seconds compared
to hanging an Athlon 1800+ trying to do it with Download Manager.
2.)  Why isn't Download Manager simply deleting this file when the user 
selects all entries and clicks "Remove From List"  ???

Comment 25

[Peter Trudelle]

Sairuh, I believe the ADT is expecting bugs that have been verified on the trunk
to  be put into the 'verified' state.  This should get the bug onto queries they
use for taking good fixes onto the branch.

Comment 26

[[e-mail]]

Test using build 2002051521/linux: on my PIII500 the removal of 1845
entries in the download manager took 6 minutes and 35 seconds, during which
all of mozilla was unresponsive and taking 100% CPU.

Comment 27

[Hixie (not reading bugmail)]

Reopening. That's not acceptable.

Comment 28

[Peter Trudelle]

The real defect here is the speed of the operation, not how it is accomplished.
 updating summary.  This operation should ideally complete in one-tenth of a
second or less, and if it takes more than one second it should display some type
of indication that a long operation is in progress, perhaps a progress indicator
or at least a busy cursor.  

Comment 29

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: proposed patch

I'm proposing this as an additional measure to speed this up. Note that I've
been unable to test this as someone's sloppy error checking *cough* means that
every time I delete, I'm deluged with assertions about row indices out of
bounds :-P

I'd appreciate someone testing this and letting me know if it fixes the problem
- basically we notify the template builder that a batch operation is about to
begin so it doesn't respond until after the operation is complete.

Comment 30

[Ben Goodger (use ben at mozilla dot org for email)]

er, the previous patch should be ammended to say 'notify the template builder'
and not 'notify the datasource'

Comment 31

[Blake Ross]

Comment on attachment 84414 [details] [diff] [review]
proposed patch

Good stuff. sr=blake

Comment 32

[Hixie (not reading bugmail)]

It's a bit ironic that the test for the optimisation has to be done once for
every item in the list... wouldn't something like:

    for (i = 0; i < selectedItems.length-1; i++) {
      gDownloadManager.removeDownload(selectedItems[i].id);
    }
    gDownloadManager.endBatchUpdate();
    observer.endUpdateBatch(ds);
    gDownloadManager.removeDownload(selectedItems[selectedItems.length-1].id);

...be better? Do we even have to have the last one done after the batching?
Doesn't ending the batching trigger all the changes?

Comment 33

[Blake Ross]

Comment on attachment 84414 [details] [diff] [review]
proposed patch

noting r=hewitt with hixie's proposed change, not that it has any noticeable
improvement.

Comment 34

[Pierre Chanial]

Some comments:
- has anybody tested this patch?
- as ben reported, I am seeing tons of assertions and the current behavior is
horked: when removing several dls, one is missed. After that, removing dls does
not work at all. I wonder if this patch is worth be checked in before that the
other pbs are fixed. That's why I could not test is neither.
- besides, there should not be methods called startUpdateBatch and
startBatchUpdate. That's confusing.
- in addition, the question raised by Ian is still unanswered: 
"Do we even have to have the last one done after the batching?
Doesn't ending the batching trigger all the changes?" See above for reference.

Comment 35

[Blake Ross]

- removing several downloads works flawlessly for me.
- they're on two different interfaces, it's not *that* confusing.  Anyway, I
maintain that startUpdateBatch is wrong, which is why I named mine
startBatchUpdate (anyway, this patch does not add or change any method names).
- yes, we have to do the last one. no, ending the batch doesn't "trigger all the
changes," I don't know what that means.

Comment 36

[Blake Ross]

Checked in on trunk.

Comment 37

[Hixie (not reading bugmail)]

VERIFIED FIXED. Much faster now.

Comment 38

[Jaime Rodriguez, Jr.]

Mass removing adt1.0.0, and adding adt1.0.1 because, we are now on 1.0.1.

Comment 39

[Peter Trudelle]

ADT1.0.1+, okay to checkin on branch once you have Driver's approval.

Comment 40

[chris hofmann]

Comment on attachment 84414 [details] [diff] [review]
proposed patch

a=chofmann for 1.0.1.
add the fixed1.0.1 keyword after checking in on the branch

Comment 41

[Blake Ross]

fixed on branch.

Comment 42

[Rob Stow]

This is NOT fixed in build 2002061707 on WinNT4 or W2K.

I had a 136 KB downloads.rdf and made the mistake of assuming
that a fix for this bug would have been implemented in the
trunk builds by now.  I selected all items and clicked on
"remove .." and five minutes later Mozilla was still totally
unresponsive.  CPU usage went up to 94% and stayed there.
Eventually I gave up and used task manager to terminate Mozilla.

That was on an Athlon 600 system with 256 MB.  I next copied
the same downloads.rdf file to an Athlon XP 1800+ system running
W2K and the same version of Mozilla and tested it there -
same thing:  I gave up after 5 minutes.

I am reasonably sure that the downloads.rdf file was not
the problem:  selecting and deleting just one item at a
time worked - albeit still quite slowly:  about 8 seconds
on the Athlon 600 and about 5 seconds on the Athlon XP 
1800+.


In the future I will try to remember to simply use Explorer
to delete the downloads.rdf file.   What I can't understand
is why Mozilla does not simply delete that file when the
user is attempting to completely clean out his download 
history ?  IE., deleting ALL items from download manager
should be a two second operation even on Pentium 133.

Much of this could be avoided if Mozilla would default to 
automatically removing finished downloads from the list.
At the very least, put at option for this in the
Preferences.

Comment 43

[Alfonso Martinez]

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

Comment 44

[sairuh (rarely reading bugmail)]

i'll wait till bug 153480 is fixed on the branch before verifying this one
--since i ran into that crasher just now with today's linux branch bits.

Comment 45

[sairuh (rarely reading bugmail)]

vrfy'd on the BRANCH using 2002.07.22 commercial branch builds on linux rh7.2,
win2k and mac 10.1.5.

Comment 46

[Michael Gabriel]

i just dare instaleld a trunk build after a couple of weeks ... since i wasnt
using download manager i dont know when this happened
but now that it was back on by default, i had a list of about 200 entries in it
and it took 4 minutes to delete it ... so to me this problem is back again !

Comment 47

[Jure Repinc (JLP)]

I also did some cleaning a few days ago and I had about 50 - 100 items on list.
And like it was said it was slow as hell and I was unable to use any part of
Mozilla during the proccess. Currently I use Mozilla 2003011508 on Windows XP.

Comment 48

[Michael Gabriel]

reopening as per comment and own experience

Comment 49

[[e-mail]]

The question is how to deal with this. As far as I can see, there are several ways:
- make the process of removing an entry faster
- avoid non-responsiveness while it takes as long as it takes
- avoid the problem by implementing an automatic delete feature, e.g. only keep
the latest N completed downloads in the list (where N around 10)
- add a feature for deleting the whole list by quickly zapping the RDF file
instead of removing each entry individually.

Comment 50

[Michael Gabriel]

well, this worked before ... when was did it change back tho ?

Comment 51

[John Morrison]

This bug was about batching multiple deletes. It was fixed.

The issue that the most recent comments discuss is bug 161783

Comment 52

[John Morrison]

putting back verify. If someone can demonstrate that batching does not work,
_then_ reopen this bug.

Comment 53

[Webmaster33]

>A similiar problem was fixed a long time ago, see Bug 132013. For me it
>works with all 1.7x and 1.8x on WinNT4. How large is your downloads.rdf file?
>Does it work with Mozilla 1.7.x or 1.8a2?

This was NOT fixed!
In v1.8a6 the problem still appears! 
Selecting all items and Remove hangs Mozilla!
My downloads.rdf is 4-5 Mb!

Please REOPEN this bug!

Comment 54

[Boris Zbarsky [:bzbarsky]]

Please see comment 51 and comment 52...