236641 - remove all with large permissions list hangs Mozilla

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Tom Hosiawa]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040224
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040224

I used the feature to always ask me before allowing a cookie for a long time.
Thus a lot of sites ended up in my permission list for cookies. I'm not exactly
sure how many sites were their, but I do recal the text file "cookies.txt" 
under .mozilla to be 45KB.

Recently I changed my mind and tried to remove all the cookie permission
settings by clicking "Remove all sites" under the "Cookie Sites" tab in the
Cookie Manager.

That caused mozilla to instanstly hang. Had to manually kill it. I manually
removed "coookies.txt" under .mozilla to remove the permissions.


Reproducible: Always
Steps to Reproduce:
1. Set cookie permissions for a lot of sites
2. Click "Remove all sites" under the "Cookie sites" tab in the cookie manager

Actual Results:  
Mozilla hanged. Had to manually kill application.

Expected Results:  
Removed all the sites from "Cookie sites" tab in the cookie manager

Comment 1

[Mike Connor [:mconnor]]

if it was a 45 KB text file it might not have hung, it might have just been
taking a long long time to process.

I don't suppose you have a backup of said file to attach for testing?  I know
I've seen instances where importing large bookmarks files takes a long time
(like 10 minutes) and the app appears to hang, but that's the whole "JS runs in
the UI thread" issue at work.

Comment 2

[Tom Hosiawa]

Unfortunetly I only thought of saving the file after deleting it. I'll check my
backups and windows installations if they have the same problem.

I don't think it was processing it. I tried to wait, maybe 15 secs, but I just
gave up as I tried it several times. I could move the Child Manager window, but
the background mozilla window wasn't redrawing itself.

Now that I think of it I should have check top activity. I'll get back to you
when I find the backup copy

Tom

Comment 3

[[e-mail]]

site permissions for cookies are stored under cookperm.txt (in 1.6), not
cookies.txt, so deleting the latter should've had no effect.

having said that, large numbers of permissions are well known to hang
cookiemanager, so i'd say this bug is a dupe...

Comment 4

[[e-mail]]

(by "hang", i mean it's just quite slow... on the order of many minutes).

Comment 5

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

(In reply to comment #2) 
> I don't think it was processing it. I tried to wait, maybe 15 secs

that action once took like 15 minutes for me...

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

bug 226511 should have improved this. But it seems it wasn't enough. (However,
if it used to take 15 minutes, now only one, and you waited only 15 secs, you
don't see the difference)

But how about if we just make the "remove all" button use the nice "remove all"
function in the api, instead of removing item-by-item? That would fix this case
(but not when someone manually selects all, and try to remove that)

Comment 7

[Tom Hosiawa]

Your right, it was cookperm.txt. I was mistaken.

Anyways, I got a backup copy of the file that is a little older (35K), I can
tell you its 1627 lines long. I tried it again and it was taking over 20 minutes
before I fell asleep. It's now complete after waking up 6 hours later and
mozilla didn't hang. 

I can't really give you the exact time it took, but it took a while. If you want
I can run it again, and get a more accurate time. Otherwise, I guess this bug
should be closed.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

To be sure, can you try again with a recent nightly? I'm somewhat confused on
whether the patch in bug 226511 made it to 1.6 (think not) and you version (it
says rv:1.6, but has a newer build id)

Comment 9

[Mike Connor [:mconnor]]

-> me (yes, YACMB :)

if someone manually selects all, then we're screwed, but since there's no Select
All keystroke in the manager, using the function mvl described is a much better
solution for this situation.  It can't fix everything, but if it improves this
situation then we're eliminating an easy to hit limitation.

Tom, I don't suppose you would mind attaching the older version so I can use it
in perf testing the alternate solution?

Comment 10

[Tom Hosiawa]

Attachment: cookie permission file with many entries

Here's my cookperms.txt file that takes a long time for the function "Remove
all sites" in the Cookie Manager to complete

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

the RemoveAll function can be fast, because it just throws away the list of
stored sites. But that is for all types. We don't want to call that from the
cookie manager, for example.
It could be extended to take an extra param, but that would mean it has to walk
over the list. I'm not sure we want that.

The real problem seem to be that the entire list is written out on every
deletion. No good. Slow.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/cookie/nsPermissionManager.cpp#337

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Use a timer to write perms

This patch uses a timer to write out the permissions file only after 2 seconds
of no changes to the list. As a result, the list is only written out once when
you do remove all, instead of once for each item there was.
The code is shamelessly copied from nsCookieService.

Comment 13

[[e-mail]]

Comment on attachment 143129 [details] [diff] [review]
Use a timer to write perms

perfect. r=dwitte

Comment 14

[Darin Fisher]

Comment on attachment 143129 [details] [diff] [review]
Use a timer to write perms

>Index: extensions/cookie/nsPermissionManager.cpp

> nsPermissionManager::~nsPermissionManager()
> {
>+  if (mWriteTimer)
>+    mWriteTimer->Cancel();

how will your destructor ever run if the timer is active?

Comment 15

[[e-mail]]

I don't think it's a problem here, no? We're initing the timer via
InitWithFuncCallback, which unlike InitWithCallback, doesn't addref.

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 143129 [details] [diff] [review]
Use a timer to write perms

(In reply to comment #14)
> how will your destructor ever run if the timer is active?
> 
Like dwitte said, there is no addref. If there was one, there would have been a
bug in cookies for a while now, because i just copied that code.
To be sure, i tested, and the dtor was called when there was a timer active.
(and normally, there isn't one, because in Observe() it is already removed when
the app shuts down. The call in the dtor is just to be sure.)

Comment 17

[Darin Fisher]

Comment on attachment 143129 [details] [diff] [review]
Use a timer to write perms

oh, crap... i read that too fast!  sorry about that.


>Index: extensions/cookie/nsPermissionManager.cpp

>+    if (!nsCRT::strcmp(someData, NS_LITERAL_STRING("shutdown-cleanse").get())) {
>+      RemoveTypeStrings();
>+      RemoveAllFromMemory();
>       if (mPermissionsFile) {
>         mPermissionsFile->Remove(PR_FALSE);
>       }
>+    } else {
>+      Write();
>+      RemoveTypeStrings();
>+      RemoveAllFromMemory();
>+    }

nit: why not move these calls to RemoveTypeStrings and RemoveAllFromMemory
outside the condition?	that way you aren't duplicating code.


sr=darin

Comment 18

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 143129 [details] [diff] [review]
Use a timer to write perms

Requesting approval for 1.7b. This is a low risk patch, because it is just code
copied from cookies. The result is a much faster 'delete all sites' in cookie
and image managers.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 143129 [details] [diff] [review]
Use a timer to write perms

a=dveditz for 1.7b

Comment 20

[Michiel van Leeuwen (email: mvl+moz@)]

Checked in.

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

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

Comment 22

[benc]

->allplats. Got this on mac, I'll verify soon.

Comment 23

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old