Closed Bug 236641 Opened 20 years ago Closed 20 years ago

remove all with large permissions list hangs Mozilla


(Core :: Networking: Cookies, defect)

Not set





(Reporter: tomek32, Assigned: mvl)




(2 files)

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
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.
Summary: cookie manager hangs mozilla → remove all with large permissions list hangs Mozilla
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

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...
Whiteboard: DUPEME
(by "hang", i mean it's just quite slow... on the order of many minutes).
(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...
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)
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.
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)
-> 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?
Assignee: darin → mconnor
Here's my cookperms.txt file that takes a long time for the function "Remove
all sites" in the Cookie Manager to complete
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.
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.
Attachment #143129 - Flags: review?(dwitte)
Comment on attachment 143129 [details] [diff] [review]
Use a timer to write perms

perfect. r=dwitte
Attachment #143129 - Flags: review?(dwitte) → review+
Attachment #143129 - Flags: superreview?(darin)
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?
Attachment #143129 - Flags: superreview?(darin) → superreview-
I don't think it's a problem here, no? We're initing the timer via
InitWithFuncCallback, which unlike InitWithCallback, doesn't addref.
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.)
Attachment #143129 - Flags: superreview- → superreview?(darin)
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.

Attachment #143129 - Flags: superreview?(darin) → superreview+
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.
Attachment #143129 - Flags: approval1.7b?
Comment on attachment 143129 [details] [diff] [review]
Use a timer to write perms

a=dveditz for 1.7b
Attachment #143129 - Flags: approval1.7b? → approval1.7b+
Checked in.
Closed: 20 years ago
Resolution: --- → FIXED
Resolution: FIXED → ---
Assignee: mconnor → mvl
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Whiteboard: DUPEME
*** Bug 246242 has been marked as a duplicate of this bug. ***
->allplats. Got this on mac, I'll verify soon.
Keywords: verifyme
OS: Linux → All
Hardware: PC → All
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.