72129 - nsLocalFile::OpenANSIFileDesc() won't create new files

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Patrick C. Beard]

If I call nsLocalFile::OpenANSIFileDesc() with mode = "wb", and the file doesn't 
exist, the file doesn't get created. The problem is that mTargetSpec is invalid 
if the file doesnt' exist. Here's a patch that fixes the problem.

Index: mozilla/xpcom/io/nsLocalFileMac.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/io/nsLocalFileMac.cpp,v
retrieving revision 1.55
diff -b -u -2 -r1.55 nsLocalFileMac.cpp
--- nsLocalFileMac.cpp	2001/03/02 09:26:45	1.55
+++ nsLocalFileMac.cpp	2001/03/15 23:26:06
@@ -1098,15 +1098,5 @@
 		return rv; 
    
-   
-   // Resolve the alias to the original file.
-	FSSpec	spec = mTargetSpec;
-	Boolean targetIsFolder;	  
-	Boolean wasAliased;	  
-	OSErr err = ::ResolveAliasFile(&spec, TRUE, &targetIsFolder, &wasAliased);
-	if (err != noErr)
-		return MacErrorMapper(err);
-		
-		
-	*_retval = FSp_fopen(&spec, mode);
+	*_retval = FSp_fopen(&mResolvedSpec, mode);
 	
 	if (*_retval)

Comment 1

[Patrick C. Beard]

The new disk cache needs this to work.

Comment 2

[Simon Fraser [no longer active]]

Over to conrad

Comment 3

[Conrad Carlen (not reading bugmail)]

mTargetSpec should be valid even if the file doesn't exist (the leaf anyway). I
think the problem is in OpenANSIFileDesc():

	OSErr err = ::ResolveAliasFile(&spec, TRUE, &targetIsFolder, &wasAliased);
	if (err != noErr)
		return MacErrorMapper(err);

If the leaf doesn't exist, ResolveAliasFile will return -43 which means the spec
is valid but this method will die on that.
Should be: if (err != noErr && err != fnfErr)
I think we should resolve aliases in this method but using mResolvedSpec instead
of mTargetSpec, we won't. 

Comment 4

[Conrad Carlen (not reading bugmail)]

Hmm, on second thought, if more than the leaf does not exist, calling
ResolveAndStat won't do the job. It's no going to create missing dirs in the
path. For any kind of "w" permission, OpenANSIFileDesc() should check for
existence and, if not, call Create(). That will work whether the leaf or most of
the path does not exist.

Comment 5

[chris hofmann]

do we need this for 0.8.1 or just for the 0.9 landing of cache next week.
lets set the milestone appropriately..

Comment 6

[Patrick C. Beard]

This needs to be fixed to land the new cache.

Comment 7

[Conrad Carlen (not reading bugmail)]

I'll have a patch shortly...

Comment 8

[Conrad Carlen (not reading bugmail)]

Attachment: Fix which creates file

Comment 9

[Conrad Carlen (not reading bugmail)]

This patch does the complete job. If we are opening with write access, it
ensures the the whole file path as well as the leaf exists first. Also, if we
create the file, we get to do our suffix->file type/creator stuff. If we let
StdCLib do it, we just get a CW text file.

Comment 10

[Simon Fraser [no longer active]]

Is it good to test like this?
+    if (mode[0] == 'w')
Can people open files with "wb" as well as "bw"? I'm not sure if order matters.

Comment 11

[Patrick C. Beard]

Take a look at int __get_file_modes(const char * mode, __file_modes * modes) in 
file_io.c in MSL. It seems to assume that if the mode starts with 'r', then the 
file must exist. But if it starts with 'w' or 'a' then the file needs to be 
created automatically. However, we don't need to do this in nsLocalFileMac, 
FSp_fopen() will take care of it. Of course, if we create it, then we get a 
chance to put our creator/type imprint on it. Ideally, if we open the file using 
a binary mode, it shouldn't have a type of 'TEXT', otherwise it should

Comment 12

[Patrick C. Beard]

Re-reading Conrad's last comment, yes, using extension mapping is good here, but 
also looking at the mode "wb" vs. "w" should use a different file type if 
possible.

Comment 13

[Conrad Carlen (not reading bugmail)]

> Take a look at int __get_file_modes

That's where I just was :-)

> looking at the mode "wb" vs. "w" should use a different file type if possible.

I'll look at this.

Comment 14

[Conrad Carlen (not reading bugmail)]

For consistency's sake, we should let Create() be able to determine the type and
creator given the file's suffix. But, for the case of a file with no suffix,
that will have no effect. I say we initialize mType based on the access mode
right before calling Create():

mType = (mode[1] == 'b') ? 'BiNA' : 'TEXT';

Then, if the suffix -> type/creator stuff can come up with better info, great.
But, we'll be initialized to better defaults if not.

'BiNA' I chose by looking in our bundle... Any better types?

Comment 15

[Simon Fraser [no longer active]]

'BiNA' is good. That was intended for random binary files.

Comment 16

[Conrad Carlen (not reading bugmail)]

Patrick, Simon - Can I get review on this so I can get it in before the cache?

Comment 17

[Simon Fraser [no longer active]]

sr=sfraser

Comment 18

[Patrick C. Beard]

I don't see a new patch which reflects our last comments, such as mode == "a"? Or 
the decision about the creator and type?

Comment 19

[Conrad Carlen (not reading bugmail)]

Attachment: 2nd patch - handles "a" mode and default file type

Comment 20

[Conrad Carlen (not reading bugmail)]

Here's a new patch in which the type defaults to 'BiNA' for files opened with
"b" and "TEXT" otherwise. This is just default. If Create() can find a better
type/creator, it still will. Also, the file is checked or existence for "a" as
well as "w" mode.

Comment 21

[Patrick C. Beard]

r=beard

Comment 22

[Conrad Carlen (not reading bugmail)]

Fix checked in.