In all sorts of places, writing a file to the profile can fail if the disk is full, or a crash happens in a real bad time (while writing). Prefs solved this by saving to a temp file first, and has special code for this. This code needs to be made public accessible. If done in the form of a stream, other code can be ported easily.

patch that creates the stream interface. Also makes cookies use it, as a proof-of-concept consumer.

+class nsSafeFileOutputStream : public nsISafeFileOutputStream can this inherit from nsLocalFileOutputStream, to avoid code duplication?

I like Christian's suggestion. Also, we need to document the side-effects of |onSaveFinished|. Like: 1) What happens if |write| is called again? 2) Do I still need to call |close|? Or, does this function call |close| under the hood? 3) What happens if the save fails? 4) Should we expose the location of the temp file? I also think that we should probably not use the verb "save" anywhere. It seems like |onSaveFinished| should be called |onWriteFinished| instead or maybe |onWriteComplete| or |onDoneWriting|. I'm not sure which I prefer.

This patch has an updated interface (two attributes instead of a method), changed inheriting stuff, hopefully better documentation and general cleanup.

Comment on attachment 151157 [details] [diff] [review] second try >Index: netwerk/base/public/nsIFileStreams.idl nit: the comments for these attributes don't read well. here's my suggestions: >+ /** >+ * Should a backup of the original file be created it is overwritten? >+ */ >+ attribute boolean backupTarget; /** * Set this attribute to true to cause a backup of the target file * to be created when the original file is overwritten. * * This attribute is ignored if the write failed for some reason. * * The default value for this attribute is false. */ >+ /** >+ * Indicates if anything during went wrong, and the original target >+ * file should not be overwritten. >+ * Defaults to false, so should be set if everything went ok. >+ * When reading the value, the result will also be false if one of >+ * the calls to Write() did not write out enough bytes. >+ */ >+ attribute boolean writeSucceeded; /** * Set this attribute to true to cause the original file to be * overwritten. Note: if any call to |write| failed to write out * all of the data given to it, then setting this attribute to * true will be ignored. The file will only be saved if all calls * to |write| succeeded and if this attribute is set to true. * * The default value for this attribute is false. */ >Index: netwerk/base/src/nsFileStreams.cpp >+NS_IMETHODIMP >+nsSafeFileOutputStream::Init(nsIFile* file, PRInt32 ioFlags, PRInt32 perm, >+ PRInt32 behaviorFlags) >+{ >+ mTargetFile = file; >+ >+ nsresult rv = mTargetFile->Exists(&mTargetFileExists); >+ if (NS_FAILED(rv)) { >+ NS_ERROR("Can't tell if target file exists"); >+ mTargetFileExists = PR_TRUE; // Safer to assume it exists - we just do more work. >+ } >+ >+ nsCOMPtr<nsIFile> tempResult; >+ rv = mTargetFile->Clone(getter_AddRefs(tempResult)); >+ if (NS_SUCCEEDED(rv) && mTargetFileExists) { >+ PRUint32 origPerm; >+ if (NS_FAILED(mTargetFile->GetPermissions(&origPerm))) { >+ NS_ERROR("Can't get permissions of target file"); >+ origPerm = perm; >+ } >+ rv = tempResult->CreateUnique(nsIFile::NORMAL_FILE_TYPE, origPerm); >+ } >+ if (NS_SUCCEEDED(rv)) { >+ mTempFile = tempResult; >+ } >+ >+ return nsFileOutputStream::Init(mTempFile, ioFlags, perm, behaviorFlags); >+} I think this code doesn't handle errors correctly. Take another look at nsSafeSaveFile::Init. Notice that it returns |rv| at the end, and notice that it only stores mTargetFile if it is successful. In nsSafeFileOutputStream::Init, failures from CreateUnique and Clone are ignored. >+nsSafeFileOutputStream::SetBackupTarget(PRBool aBackupTarget) >+nsSafeFileOutputStream::GetBackupTarget(PRBool *aBackupTarget) >+nsSafeFileOutputStream::SetWriteSucceeded(PRBool aWriteSucceeded) >+nsSafeFileOutputStream::GetWriteSucceeded(PRBool *aWriteSucceeded) collapsing these attributes into a single 'set' style method might be good for codesize. do we need separate attributes? >+nsSafeFileOutputStream::Close() >+{ >+ // XXX Should we clean up in case of failure to close? >+ nsresult rv = nsFileOutputStream::Close(); >+ NS_ENSURE_SUCCESS(rv, rv); yes, we should take care to cleanup properly on errors. close may fail if the OS is unable to flush its disk buffer for some reason. >+ if (!mTempFile) >+ return NS_OK; >+ >+ if (mWriteSucceeded && mInternalWriteSucceeded) { >+ NS_ENSURE_STATE(mTargetFile && mTempFile); nit: we already know that mTempFile is non-null due to the check above. so, no need to check again. >+NS_IMETHODIMP >+nsSafeFileOutputStream::Flush(void) >+{ >+ return nsFileOutputStream::Flush(); >+} >+ >+NS_IMETHODIMP >+nsSafeFileOutputStream::WriteFrom(nsIInputStream *inStr, PRUint32 count, PRUint32 *_retval) >+{ >+ return nsFileOutputStream::WriteFrom(inStr, count, _retval); >+} >+ >+NS_IMETHODIMP >+nsSafeFileOutputStream::WriteSegments(nsReadSegmentFun reader, void * closure, PRUint32 count, PRUint32 *_retval) >+{ >+ return nsFileOutputStream::WriteSegments(reader, closure, count, _retval); >+} >+ >+NS_IMETHODIMP >+nsSafeFileOutputStream::IsNonBlocking(PRBool *aNonBlocking) >+{ >+ return nsFileOutputStream::IsNonBlocking(aNonBlocking); >+} you shouldn't need to override all of these methods. you should just let the compiler take care of it for you. just don't declare these methods in the header, and it will hook up the methods automagically. >Index: netwerk/base/src/nsFileStreams.h >+class nsSafeFileOutputStream : public nsFileOutputStream, >+ NS_DECL_ISUPPORTS >+ NS_DECL_NSISAFEFILEOUTPUTSTREAM >+ NS_DECL_NSIOUTPUTSTREAM >+ NS_DECL_NSIFILEOUTPUTSTREAM just declare the methods that you are overriding. also, i think you want to use NS_DECL_ISUPPORTS_INHERITED. otherwise, you end up declaring a mRefCnt member variable that you don't need -- you are using the one declared by nsFileOutputStream :-) >+ nsSafeFileOutputStream() : >+ nsFileOutputStream(), nit: no need to explicitly call the base class constructor if you are not passing arguments to it. it will get called automagically. >+ virtual ~nsSafeFileOutputStream() { nsSafeFileOutputStream::Close(); } hmm... your Close method calls the base class Close method, but the base class destructor also calls the base class Close method. perhaps that's okay since you do need to close the file stream before doing any file moves. ok. >+ nsCOMPtr<nsIOutputStream> mFileOutputStream; this doesn't appear to be used for anything. leftovers? >Index: netwerk/build/nsNetCID.h add a brief comment explaining what interfaces this guy supports. see other examples in this file. >+#define NS_SAFELOCALFILEOUTPUTSTREAM_CLASSNAME \ >+ "nsSafeFileOutputStream" >+#define NS_SAFELOCALFILEOUTPUTSTREAM_CONTRACTID \ >+ "@mozilla.org/network/safe-file-output-stream;1" >+#define NS_SAFELOCALFILEOUTPUTSTREAM_CID \ >+{ /* a181af0d-68b8-4308-94db-d4f859058215 */ \ >+ 0xa181af0d, \ >+ 0x68b8, \ >+ 0x4308, \ >+ {0x94, 0xdb, 0xd4, 0xf8, 0x59, 0x05, 0x82, 0x15} \ >+} can you also migrate prefs to use this code? (or are you planning to do that in a followup patch?)

>>+NS_IMETHODIMP >>+nsSafeFileOutputStream::Flush(void) >>+{ >>+ return nsFileOutputStream::Flush(); >>+} >>+ > >you shouldn't need to override all of these methods. you should >just let the compiler take care of it for you. just don't >declare these methods in the header, and it will hook up the >methods automagically. I tried that, and i failed. removed NS_DECL_NSIOUTPUTSTREAM, declared the two functions i need, and left out the other. now, gcc complains: /home/michiel/mozhack/tree1/mozilla/netwerk/build/nsNetModule.cpp:108: error: cannot allocate an object of type `nsSafeFileOutputStream' because the following virtual functions are abstract: ../../dist/include/xpcom/nsIOutputStream.h:82: error: virtual nsresult nsIOutputStream::Flush()

nit: coalesce those PRBool members into a contiguous block of PRPackedBools? also, should we document that if the |backupTarget| attribute is set, the backup filename will be the target name with ".bak" appended? can you explain the purpose of setting |writeSucceeded| (i.e., an example where it is useful)? i'm guessing you want it so that a consumer can veto a supposed success, for some reason that this interface isn't aware of? also, in the |writeSucceeded| getter: + *aWriteSucceeded = mInternalWriteSucceeded ? mWriteSucceeded : mInternalWriteSucceeded; you can make that just |*aWriteSucceeded = mInternalWriteSucceeded && mWriteSucceeded;| :)

> can you explain the purpose of setting |writeSucceeded| (i.e., an example where > it is useful)? i'm guessing you want it so that a consumer can veto a supposed > success, for some reason that this interface isn't aware of? yes. All the checks on rv you do between opening and closing. Out of memory errors. It defaults to false because you can have multiple of those failures. Having to set it to false at every check is painful.

Updated to review comments, except that is still has flush() and friends. I couldn't get it to work. I changed the interface somewhat. It now has one method to set writesucceeded and backuptarget. But reading the resulting code again, i'm not sure if i really like it. safeStream->SetWriteSucceeded(PR_FALSE); looks liek it didn't succeed, while it did. It just doesn't want a backup. Suggestions for better names/parameters?

> I tried that, and i failed. removed NS_DECL_NSIOUTPUTSTREAM, declared the two > functions i need, and left out the other. now, gcc complains: perhaps it would be better than to make nsISafeFileInputStream inherit from nsISupports instead. there is considerable convention for doing that sort of thing. cookie code is already QI'ing to nsISafeFileInputStream, so maybe there is little value to having the interface inherit nsIFileInputStream.

<mvl> darin: i'm open to any better inheritance solutions. I couln't make it any better <darin> mvl: well, what do you think about making nsISafeFileOutputStream inherit from nsISupports instead? then you'll get a much simpler inheritance story.

This patch makes nsISafeFileOutputStream inherit only from nsISupports.

dwitte, darin: i didn't obsolete the previous patch. I'm not sure which way is better. To prevent spam, i'm not touching the review flags. Just review the one you like better :)

+ if (count != *result) don't you need to check rv too? I mean, out parameters are only guaranteed to be set if the function succeeded, right?

right, out params are only guaranteed to be set if the function returns a success code.

biesi: yes, you are right. I will fix that.

Comment on attachment 152202 [details] [diff] [review] changed inheritance >+/** >+ * An output streams that makes sure the original file isn't overwritten >+ * until it is known that the writes didn't fail. >+ * Writes to a temporary file, and moves it over the original file >+ * when the stream is closed, and all writes succeeded. If the stream >+ * is closed, and something went wrong during writing, will delete the >+ * temporary file, and not touch the original. >+ */ probably need to revise this comment slightly now that nsISafeFileOutputStream isn't a nsIFileOutputStream in the inheritance sense. how about this: /** * This interface provides a mechanism to control a file output stream * that takes care not to overwrite an existing file until it is known * that all writes to the file succeeded. * * An object that supports this interface is intended to also support * nsIFileOutputStream. * * A file output stream that supports this interface writes to a * temporary file, and moves it over the original file when the stream * is closed, and all writes succeeded. If the stream is closed, and * something went wrong during writing, it will delete the temporary * file and not touch the original. */ sr=darin

Comment on attachment 152202 [details] [diff] [review] changed inheritance to have this actually work, the impl_isupports block should be: NS_IMPL_ISUPPORTS_INHERITED3(nsSafeFileOutputStream, nsFileOutputStream, nsISafeFileOutputStream, nsIOutputStream, nsIFileOutputStream) (added the second line) Let me know if you want a new patch.

Comment on attachment 152202 [details] [diff] [review] changed inheritance + void setWriteSucceeded(in boolean backupTarget); ok... reading setWriteSucceeded(PR_FALSE) is somewhat confusing. Maybe you should name this writeSucceeded? hmm. that wouldn't quite work, as it'd conflict with the writeSucceeded attribute... maybe that one should get a new name as well :) + // It will destroy the targetfile if close() is called twice. s/will/would/ ? + NS_ENSURE_STATE(mTargetFile); I don't really think this needs to be checked in release builds. why not make this an assertion? if people ignore failure from Init and call other methods, they deserve what they get. hmm, although you should probably mention in nsNetUtil.h that Init must be called on the resulting stream, qi'd to nsISafeFileOutputStream + PRInt32 pos = backupFilename.RFindChar('.'); + if (pos != -1) + backupFilename.Truncate(pos); + backupFilename.Append(NS_LITERAL_CSTRING(".bak")); hm, why not just append .bak at the end? so the backup for say prefs.js would be prefs.js.bak. + // Find a unique name for the backup by using CreateUnique hmm, this code looks like it will create an "infinite" number of backups. is that a good thing? + if (NS_SUCCEEDED(rv)) + rv = mTempFile->MoveToNative(nsnull, targetFilename); // This will replace target + } hmm... so if mBackupFile was true, this check looks at CreateUnique's return value. that seems rather unrelated. also, this: + nsCAutoString backupFilename(targetFilename); does not check GetNativeLeafName's rv. maybe you should check that rv immediately after the call and return if NS_FAILED. + NS_ENSURE_STATE(mTempFile); this check is not needed. you checked this at the top of this function and returned early. + if (count != *result) as said above... should be if (NS_FAILED(rv) || count != *result) r=me with that... but maybe you should not create an infinite number of backups.

Comment on attachment 151894 [details] [diff] [review] updated patch so, this one is now obsolete, I guess

After better reading, i found out why prefs needed the backup attribute. It uses it to backup the original file if it guesses that writing the file didn't succeed, by comparing file sizes. If the new file is less then half the size, it does replace the original file, but because it looks like writing failed, it makes a backup. In this new code, we know if the writing was successfull, because we check every write() call. So, a backup isn't needed. And if a consumer really wants it, it can do it for itself. So, i removed the code. It's smeller now, and 'fixed' the issues biesi mentioned Darin, are you ok with this change?

Comment on attachment 152292 [details] [diff] [review] another iteration + NS_ENSURE_STATE(mTargetFile); + NS_ENSURE_STATE(mTempFile); these are still not needed, right? + safeStream->SetWriteSucceeded(PR_FALSE); this should be PR_TRUE, right?

Comment on attachment 152292 [details] [diff] [review] another iteration Darin, can you check if you are ok with the changes on the backup stuff? biesi: yeah, you are right. Will fix.

(turns out this one: + NS_ENSURE_STATE(mTargetFile); _is_ needed, when called from the dtor, when Init failed)

> Darin, are you ok with this change? hmm... so, we are now proposing a change in behavior. previously, we would preserve a backup of the original prefs.js whenever the new file is much smaller than the original regardless of whether or not a write failed. that never seemed like a very clean solution to me. now, we are saying that we will not overwrite the original file unless all writes succeed. ok, so we are protecting ourselves from dataloss with the new design. but, when would we ever want to create a backup of the original? there doesn't seem to be much value in keeping the old backup algorithm. so, do we just discard all pref changes if one of the Write calls fails? perhaps that is best. then there doesn't seem to be any point to the backup flag, right? moreover, it seems like all we really need to know is whether or not the caller passed us all the data it wanted to. because, it's possible that all writes may succeed, but for some reason the caller was not able to completely get all data that needed to be written out. so, we really just need a way for the caller to tell the safe file input stream that it is done writing. hmm... i suppose we could override |nsIOutputStream::flush| and use that as the trigger :-) so, perhaps we don't even need nsISafeFileOutputStream. the caller could be required to call |flush| before calling |close|. calling |flush| would indicate that the caller is happy to have the data flushed to the target file. if they call close or release the stream without flushing it then we assume that they don't want the data persisted to the target file. or does that abuse nsIOutputStream too much? if it does, then i'd be happy with |nsISafeFileOutputStream::doneWriting()| in place of |flush|. thoughts?

(In reply to comment #25) > but, when would we ever want to create a backup of the original? That was my question too. And i answered 'never', so removed the code. > so, do we just discard all pref changes if one of the > Write calls fails? perhaps that is best. Yes. It's dataloss, you loose the changes in your prefs. You don't loose all your prefs. In the old sysem, you end up with a prefs.js that misses half the info, and a backup without the changes. If you really wanted, you could recoved some changed prefs from the truncated file. In the new system you can't. But i doubt anyone will do that. And if the disk is full, there is nothing we could do. So some dataloss is unavoidable. > so, we really just need a way for the caller to tell the > safe file input stream that it is done writing. Yes. That is the writeSucceeded parameter. > hmm... i suppose we could > override |nsIOutputStream::flush| and use that as the trigger :-) That would remove the ability for the caller to check if the write has been successful. So it can't warn the user about it, if it wanted to. Or to try again at a later time. We could use flush(), and only have a readonly parameter in nsISafeFileOutputStream. that way, a caller that doesn't care about failures doesn't have to QI. But i'm not sure if that is worth the less-reable code. flush() still sounds a bit hackish in my opinion.

> That would remove the ability for the caller to check if the write has been > successful. The caller should check the return value of the Write method to determine if writing to the file was successful. > But i'm not sure if that is worth the less-reable code. > flush() still sounds a bit hackish in my opinion. Right, I agree that it is hackish. But, it would be possible to avoid any additional interfaces which might be nice. Ah heck... like I said, make it just be a single method on nsISafeFileOutputStream. IMO, there's no need to expose a method to test whether or not all writes succeeded. The caller should just be expected to check the return value of Write.

(In reply to comment #27) > The caller should check the return value of the Write method to determine if > writing to the file was successful. That, and the number of bytes written (the aCount outparam). The safeoutputstream already does this. Why let the caller also check, if it could just ask? It's a pain to check all the calls. There can be quite a lot of them.

Comment on attachment 152292 [details] [diff] [review] another iteration >Index: netwerk/base/src/nsFileStreams.cpp >+nsSafeFileOutputStream::Init(nsIFile* file, PRInt32 ioFlags, PRInt32 perm, ... >+ if (NS_FAILED(file->GetPermissions(&origPerm))) { >+ NS_ERROR("Can't get permissions of target file"); >+ origPerm = perm; >+ } >+ rv = tempResult->CreateUnique(nsIFile::NORMAL_FILE_TYPE, origPerm); hmm.. interesting. what if |perm| is more restrictive than |origPerm|. seems like we are ignoring |perm| in some cases. perhaps that is okay, but can you at least add a XXX comment prior to checkin? >Index: netwerk/base/src/nsFileStreams.h >+ safeStream->SetWriteSucceeded(PR_FALSE); pass PR_TRUE right? sr=darin

patch checked in

Is this planned for the aviary branch as well?

this fixes what i consider a bug in Close(). if the forwarded call here http://lxr.mozilla.org/mozilla/source/netwerk/base/src/nsFileStreams.cpp#545 fails, i think it's important we propagate that back to the caller... but in that failure case, rv gets clobbered here http://lxr.mozilla.org/mozilla/source/netwerk/base/src/nsFileStreams.cpp#576 by an irrelevant value. (if it succeeds, we'll be lying to the consumer... if it fails, we just have an extra tempfile lying about on-disk. who cares?). sorry for the triviality ;)

Comment on attachment 152849 [details] [diff] [review] fix rv propagation in Close() If you want to propagate back the result of Close(), you should also do that at line 550. (the !mTempFile case)

Comment on attachment 152849 [details] [diff] [review] fix rv propagation in Close() yeah, Close failure should be treated like a Write failure, since Close might fail as a result of not being able to "flush" some buffer to disk. I think it would be better to add a |if (NS_FAILED(rv)) return rv;| line at the top of this function just after calling nsFileOutputStream::Close. then the rest of the |rv| handling would make sense, and this patch wouldn't be needed.

(In reply to comment #34) > I think it would be better to add a |if (NS_FAILED(rv)) return rv;| line at the > top of this function just after calling nsFileOutputStream::Close. then the > rest of the |rv| handling would make sense, and this patch wouldn't be needed. That was what i had in attachment 151157 [details] [diff] [review], asking if we should cleanup. you answered 'yes'. Now you say to return. I'm a bit confused :) So, if close() fails, should the tempfile be removed? Should the original be overwritten? What should the end return value of our close() be?

> That was what i had in attachment 151157 [details] [diff] [review], asking if we should cleanup. you > answered 'yes'. Now you say to return. I'm a bit confused :) > So, if close() fails, should the tempfile be removed? Should the original be > overwritten? What should the end return value of our close() be? oh, you're right. i was thinking in terms of the backup logic that used to exist in the old patch, but we still have to deal with cleaning up the temp file. if nsFileOutputStream::Close fails, then we should remove the temp file, and propogate the failure code. make sense?

Comment on attachment 152849 [details] [diff] [review] fix rv propagation in Close() r+sr=darin ugh, i suck... sorry about that. i totally misinterpreted the patch :-(

> nsresult rv = nsFileOutputStream::Close(); > > // if there is no temp file, don't try to move it over the original target. > // It would destroy the targetfile if close() is called twice. > if (!mTempFile) > return NS_OK; actually, shouldn't this be |return rv;| ?

ah, comment #33... i'll go away now :-/

fixes NS_OK thing too, per darin

Comment on attachment 152982 [details] [diff] [review] v2 this is a trivial fix to a patch that landed a week or so ago.

Comment on attachment 152982 [details] [diff] [review] v2 nevermind. darin came up with a better idea.

filed bug 251091 about solving the rv propagation problems.

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