251648 - cookies are not saved

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

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

currnet cvs. seen on linux. not sure if I'Ve seen this on windows, but what I
definitely saw on windows was the warning from saveoutputstream about data loss.

when I log in to bugzilla, close the browser, and start it again, I'm no longer
logged in.

this may have to do with the fact that there's a buffered output stream before
the safefileoutputstream that has unflushed data, not sure...

Comment 1

[[e-mail]]

if this is for real, and it's a problem relating to buffered + safe streams,
then this is really not good. we've shipped alpha 2 with cookies and perms as
consumers. ;)

investigating...

Comment 2

[[e-mail]]

(btw, the NS_WARNING from safestream about data loss is about an individual
Write() call failing - not about the temp file being tossed. so that _should_ be
unrelated to your proposed buffered + safe stream problem.)

Comment 3

[[e-mail]]

confirmed. i'm seeing truncation of the cookie file, i.e. the last <4kb is
missing, due to the underlying nsIOutputStream being closed before the buffered
stream has had a chance to write its last chunk of data. i'm assuming this is
the same problem you're seeing?

so this is a fundamental problem with the way things are set up now. say we
create a safe output stream (NS_NewSafeLocalFileOutputStream()), and then grab a
buffered stream (NS_NewBufferedOutputStream()) on top of that. the buffered
stream does its buffering magic and eventually forwards the Write() etc. calls
on to the underlying safe stream, to which it holds a strong reference.
ordinarily, the underlying stream cannot be destroyed until the buffered stream
does its flushing and closing, and everything's happy. in this case, we do
something which (imo) should be forbidden: when we call Finish(), we go behind
the buffered stream's back, and close the underlying stream. when it tries to
close itself down normally, it can't, and any remaining buffered data is lost.

what we really want to do is have nsISafeFileOutputStream be at the top of the
foodchain: we create our buffered stream first, and then the safe stream can
wrap it. this not only allows data to be flushed properly, but it lets us
properly check the rv's of the buffered stream methods - at the moment we're
checking rv's of the underlying stream's methods, which is not the same thing.
but unfortunately, the safe stream has to be created before anything else,
because the stream points to a tempfile, not the original.

