Open Bug 200024 Opened 21 years ago Updated 2 years ago

nsIFile::moveTo not implemented consistently across platforms

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

People

(Reporter: darin.moz, Unassigned)

Details

(Keywords: topembed-)

nsIFile::moveTo not implemented consistently across platforms.

WIN32 and mach-o implementations modify the nsLocalFile object to point at the
new file location instead of the old file location.  UNIX implementation
preserves what the nsLocalFile was previously point at.  IMO, the UNIX
implementation is much more consistent with what you'd expect from something
that represents a file path.  of course, changing the behavior of this code is
probably going to cause bugs :-(

bug 198947 is a symptom of this problem.  the bigger problem being that nsIFile
and nsILocalFile are poorly documented.
Status: NEW → ASSIGNED
Keywords: topembed
Priority: -- → P3
Target Milestone: --- → mozilla1.4beta
198947 can be fixed independently of this bug and edt would like to avoid the
risk this one injects; minusing.
Keywords: topembedtopembed-
Um.. This is topembed because it makes life hell for embeddors, not because it
breaks our installer.  The fact that the installer can hack around it is
irrelevant to whether this bug is topembed or not!

Not like that's going to change the EDT's mind, of course....
> the UNIX implementation is much more consistent with what you'd expect from
> something that represents a file path.

I thought that it represented a file, not a file path. After all, the interface
is  called nsIFile, not nsIFilePath.
ccarlen: well, SetLeafName does not rename the file, right?  there are two sets
of functions.. those that affect the file path and those that affect the
underlying file itself.  where we draw the line is not well defined.
so, i tried this little code snipet to test out the behavior of Java's
File.renameTo method (nsIFile is based on Java's File class).

  import java.io.* ;
  class FileTest {
    public static void main(String[] args) {
      File f = new File("a");
      System.out.println(f.getPath());
      f.renameTo(new File("b"));
      System.out.println(f.getPath());
    }
  }

if i had a file named "a" in the current directory, then this program would
rename the file as "b".  the program prints out "a" twice, indicating that
Java's renameTo method does not change the file path referenced by the File
object even though renameTo clearly makes |f| reference a file that no longer
exists.
The nsIFile iface doesn't mention Java's File class, so if I'm using nsIFile,
how would I know to look to Java as the model? I didn't know there was any
connection until you pointed it out.

Win32, Mach-O, and OS/2 all update the file on moving it - making Unix the sole
exception. Not just to say it's easier to change 1 impl instead of 3, but
independent programmers made those 4 implementations and 3 out of 4 expected the
file to be updated on the move. If 3/4 people who use the iface expect that as
well, seems natural to me.

> but independent programmers made those 4 implementations

OS/2 was a copy/paste of the Win32 impl to start with.  Hardly independent.

No sure how exactly OS/X was done, but based on some other things I've seen in
that code, it also borrowed heavily from the win32 code.
conrad: doug said he modeled nsIFile after Java's File class.  while true, it
doesn't mean that we are in any way bound to following Java's implementation, i
just thought it was interesting to see how they interpreted virtually the same
problem.
The nsIFile is an abstraction for the logical file -- not an abstaction of the 
path.  If we go down the road of making nsIFile just represent the path, then 
we had better stop caching the stat info.  (More over, why not just use a const 
char* and NSPR?)  The nsIFile is a stateful object representing the file on 
disk.  If I move the file on disk, i would expect that nsIFile object to 
continue to reference the same file.

i am with conrad.  update the file abstraction after a move fix the IDL to 
reflect this soon-to-be consistent behavior.  
doug:

what about comment #4?  nsIFile represents exactly a file path in some cases and
exactly a file in others.  if nsIFile always represents a file, then why doesn't
SetLeafName move the underlying file?
Darin, as you described in comment #4, we need a set of methods for constructing
an nsIFile object, and a separate set of methods for operating on the object. 
We obviously need more clarity between the two.  Can we do it with more
documentation and fixing the various implementations to be more consistent, or
do we need another interface?
so, i've surveyed consumers of MoveTo/MoveToNative.  most don't care about the
nsIFile object after MoveTo is called.  some do, however.  here's an example of
one that does:

    nsCOMPtr<nsIFile> parentDir;

    rv = mCacheDirectory->GetParent(getter_AddRefs(parentDir));
    if (NS_FAILED(rv))  return rv;

    rv = mCacheDirectory->MoveToNative(uniqueDir, nsCString());
    if (NS_FAILED(rv))  return rv;
     
    // set mCacheDirectory to point to parentDir/Cache/ again
    rv = parentDir->AppendNative(NS_LITERAL_CSTRING("Cache"));
    if (NS_FAILED(rv))  return rv;
     
    mCacheDirectory = do_QueryInterface(parentDir);

notice that if MoveToNative didn't change the state of mCacheDirectory, then
this could have been move compactly written like so:

    rv = mCacheDirectory->MoveToNative(uniqueDir, nsCString());
    if (NS_FAILED(rv))  return rv;

i also found examples in nsAbLDAPReplicationData.cpp, in which it calls
SetLeafName after calling MoveToNative in order to ensure that the state of the
file object is indeed changed to point to MoveToNative.  in that case, it is one
extra line of code:

    rv = mReplicationData->MoveTo(nsnull, newName);
    if (NS_SUCCEEDED(rv))
        rv = mReplicationData->SetLeafName(newName);

currently, on some platforms this is redundant.  if we make the change i'm
proposing than this code would be required.  notice that it is not a lot of code
either compared to the cache example.  if you think about it, MoveTo currently
does two things A) move the underlying file, and B) change what the nsIFile
points at.  some consumers are happy with MoveTo doing A+B, but other consumers
only want step A.  so, they must manually do A+B-B.  so, i'm saying that i think
the WIN32/OSX impls have factored things in a less favorable way.
dougt: also, is it really valid for MoveTo to preserve the cached stat result? 
if MoveTo crosses filesystem boundaries, then the result of stat'ing the file
could be different, right?  i guess, in the majority of cases, MoveTo could
rightly preserve the cached stat result.  but, given that few places in the code
care maybe this isn't a valuable optimization?

