164190 - nsSafeSaveFile mistreats nsLocalFile, deleting files under one.

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Simon Fraser [no longer active]]

nsSafeSaveFile has this pattern (mTarget and mBackup are both nsILocalFiles).
Pseudocode:

  mTarget->Clone(&mBackup);
  mBackup->Remove();  // bye bye
  mBackup->GetParent(); // whoops

This hoses the Mac OS X nsLocalFileImpl, which is a separate issue (bug 164189).
It's easy to fix, and makes more sense this way too:

  mTarget->Clone(&mBackup);
  mBackup->Remove();  // bye bye
  mTarget->GetParent(); // ok

Comment 1

[Simon Fraser [no longer active]]

Attachment: nsSafeSaveFile patch; use mTargetFile to get the parent, not mBackupFile

Comment 2

[Simon Fraser [no longer active]]

Attachment: Better patch: just pass nsnull as the parent

Comment 3

[Alec Flett]

Comment on attachment 96412 [details] [diff] [review]
Better patch: just pass nsnull as the parent

sr=alecf

Comment 4

[Conrad Carlen (not reading bugmail)]

Comment on attachment 96412 [details] [diff] [review]
Better patch: just pass nsnull as the parent

r=ccarlen

Comment 5

[Brian Nesse (gone)]

Checked in sfrasers patch.

Comment 6

[Brian Nesse (gone)]

Had to back this out. It turned the "brad" and "Btek" linux Tinderboxen orange.

Comment 7

[Adrian Buckley]

This patch might cause dataloss.

I did a CVS pull & compile yesterday and now my prefs.js AND prefs.bak files
keep getting deleted - this backed-out patch looks a likely candidate ;-)

Comment 8

[Brian Nesse (gone)]

This patch caused bug 164345. From that bug:
>nsSafeSaveFile::CreateBackup() calls CopyToNative with a 1st parameter of
>nsnull.  That's "newParent" in io/nsLocalFileUnix.cpp's CopyToNative(), and
>there's no check for a null newParent before using it.

>Win32 (nsLocalFileWin.cpp) ends up checking for/handling a null newParent.

So it seems that Win and Mac handle this case, but Unix does not.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Please go back to the original (simpler) patch which does not have this bug...

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 96410 [details] [diff] [review]
nsSafeSaveFile patch; use mTargetFile to get the parent, not mBackupFile

un-obsoleting;
r=[e-mail]

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 96412 [details] [diff] [review]
Better patch: just pass nsnull as the parent

obsoleting nsnull version of the patch.  If you want to go back to using this
version, the bug in file IO will need to be fixed first.

Comment 12

[Conrad Carlen (not reading bugmail)]

The cause of this problem, bug 165561, is checked in. Passing nsnull as the
parent to copyToNative now works on Unix. I think the original patch could go
back in now.

Comment 13

[Brian Nesse (gone)]

As per Conrad's comment, landed patch again.

Comment 14

[Stephen Walker]

With my cvs build on windows from 5PM today, I'm seeing prefs.js being deleted 
on shutdown.

Comment 15

[Brian Ryner (not reading)]

Me too, on the Phoenix tinderbox.  This is bad.  Does windows LocalFile
implementation not handle this case?

Comment 16

[Brian Ryner (not reading)]

I think the problem is that in the windows implementation of CopyToNative (which
calls CopyMove), we incorrectly decide that a null parent means we should always
rename instead of copy.  Oops.

Comment 17

[Brian Ryner (not reading)]

patch coming up for what I think is the underlying LocalFileWin problem.

Comment 18

[Brian Ryner (not reading)]

Attachment: LocalFileWin patch

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 100078 [details] [diff] [review]
LocalFileWin patch

r/sr=[e-mail]

/be

Comment 20

[Joe Hewitt (gone)]

Comment on attachment 100078 [details] [diff] [review]
LocalFileWin patch

r=hewitt

Comment 21

[Brian Ryner (not reading)]

patch checked in, reclosing.

Comment 22

[Mike Kaply [:mkaply]]

Argh. This broke OS/2. We are a port of nsLocalFileWin.

Comment 23

[Felix Miata]

OS/2 bug 170638: should it be duped to this and this reopened?

Comment 24

[Mike Kaply [:mkaply]]

OS/2 fix is already in - same as nsLocalFileWin fix.

We should be done here.