i think we can solve this by adding an |nsIOutputStream stream| attribute to
nsISafeFileOutputStream, and adding an nsIOutputStream *mStream member to
nsSafeFileOutputStream which holds a pointer to the underlying stream (which
we'd eventually forward calls to). then a consumer could do this:

NS_NewSafeLocalFileOutputStream(getter_AddRefs(safeStream), ...);
nsIOutputStream *normalStream = safeStream->GetStream();
NS_NewBufferedOutputStream(getter_AddRefs(bufStream), normalStream, ...);
safeStream->SetStream(bufStream);

this is ugly :/ is there some way we can change the vtable pointers of a class
instance at runtime? so, for instance, we could swap the virtual function ptrs
for the nsFileOutputStream base class (that nsSafeFileOutputStream inherits)
from one stream instance to another? i don't know enough about C++ machinery :/

a hackier solution would just be to Close() the buffered stream explicitly (and
check for success) before Finish()'ing the safe stream. seems fragile to me.

Comment 4

[[e-mail]]

Attachment: quick (temporary) hack

this will get trunk working again, while we figure out a real solution :/

Comment 5

[[e-mail]]

bz proposed something that would work, if the only other "stream layer" we care
about is the buffered stream (and no others exist):

<bz> Or rather, why is the safe output stream not automatically buffered?
<bz> Well, if there is a good reason not to make it buffered, that's fine.  But
it seems to me you'd always want it buffered.
<bz> So it can create its own _internal_ buffered stream
<bz> the way you want it
<bz> so the chain is
<bz> you -> safe stream -> buffered stream -> file

so we could specify a buffer size inparam on the safe stream's Init(), where 0
means unbuffered, -1 means default and anything else is a desired size.

Comment 6

[[e-mail]]

Attachment: hack v2 (checked in)

... and this one works.

Comment 7

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

(In reply to comment #3)
> confirmed. i'm seeing truncation of the cookie file, i.e. the last <4kb is
> missing, due to the underlying nsIOutputStream being closed before the buffered
> stream has had a chance to write its last chunk of data. i'm assuming this is
> the same problem you're seeing?

ah, that could well be. I haven't looked at the size of the file. I only noticed
that my bugzilla login was never remembered. and that after several restarts,
all my cookies were gone.

(In reply to comment #2)
> (btw, the NS_WARNING from safestream about data loss is about an individual
> Write() call failing - not about the temp file being tossed. so that _should_ be
> unrelated to your proposed buffered + safe stream problem.)

Could it be the Write() of the buffered stream after the Finish() call?

Comment 8

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

So... I don't think the patch here is a hack, really. if someone layers random
other output streams on top of the safefileoutputstream, it is the
responsibility of the caller to make sure all the data arrived at the
safeoutputstream by the time Finish() is called.

Comment 9

[Darin Fisher]

Perhaps we should make nsBufferedOutputStream support nsISafeFileOutputStream? 
If nsISafeFileOutputStream is not supported by the inner nsIOutputStream, then
we could just return a failure code from Finish, like NS_ERROR_UNEXPECTED.

bz mentioned some alternatives such as:

1) Make the safe stream always be buffered (or take an arg to init() that says
whether to buffer)

2) Change the finish() to abort() and let finish() happen automatically when the
stream is closed

bz, mvl, and myself discussed this over irc, and it looks like #2 would cause
problems because we'd have to call abort() on every early exit point.  morever,
how would we find out if the save finished successfully?

as for #1, adding an init flag would suggest that we'd also need to support if
on nsFileOutputStream.  that is tricky since it means that we'd want to refactor
a bunch of stuff from nsBufferedStream.  or, we could do that more easily by
adding additional layers.  but what about the other interfaces supported by
nsBufferedOutputStream?  it seems like we'd want to support those too in a
buffered stream mode.

that's why i suggest just having nsBufferedOutputStream implement
nsISafeFileOutputStream.  or, we could just accept this Flush solution.

maybe it would be less ugly having nsBufferedOutputStream implement
nsISafeFileOutputStream if nsISafeFileOutputStream were renamed to something
else, like nsISafeOutputStream!  why does it's name need to convey the word
"file"?  why couldn't you write an output stream that used a memory buffer and
then copied that memory buffer over to the real output stream when Finish is
called?  so, i vote for nsISafeOutputStream.  the ContractID of the current
nsSafeFileOutputStream would stay the same and it would indicate support for a
specific set of interfaces: nsIOutputStream, nsIFileOutputStream, and
nsISafeOutputStream.

sound good?

Comment 10

[Darin Fisher]

I think this should be a trunk blocker.

Comment 11

[Darin Fisher]

Comment on attachment 153390 [details] [diff] [review]
hack v2 (checked in)

r+sr=darin for a quick fix, but i'd really like to see nsISafeOutputStream
implemented.

Comment 12

[[e-mail]]

>Could it be the Write() of the buffered stream after the Finish() call?

yeah, that'll be what it was :)

>So... I don't think the patch here is a hack, really.

yeah, it makes sense from an ownership perspective... but it's a bit fragile for
consumers not to mess up, imo. it'd be much more robust to have something that
"just works".

so i talked with darin on irc, and doing |nsBufferedOutputStream :
nsISafeOutputStream| sounds good to me:

<darin> mvl: make nsBufferedOutputStream implement nsISafeOutputStream too..a nd
make its Finish method call its Flush and then the inner output stream's Finish

i'll hack that up tonight.

Comment 13

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

yeah, I think I like that.

can me make it only qi to nsISafeOutputStream if the inner stream actually
supports it?
(http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsISupportsImpl.h#322)

Comment 14

[[e-mail]]

Attachment: fix v1

thus spoken. (i split out nsISafeOutputStream into its own idl, since i figure
it doesn't belong with the file ones anymore. does that sound okay?)

>can me make it only qi to nsISafeOutputStream if the inner stream actually
>supports it?

this would be nice, but the conditional macro wouldn't make it easy for us to
call QI and _then_ test the result... we could write out the QI code manually,
but that's going too far i think. i have nsBufferedOutputStream::Finish
returning failure if the underlying stream doesn't support it, does that sound
okay? any other ideas?

Comment 15

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

(In reply to comment #14)
> this would be nice, but the conditional macro wouldn't make it easy for us to
> call QI and _then_ test the result... 

you could qi the inner stream to safeoutputstream in Init and store that in a
member var, and use that in the cond. macro (that's what other code does,
viewsource does it that way iirc)

Comment 16

[[e-mail]]

i thought about that but it seemed ugly to me. but if other code does it, i
guess i can abide...

Comment 17

[[e-mail]]

Attachment: fix v2

per biesi. (also removes a redundant fncall in cookieservice's dtor, when the
hashtable goes out of scope it cleans itself up anyway. i verified this.)

Comment 18

[[e-mail]]

Comment on attachment 153500 [details] [diff] [review]
fix v1

mvl, biesi... would both of you mind taking a look? ;)

Comment 19

[[e-mail]]

Comment on attachment 153500 [details] [diff] [review]
fix v1

now, if i can get the right patch...

Comment 20

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

Comment on attachment 153538 [details] [diff] [review]
fix v2

To be honest, i don't really like this patch. It's just too confusing for me.
There are two objects implementing nsISafeOutputStream, and they somehow
magicly talk to each other. And bufferedoutputstream suddenly also knows about
safestream. It's the only thing that knows about it. Why is it special?

I like the other option better: make safefileoutputstream internally use a
buffered stream. And some warning about flushing streams etc in the idl.
Buffereing is for the consumeres, to make their lives easier. This will only be
used for files anyway, and mostly profile files. They will be written quickly
from memory, so buffered makes sense for most, if not all, of them.

Comment 21

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

Comment on attachment 153538 [details] [diff] [review]
fix v2

re-marking my review+ :)


hm... while this special use of nsISafeFileOutputStream in the buffered output
stream is not something I like very much, I don't think your suggestion is much
better... Your way, the safefileoutputstream has special knowledge of a
buffered output stream; this way it's the other way round...

Comment 22

[Darin Fisher]

Comment on attachment 153538 [details] [diff] [review]
fix v2

>Index: netwerk/base/src/nsBufferedStreams.cpp

>+    NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsISafeOutputStream, mSafeStream)
...
> nsBufferedOutputStream::Init(nsIOutputStream* stream, PRUint32 bufferSize)
> {
>+    // QI stream to an nsISafeOutputStream, to see if we should support it
>+    mSafeStream = do_QueryInterface(stream);

nice idea... it can violate COM rules if Init is ever called twice.
let's pretend that that won't happen ;-)


>Index: netwerk/cookie/src/nsCookieService.cpp

> nsCookieService::~nsCookieService()
> {
>   gCookieService = nsnull;
> 
>   if (mWriteTimer)
>     mWriteTimer->Cancel();
>-
>-  // clean up memory
>-  RemoveAllFromMemory();
> }

intended as part of this patch?


sr=darin

Comment 23

[[e-mail]]

Attachment: fix v3

this is what i checked in (i massaged a few comments here and there).

(i did intend to sneak the unrelated cookieservice dtor fix in. it wasn't
really worth its own bug. ;)

Comment 24

[[e-mail]]

fixed on trunk. definitely aviary1.0 fodder.