gordon: i don't think major interfaces changes are really an option.  this is a
frozen interface, and i think it is sufficient modulo implementation
inconsistencies.  sure, it would be nice to have a clearer separation between
methods which affect only the path and methods which affect only (or also) the
underlying file.
Yes, I realize nsIFile is frozen, which is why (as a future direction) I was
wondering if we needed to transition to a *new* interface.  My hope though is we
can deal with the current problem by making the implementations consistent and
clarifying the documentation.

I think this discussion is a good start on clarification :-)
not another interface!  The problem that darin describes is minor and can be
resolved by properly documenting what our behavior is.

>if nsIFile always represents a file, then why doesn't SetLeafName move the
underlying file?  

apples and oranges.  on the nsIFile exists methods for pointing to a file on
disk (setLeafName, append) and methods for modification/acces of the file
(moveTo).  (at a couple points in history we proposed to break this interface
into to pieces)  Since implementation are expected to delay stat, methods like
append, setLeafName, InitWithPath, simply do path construction.  

plus we have MoveTo(nsnull, new-name).


I think that the developers that need to hold onto the origanal file location
can simply make a clone of the nsIFile before the MoveTo call.  
> also, is it really valid for MoveTo to preserve the cached stat result? 

no, of course not.

>if MoveTo crosses filesystem boundaries, then the result of stat'ing the file
could be different, right?  

yup

>i guess, in the majority of cases, MoveTo could
rightly preserve the cached stat result.  but, given that few places in the code
care maybe this isn't a valuable optimization?

I don't think anyone is asking for that -- we don't really are about the
optimization, but rather that the nsIFile object which is being moved continues
to point at the resulting file which was just moved.
One question.  Say I have an nsIFile object called |file|, ok?  What is the
expected result of the following three sequences of calls? (Each time |file|
starts out with the leafName "file").

1)  file.moveTo(nsnull, "newName");
2)  file.copyTo(nsnull, "newName");
3)  file.copyTo(nsnull, "newName");  file.remove();
1)  file.moveTo(nsnull, "newName");

file points to a "newName" in the same directory as the inital directory.

