368317 - temp files remain if MoveToNative fails in nsSafeFileOutputStream::Finish()

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[Kathleen :Brade]

When writing some extension code that uses nsSafeFileOutputStream, I noticed that temp files were not always removed.  I tracked the problem down to the fact that inside nsSafeFileOutputStream::Finish(), mTempFile is not removed when MoveToNative() fails.

Is this behavior intentional?  It seems to me that a failure in the final move should be treated the same as other errors such as failure while writing to the temp file.

I am happy to create a patch if this is indeed a bug.

Comment 1

[Kathleen :Brade]

Possibly related: In one of my main Firefox profiles, I see many files named bookmarks-nn.html.  I'm not sure if this bug is causing that.

Comment 2

[Kathleen :Brade]

For reference, the original work and discussion is in bug 246675.

Comment 3

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

I'm not planning on working on this: -> default assignee.

Comment 4

[Ryan Jones-Ward [:sciguyryan]]

mvl: Is this behaviour actually intended? If not can we actually just remove the backup files without the risk of data loss or could there be another solution too this problem?

Comment 5

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

I guess one could argue that removing the temp file means there is data that's getting lost.
But because the temp file won't be read anyway, I think that it's OK to remove the temp file.

Comment 6

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1

Sounds good MVL, patch to make it so.

Comment 7

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

Comment on attachment 264390 [details] [diff] [review]
Patch v1

>Index: netwerk/base/src/nsFileStreams.cpp
>+        else {
>           nsCAutoString targetFilename;
>           rv = mTargetFile->GetNativeLeafName(targetFilename);

I know you are not changing those two lines, but please do fix the indeting to 4 spaces. That's the file's style.

r=mvl

Comment 8

[Ryan Jones-Ward [:sciguyryan]]

Attachment: For checkin

For checkin. (updated to comments)

Comment 9

[[e-mail]]

do you want this just on trunk? (bug was filed for 1.8 branch)

Comment 10

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #9)
> do you want this just on trunk? (bug was filed for 1.8 branch)
> 

Yea, the "version" should have been set to trunk (and I thought I did that actually).

Comment 11

[[e-mail]]

ok, checked in to trunk.

Comment 12

[Jeff Walden [:Waldo]]

Er.  It has occurred to me that it *is* actually possible to make MoveToNative reliably fail (permissions would be easiest on Unix-ish platforms, at the least), but I'd been thinking that it wasn't when I set this to -.  Resetting to ? since making it fail reliably is *not* difficult, and thanks to biesi for making me think again about this...

Comment 13

[Simon Bünzli]

Could we please get this patch for the branch as well? This would fix 351551 which can lead to notable performance issues.

Comment 14

[Daniel Veditz [:dveditz]]

A branch patch would be nice to have, not sure this is a "blocker" though. If you come up with a branch version please request branch approval for it.

Comment 15

[Simon Bünzli]

Comment on attachment 265425 [details] [diff] [review]
For checkin

So, this patch even still applies cleanly to the 1.8.1 branch. Nice.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 265425 [details] [diff] [review]
For checkin

approved for 1.8.1.5, a=dveditz for release-drivers

Comment 17

[Phil Ringnalda (:philor)]

netwerk/base/src/nsFileStreams.cpp 1.72.8.1