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

RESOLVED FIXED in mozilla1.9alpha5

Status

()

Core
Networking: File
--
major
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Kathleen Brade, Assigned: Ryan Jones)

Tracking

({fixed1.8.1.5})

Trunk
mozilla1.9alpha5
fixed1.8.1.5
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.69 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
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.
(Reporter)

Comment 2

11 years ago
For reference, the original work and discussion is in bug 246675.
I'm not planning on working on this: -> default assignee.
Assignee: mvl → nobody
(Assignee)

Comment 4

10 years ago
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?
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.
Blocks: 157152
(Assignee)

Comment 6

10 years ago
Created attachment 264390 [details] [diff] [review]
Patch v1

Patch v1

Sounds good MVL, patch to make it so.
Assignee: nobody → sciguyryan
Status: NEW → ASSIGNED
Attachment #264390 - Flags: superreview?(cbiesinger)
Attachment #264390 - Flags: review?(cbiesinger)
Attachment #264390 - Flags: superreview?(cbiesinger)
Attachment #264390 - Flags: superreview+
Attachment #264390 - Flags: review?(mvl)
Attachment #264390 - Flags: review?(cbiesinger)
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
Attachment #264390 - Flags: review?(mvl) → review+
(Assignee)

Comment 8

10 years ago
Created attachment 265425 [details] [diff] [review]
For checkin

For checkin. (updated to comments)
Attachment #264390 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Whiteboard: [checkin needed]

Comment 9

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

Comment 10

10 years ago
(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).
Version: 1.8 Branch → Trunk

Comment 11

10 years ago
ok, checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Flags: in-testsuite-
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...
Flags: in-testsuite- → in-testsuite?

Comment 13

10 years ago
Could we please get this patch for the branch as well? This would fix 351551 which can lead to notable performance issues.
Flags: blocking1.8.1.5?
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.
Flags: blocking1.8.1.5? → wanted1.8.1.x+

Updated

10 years ago
Blocks: 351551

Comment 15

10 years ago
Comment on attachment 265425 [details] [diff] [review]
For checkin

So, this patch even still applies cleanly to the 1.8.1 branch. Nice.
Attachment #265425 - Flags: approval1.8.1.5?
Comment on attachment 265425 [details] [diff] [review]
For checkin

approved for 1.8.1.5, a=dveditz for release-drivers
Attachment #265425 - Flags: approval1.8.1.5? → approval1.8.1.5+

Updated

10 years ago
Whiteboard: [checkin needed (1.8 branch)]
Target Milestone: --- → mozilla1.9alpha5
netwerk/base/src/nsFileStreams.cpp 1.72.8.1
Keywords: fixed1.8.1.5
Whiteboard: [checkin needed (1.8 branch)]
You need to log in before you can comment on or make changes to this bug.