246675 - create a non-overwriting file output stream

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

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.

Comment 1

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

Attachment: first try

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

Comment 2

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

+class nsSafeFileOutputStream : public nsISafeFileOutputStream

can this inherit from nsLocalFileOutputStream, to avoid code duplication?

Comment 3

[Darin Fisher]

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.

Comment 4

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

Attachment: second try

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

Comment 5

[Darin Fisher]

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?)

Comment 6

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

>>+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()

Comment 7

[[e-mail]]

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;| :)

Comment 8

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

> 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.

Comment 9

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

Attachment: updated patch

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?

Comment 10

[Darin Fisher]

> 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.

Comment 11

[Darin Fisher]

<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.

Comment 12

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

Attachment: changed inheritance

This patch makes nsISafeFileOutputStream inherit only from nsISupports.

Comment 13

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

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 :)

Comment 14

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

+    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?

Comment 15

[Darin Fisher]

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

Comment 16

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

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

Comment 17

[Darin Fisher]

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 18

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

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 19

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

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 20

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

Comment on attachment 151894 [details] [diff] [review]
updated patch

so, this one is now obsolete, I guess

Comment 21

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

Attachment: another iteration

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 22

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

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 23

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

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.

Comment 24

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

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

Comment 25

[Darin Fisher]

> 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?

Comment 26

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

(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.

Comment 27

[Darin Fisher]

> 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.

Comment 28

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

(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 29

[Darin Fisher]

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

Comment 30

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

patch checked in

Comment 31

[Bogdan Stroe]

Is this planned for the aviary branch as well?

Comment 32

[[e-mail]]

Attachment: fix rv propagation in Close()

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 33

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

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 34

[Darin Fisher]

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.

Comment 35

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

(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?

Comment 36

[Darin Fisher]

> 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 37

[Darin Fisher]

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 :-(

Comment 38

[Darin Fisher]

>    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;| ?

Comment 39

[Darin Fisher]

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

Comment 40

[[e-mail]]

Attachment: v2

fixes NS_OK thing too, per darin

Comment 41

[[e-mail]]

Comment on attachment 152982 [details] [diff] [review]
v2

this is a trivial fix to a patch that landed a week or so ago.

Comment 42

[[e-mail]]

Comment on attachment 152982 [details] [diff] [review]
v2

nevermind. darin came up with a better idea.

Comment 43

[[e-mail]]

filed bug 251091 about solving the rv propagation problems.

Comment 44

[Oscar Manuel Gómez Senovilla]

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