Last Comment Bug 368317 - temp files remain if MoveToNative fails in nsSafeFileOutputStream::Finish()
: temp files remain if MoveToNative fails in nsSafeFileOutputStream::Finish()
Status: RESOLVED FIXED
: fixed1.8.1.5
Product: Core
Classification: Components
Component: Networking: File (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla1.9alpha5
Assigned To: Ryan Jones
:
Mentors:
Depends on:
Blocks: 157152 351551
  Show dependency treegraph
 
Reported: 2007-01-26 07:54 PST by Kathleen Brade
Modified: 2007-06-30 12:45 PDT (History)
12 users (show)
dveditz: wanted1.8.1.x+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.57 KB, patch)
2007-05-10 12:55 PDT, Ryan Jones
mvl: review+
cbiesinger: superreview+
Details | Diff | Splinter Review
For checkin (1.69 KB, patch)
2007-05-20 07:18 PDT, Ryan Jones
dveditz: approval1.8.1.5+
Details | Diff | Splinter Review

Description Kathleen Brade 2007-01-26 07:54:19 PST
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 2007-01-26 07:55:15 PST
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 2007-01-26 09:48:43 PST
For reference, the original work and discussion is in bug 246675.
Comment 3 Michiel van Leeuwen (email: mvl+moz@) 2007-05-10 11:12:02 PDT
I'm not planning on working on this: -> default assignee.
Comment 4 Ryan Jones 2007-05-10 12:16:51 PDT
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@) 2007-05-10 12:29:56 PDT
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 2007-05-10 12:55:54 PDT
Created attachment 264390 [details] [diff] [review]
Patch v1

Patch v1

Sounds good MVL, patch to make it so.
Comment 7 Michiel van Leeuwen (email: mvl+moz@) 2007-05-19 12:07:26 PDT
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 2007-05-20 07:18:16 PDT
Created attachment 265425 [details] [diff] [review]
For checkin

For checkin. (updated to comments)
Comment 9 dwitte@gmail.com 2007-05-20 11:04:14 PDT
do you want this just on trunk? (bug was filed for 1.8 branch)
Comment 10 Ryan Jones 2007-05-20 11:09:14 PDT
(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 dwitte@gmail.com 2007-05-20 11:14:33 PDT
ok, checked in to trunk.
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2007-05-20 18:30:17 PDT
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 2007-06-23 05:19:38 PDT
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] 2007-06-25 10:26:17 PDT
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 2007-06-25 11:49:24 PDT
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] 2007-06-28 11:59:53 PDT
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) 2007-06-30 12:45:06 PDT
netwerk/base/src/nsFileStreams.cpp 1.72.8.1

Note You need to log in before you can comment on or make changes to this bug.