148213 - Add COOKIE_Write() to nsCookieManager::RemoveAll()

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Mike Pinkerton (not reading bugmail)]

As steve and I discussed in email, nsCookieManager::RemoveAll() needs to call
COOKIE_Write in order to flush the empty cookie list to disk.

We need this for the "clear all cookies" button in chimera. Sure, we could do
what seamonkey does and enumerate all the cookies and delete them one by one,
but why not use code that's already there?

Steve: I can take this bug and get it on the trunk and on the branch if you
don't have time.

Comment 1

[Stephen P. Morse]

Attachment: Flushing to disk after removing all cookies

Comment 2

[Mike Pinkerton (not reading bugmail)]

we need this for chimera, it is topembed. cc'ing beard for an SR

Comment 3

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 85711 [details] [diff] [review]
Flushing to disk after removing all cookies

tested, this works. thanks steve! beard, can you sr?

r=pink

Comment 4

[Stephen P. Morse]

nominating nsbeta1 per comment 2.

Comment 5

[Patrick C. Beard]

Comment on attachment 85711 [details] [diff] [review]
Flushing to disk after removing all cookies

sr=beard, this will also have to be checked into CHIMERA_M1_0_BRANCH.

Comment 6

[Stephen P. Morse]

Checked in on trunk.

Comment 7

[Mike Pinkerton (not reading bugmail)]

requesting branch permission

Comment 8

[chris hofmann]

Comment on attachment 85711 [details] [diff] [review]
Flushing to disk after removing all cookies

a=chofmann for 1.0.1

Comment 9

[Judson Valeski]

this patch is doing more than writing to disk. what's the other code supposed to
be doing?

Comment 10

[Stephen P. Morse]

The other code makes sure we write to disk even if the cookie list is empty.  As 
currently coded, there is a null check and no write occurs.  Problem is that if 
you delete all cookies you would wind up with an empty list and nothing gets 
written, leaving the old (non-empty) cookie file on disk.

Comment 11

[scottputterman]

this needs the browser triage teams approval before being approved by the adt.

Comment 12

[Jaime Rodriguez, Jr.]

tever - pls verify the fix on the trunk, and check around for possible
regressions. thanks!

Comment 13

[Tom Everingham]

According to Steve, this can't really be checked since it is not used in the
product.  Or at least without adding additional test code.  I did check with LXR
that the fix is checked in and based on comment #3 I think this can be marked
verified.

Comment 14

[scottputterman]

adding adt1.0.1+. 

Comment 15

[Samir Gehani]

Not needed for Mach V, but needed for ProjectX which is using the
MOZILLA_1_0_BRANCH per saari.

Comment 16

[scottputterman]

Please check this in asap and change the mozilla1.0.1+ keyword to fixed1.0.1

Comment 17

[Stephen P. Morse]

Will check in as soon as drivers gives their reapproval (previous approval has 
expired).

Comment 18

[Mike Pinkerton (not reading bugmail)]

doh, i'm not cc'd so i missed the adt+. i'm gonna land this on the branch today.

Comment 19

[Stephen P. Morse]

But the driver's approval in this expired a while ago.  I re-rerequested 
drivers approval yesterday.

Comment 20

[Mike Pinkerton (not reading bugmail)]

landed on branch

Comment 21

[Tom Everingham]

adding verified1.0.1

Comment 22

[Stephen P. Morse]

pinkerton:

Was the important part of this patch the fact that the cookies were not written 
out immediately upon doing the remove-all, or that they never got written out 
even when the browser was shut down because of the error with not writing if the 
cookie list is empty?  If the latter, then would it be ok to delay all cookie 
writing until the browser closes?  I'm asking because of bug 158216 (needed for 
Chimera) in which I'm considering never writing out the cookie file until the 
browser closes.

Comment 23

[Steve Dagley]

my $.02 - write the file when the user edits the pref and on browser exit.  If
you only write on exit any crash loses the changes made in the edit.

Comment 24

[Mike Pinkerton (not reading bugmail)]

it should clear and write immediately, because then if the browser crashes
before quitting, all your cookies would be back and you'd never know why.

Comment 25

[Stephen P. Morse]

I'm not sure I undertand Dagley's comment:

   > write the file when the user edits the pref

What pref are you talking about?  Are your referring to deletions that the user
specifically makes, such as when using the cookie-manager dialog?  Yes, I could
easily keep the saving in that case but not in the case when new cookies arrive,
as the latter is the cause of the performance hit mentioned in bug 158216.

If your concern is with browser crashes, I thought we didn't crash any more. ;-)

In any case, we don't write out other items (e.g., prefs) whenever they change,
so why do we need to single out cookies?

Comment 26

[Mike Pinkerton (not reading bugmail)]

we do write out prefs to disk when you close the prefs window. i think we're all
in agreement here: writing cookies as they come in while browsing bad, writing
cookies when the user makes explicit changes they expect saved (editing,
deleting) good. ;)