165268 - Cookies collected during a session lost on crash or abnormal shutdown

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

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

Build-id: 2002082808

When mozilla crashes, new cookies are lost. I know this is by design (bug
158216, comment 2) but I think that it is not good.

I suggest to use the same thing as what is done with the browsing-history. Only
write after a couple of seconds with no additions to the history.

So every cookie that is set should (re)set a timer. When the timer is fired, the
cookies are written to disk. When loading one page, the cookies are written only
once.

Comment 1

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

bug 158216 has fixed this. closing as worksforme.

Comment 2

[Asa Dotzler [:asa]]

No, bug 158216 didn't fix this. If anything, it broke this. Steve said in that
bug "If anyone feels now that cookies are not being written out often enough
(after each page loads), that should be opened in a separate bug report.  That
separate report can deal with the two approaches mentioned in this report".  
I'm designating this bug as that. 

Comment 3

[Asa Dotzler [:asa]]

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

Comment 4

[Darin Fisher]

-> danm :-)

Comment 5

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 6

[Brant Gurganus]

From Peter Beckman:
This happens on the PC Windows side (Win 2000), although it may have
already been resolved.  I noticed that the ticket is for Linux only.

Comment 7

[Dan M]

Attachment: write cookies file after setting a cookie

Like the main said, save the cookies on a timer to prevent their being lost
should the app unexpectedly quit.

This will of course increase page load times! But probably not so one would
notice. I got 2 msec slower (from 512) on the pageloader test with this patch
but I think it was an anomaly; there were no new cookies at test's end. This
patch should cause a single disk access soon after loading a site that sets
cookies. I expect a user wouldn't notice it.

Comment 8

[Conrad Carlen (not reading bugmail)]

What if we got many closely spaced requests to write over a long period of time?
They'd keep being postponed by kLazyWriteTimeout and nothing would be written
for a while. Maybe this doesn't happen in the real world? But, it would be
pretty easy to check if a task was pending and, if so, not reset the timer for
the entire interval.

Comment 9

[Dan M]

Conrad: What kind of task are you thinking of? Docshell still loading? Scripts
still executing? I think neither would be easy to get at from way down in the
cookie code. And actually I think the cookie code shouldn't know about things
like that at all. But if you have something easy in mind, I'm game :)

I'll be able to post something tomorrow about the real world effect of this
patch. Assuming everyone goes to the same sites I do.

Comment 10

[Darin Fisher]

ccarlen: so for example, while running through i-bench, we'd delay writing the
cookies file until the end of the test.  that can only be a good thing ;-)
of course, we must be sure to flush the cookies on shutdown and profile change,
even if the timer has yet to expire.

Comment 11

[Darin Fisher]

Comment on attachment 114385 [details] [diff] [review]
write cookies file after setting a cookie

>+  nsCOMPtr<nsITimer> mWriteTimer;
>+  
> };

nit: bag the extra newline


>+const PRUint32 kLazyWriteTimeout = 1000; //msec

static?  (to avoid name collisions w/ other libraries, right?)


>   if (!nsCRT::strcmp(aTopic, "profile-before-change")) {

i can never remember, do we always get this notification at shutdown?  or only
when actually changing profiles?  what if there is only one profile?  should we
be listening for xpcom-shutdown as well?  should we add an assertion to
~nsCookieService to check that cookies have been flushed?

otherwise, sr=darin

Comment 12

[Dan M]

Attachment: write cookies file after setting a cookie

Darin: extra newline bagged and const staticked. Check. The Cookie Service did
once listen to xpcom-shutdown but Conrad recently removed it or bug 193078.
However profile-before-change does fire at app shutdown; this is currently the
only place cookies are saved.

Conrad: right you are. A little experimentation showed that cookies commonly
come streaming in very slowly, and my 1 second delay was causing cookies to be
written several times while loading a single page. Here's my second go at it: a
10 second delay that shortens itself to 1 second when the network stop event
comes by. It's behaving well for me.

Comment 13

[Darin Fisher]

Comment on attachment 114465 [details] [diff] [review]
write cookies file after setting a cookie

>+      if (mLoadCount > 0) // pointless paranoia?
>+        --mLoadCount;
>+      if (mLoadCount == 0)
>+        LazyWrite(PR_FALSE);

no, not at all pointless.  cookie's webprogresslistener is hooked up lazily...
in fact, you will probably miss the first START event, which means that your
mLoadCount could end up being negative very easily.  you really do need this
check.	i have similar code inside nsPrefetchService.

patch looks fine to me otherwise.  sr=darin

Comment 14

[Conrad Carlen (not reading bugmail)]

Comment on attachment 114465 [details] [diff] [review]
write cookies file after setting a cookie

r=ccarlen

Comment 15

[Dan M]

Patch is in 1.4a. Tinderbox shows no performance hit.