2)  file.copyTo(nsnull, "newName");

failure - file already exists.

3)  file.copyTo(nsnull, "newName");  file.remove();

Failure - file already exists.  
newName is deleted from the file system;
file now doesn't have any valid stat info, but still points to the location of
where "newName" was.

Wow!  I just had two collision in a row on this bug.

>I think that the developers that need to hold onto the origanal file location
can simply make a clone of the nsIFile before the MoveTo call.  

That doesn't currently work on Mac because of the way FSSpecs/FSRefs/File IDs
work, which the existing implementation depends on.  Cloning an nsIFile and
moving the original results on the clone pointing to the new location on Mac.
re: comment #18, huh?

I don't think that's what Boris meant.  I think he meant each of those three
examples as separate test cases, not that they would be executed in order.  He
states that 'Each time |file| starts out with the leafName "file"', so #3 should
result in "file" being deleted, not "newFile", because CopyTo() doesn't change
what the nsIFile is pointing at...right?
sorry about that.  Assuming no errors:

1)  file.moveTo(nsnull, "newName");

file points to "newFile".  The file "file" has been moved in the file system to
"newName"

2)  file.copyTo(nsnull, "newName");

file still points to "file" and there exists a new file in the same directory as 
"file" named "newName"

3)  file.copyTo(nsnull, "newName");  file.remove();

file still points to "file" and there exists a new file in the same directory as 
"file" named "newName".  Lastly, "file" is removed from the file system.  This
is the same as (2) although not as safe.
OK.  So just checking that we're ok with moveTo and copyTo having different
semantics as far as whether the nsIFile points to the previous or new location
after the operation....
bz: that was my other point.  i don't think they should differ.  it just seems
awkward for those to differ.
CopyTo -- meaning: make a copy of this file and put it there.
MoveTo -- meaning: move this file there.
CopyTo: make a copy of the file named by this path and put it "there".
MoveTo: move the file named by this path to "there".

If nsIFile really represents a path -- and you can't do all the usual "file"
things on it, like write or read, but you can do all the stuff you usually do
with a path, including point to non-existent file -- then I don't think it
should change when we MoveTo.
> Cloning an nsIFile and moving the original results on the clone pointing to
the new location on Mac.

Gordon: That's a bug, and there's a patch which fixes it - bug 164396. It's a
characteristic of FSRef, not FSSpec. Because of that, bug 164396 stops using an
FSRef to represent the file. But, assuming that nasty bug will be behind us, it
shouldn't affect this argument.
shaver - 

the problem is that nsIFile is both a path and a logical file.  some methods, we
treat the nsIFile like a path (setLeafName), others we treat it like a file
object (GetFileSize)


At the end of the day, we should do what is easier for the developer.  

I would like to hear from Pete Colins since he did alot of work to support
nsIFile ease-of-use in js (with his jslib)

All the places where you would pass a string to a POSIX function ("/tmp/foo"),
we use nsIFile.  stat(2) returns file size, permissions, etc., which we've
broken out into a handful of methods.  I think a lot of developer comfort is
"principle of least surprise", and having moveTo mutate the path would certainly
surprise me.

(What happens if you call GetFileSize and your path points to a directory?)
I think what this bug demonstrates is that the principle of least surprise is a
myth, at least in this case.. what it should be called is "the principle of what
I personally would expect" :)

it seems like we need to go with what is going to cause the least pain for the
rest of the codebase, and then notify embeddors of the clarification to the API.
i believe that the nsLocalFileUnix implementation will give us just that -- "the
least pain for the rest of the codebase."  i think my examples demonstrate this.
re: comment #26:

Conrad, directories can be specified in FSSpecs with just the vRefNum, dirID,
and no leaf name, though technically it's not canonical. That could result in
the pathname/(FSSpec|FSRef) incoherence problem for directories, but perhaps our
code avoids that type of use.  Anyway, I'm glad to see bug 164396 getting fixed.
 I was recently bitten by it in my disk cache cleaning code.
QA Contact: scc → xpcom
Assignee: darin → nobody
Status: ASSIGNED → NEW
Target Milestone: mozilla1.4beta → ---
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.