Closed Bug 118203 Opened 23 years ago Closed 22 years ago

[mach] Need an nsLocalFile implementation

Categories

(Core :: XPCOM, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: ccarlen, Assigned: ccarlen)

References

Details

(Keywords: topembed+)

Attachments

(2 files, 7 obsolete files)

This came from bug 110372. Summary and that bug say it all.
Taking.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.8
Summary: Need an nsLocalFile implementation for Mach-0 → [mach] Need an nsLocalFile implementation
Mass move to 0.9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Will fixing this bug fix the following two issues, or should I file seperate
bugs on them?

1) When downloading something, files end up in the /tmp directory when the
download is finished, rather than beign moved to where I told it to save it

2) when trying to upload a file, (like adding an attachment to bugzilla), after
selecting the file through the file picker, the textfield that normall contains
a file name isnt' populated.
Marcus, yes, the problems you mention are due to nsLocalFile.  No need to file 
additional bugs.
Target Milestone: mozilla0.9.9 → mozilla1.0
scc, I have been taking qa on mach-o bugs since most qa contacts do not 
have access to mach-o builds. Do you want to keep this one?
zach, scc is on sabbatical for a few more weeks.  And I doubt he'd mind you 
taking the QA contact on this :-)
oh, well then... Thanks for letting me know sdagley.
QA Contact: scc → zach
nominating mozilla1.0
Keywords: mozilla1.0
chimchim needs this
Keywords: topembed
Keywords: topembedtopembed+
*** Bug 132841 has been marked as a duplicate of this bug. ***
Blocks: 145804
Blocks: 86538
I will gladly test a fix for this bug *IF* it indeed fixes 86538 as well.
Blocks: 150977
Attached patch patch (obsolete) — Splinter Review
The patch does this:
1) Adds nsLocalFileOSX
2) Adds new OSX files to our build of MoreFiles, removes the old HFS impl files
from MoreFiles. The new files added to MoreFiles are from Apple DTS and (IMO)
don't needs review.
3) Changes the registry lib code to use MoreFilesX instead of old MoreFiles. It
was the only consumer of MoreFiles in the tree.
4) Changes to chimera and directory service to make use of the new file impl.

Argh. With the new files, the patch exceeded the 300K attachment limit. So, I
had to ZIP it.
Attached file patch (obsolete) —
The patch does this:
1) Adds nsLocalFileOSX
2) Adds new OSX files to our build of MoreFiles, removes the old HFS impl files
from MoreFiles. The new files added to MoreFiles are from Apple DTS and (IMO)
don't needs review.
3) Changes the registry lib code to use MoreFilesX instead of old MoreFiles. It
was the only consumer of MoreFiles in the tree.
4) Changes to chimera and directory service to make use of the new file impl.

Argh. With the new files, the patch exceeded the 300K attachment limit. So, I
had to ZIP it.
Comment on attachment 88790 [details] [diff] [review]
patch

nevermind this
Attachment #88790 - Attachment is obsolete: true
Attachment #88791 - Attachment is obsolete: true
Attached patch the rest of the patch (obsolete) — Splinter Review
The two patches here should make for easier review. The Apple DTS code is split
off into it's own patch and some chimera code to make use of the new file code
has been removed - that should be its own bug.
are there any possible performance effects w/ this bug?
Comment on attachment 88792 [details] [diff] [review]
patch with only AppleDTS code to be added to MoreFiles

r=sdagley (being is this _is_ the Apple MoreFiles code)
Attachment #88792 - Flags: review+
Blocks: 154135
Attached patch updated patch (obsolete) — Splinter Review
Attachment #88793 - Attachment is obsolete: true
comments:

- you set an arbitrary 512 max length on paths, is there any way to remove that
or figure out how long the path is for an FSRef before you convert it to a path?

- how does that 512 relate to PATH_MAX on other platforms, or on darwin?

+  FileInfo fInfo; // Finder flags are in the same place whether we use FileInfo
or FolderInfo
+  ::BlockMoveData(&catalogInfo.finderInfo, &fInfo, sizeof(FileInfo));
+  if ((fInfo.finderFlags & kIsInvisible) != 0) {
+    *_retval = PR_TRUE;
+  }

don't need to copy data just to check a flag

+  // XXX - Because we define DARWIN, we can't use CFURLGetFSRef. This blows.

file a bug for us to turn that off. we really should not be defining DARWIN when
building mozilla. as a workaround, can't you just #undef DARWIN at the top of
the file before including anything else? we do that in widget/src/cocoa all over
the place.

+  // XXX - This should be cut from the API. Would create an evil dependency.

how do the other platforms handle it?

+  // *outIsPackage = ((mCachedCatInfo.dirInfo.ioFlAttrib & kioFlAttribDirMask) &&
+  // (mCachedCatInfo.dirInfo.ioDrUsrWds.frFlags & kHasBundle));

if we're not going to check the bundle flag, we need to check for more than just
.app (.plugin? or any cfm app that's a package that doesn't end in .app, like
mozilla)

with these fixed, r=pink. conrad and i have already discussed them and he's
working on them. just wanted to include them here so any sr (read: smfr) would
know i'd already brought them up.
Comment on attachment 89068 [details] [diff] [review]
updated patch

r=pink with above comments fixed
Attachment #89068 - Flags: review+
Comments on libreg part of the patch:

Index: modules/libreg/src/reg.c
===================================================================

+static OSErr isFileInTrash(FSSpec *fileSpec, PRBool *inTrash)
+{
+    OSErr err;
+    short vRefNum;
+    long dirID;
+    
+    if (fileSpec == NULL || inTrash == NULL)
+        return paramErr;
+    *inTrash = PR_FALSE;
+    
+    err = FindFolder(fileSpec->vRefNum, kTrashFolderType, false, &vRefNum, &dirID);
+    if (err == noErr)
+        if (dirID == fileSpec->parID)  /* File is inside the trash */
+            *inTrash = PR_TRUE;
+    
+    return err;
+}
+#endif

Can you add a comment here that this won't always work, because the file
may be inside another folder in the trash (but don't bother fixing this time
around). It needs logic like your Mac OS X version above.

+    err = FSResolveAlias(NULL, h, &fsRef, &wasChanged);
+    if (err != noErr)
+        goto fail;
+
+    /* if the alias has changed and the file is now in the trash,
+       assume that user has deleted it and that we do not want to look at it */
+    if (wasChanged && (isFileInTrash(&fsRef, &inTrash) == noErr) && inTrash)

I don't think the 'wasChanged' param means that the file is an alias.
'wasChanged' means that the alias had to be updated, because the target
of the alias moved. To tell if it is actually an alias, test to see if
FSResolveAlias() changed the fsRef (FSCompareFSRefs on the before and
after).

Index: modules/libreg/src/vr_stubs.c
===================================================================

+static const UniChar kOSXRegName[] = { 'g', 'l', 'o', 'b', 'a', 'l', 'r', 'e',
'g', '.', 'd', 'a', 't'};

This is our chance to have a nice human-readable name for Mac OS X.
Can we choose something better? OS 9 has "Application Registry".
How about "Profiles Registry.dat" or somesuch? It would also be nice
to have a unique extension for our registry files, so that they don't
show up as Excel documents in the Finder.

+    if (!err)

if (err == noErr) please (several places).

+        err = FSMakeFSRefUnicode(&foundRef,
UNICHAR_ARRAY_LEN(kOSXRegParentName), kOSXRegParentName,
+                                 kTextEncodingUnknown, &parentRef);

kTextEncodingUnknown? Some source examples use kTextEncodingUnicodeDefault.

+        if (err == fnfErr)

Are you always going to get fnfErr here? Did you test this code path?


+                FSCatalogInfo catalogInfo;
+                ((FileInfo *) &(catalogInfo.finderInfo))->fileType = 'REGS';
+                ((FileInfo *) &(catalogInfo.finderInfo))->fileCreator = 'MOSS';
+                ((FileInfo *) &(catalogInfo.finderInfo))->finderFlags = 0;
+                ((FileInfo *) &(catalogInfo.finderInfo))->location.h = 0;
+                ((FileInfo *) &(catalogInfo.finderInfo))->location.v = 0;
+                ((FileInfo *) &(catalogInfo.finderInfo))->reservedField = 0;

Maybe more nicely written as:
  FileInfo    fInfo = { 'REGS', 'MOSS' };     // rest of fields filled with zero
  BlockMoveData(&fInfo, &catalogInfo.finderInfo, sizeof(FileInfo));

[above comments apply to vr_findVerRegName() too]
Blocks: 150984
Comments on the rest:


Index: xpcom/io/nsLocalFileOSX.cpp
===================================================================

+              nsLocalFile *newFile = new nsLocalFile(mFSRefsArray[mArrayIndex],
nsString());

NS_LITERAL_STRING("") might be slightly more efficient than nsString().

+            err = ::FSMakeFSRefUnicode(&mFSRef, aNode.Length(),
+                                        (UniChar *)PromiseFlatString(aNode).get(),
+                                        kTextEncodingUnknown, &newLeafRef);

Maybe use kTextEncodingUnicodeDefault? (Several other places too.)

+NS_IMETHODIMP nsLocalFile::SetLastModifiedTime(PRInt64 aLastModifiedTime)
...
+#if XP_MACOSX
+  FSRef parentRef;
+  PRBool notifyParent;

Why the #ifdefs here?

+NS_IMETHODIMP nsLocalFile::IsSymlink(PRBool *_retval)
+{
+    NS_ERROR("NS_ERROR_NOT_IMPLEMENTED");
+    return NS_ERROR_NOT_IMPLEMENTED;
+}

Should we implement this for OS X? Should we also consider an alias to be a symlink?

+NS_IMETHODIMP nsLocalFile::Contains(nsIFile *inFile, PRBool recur, PRBool *_retval)
...
+    while ((::FSGetParentRef(&inFSRef, &parentFSRef) == noErr &&
+            memcmp(&parentFSRef, &kInvalidFSRef, sizeof(FSRef)) != 0)) {

Use the MoreFilesX routine to test for a valid FSRef here.

+  *_retval = (::strncmp(PromiseFlatCString(thisPath).get(),
+                        PromiseFlatCString(inPath).get(), thisPath.Length()) ==
0);  
+  

Do paths to directories always end in a separator? If not, then this comparison
might erroneously say that "foo/bar/baz" contains "foo/bar/bazooka/quux".

+NS_IMETHODIMP nsLocalFile::GetParent(nsIFile * *aParent)
...
+#if DEBUG
+  // In a debug build, where components are symlinks, we need to do it this way.
+  // If a file object which is a symlink is asked for its parent, its parent
+  // will be that of the resolved link which isn't what the component mgr
+  // expects.

Oh lordy. It seems very wrong to have code here that is

 i) Debug only
ii) Pandering to some idiosyncracy of the component manager.

It seems quite normal for release builds to have symlinks to other libs
in the component directory. We should either state that this is not
supported, or fix the component manager.

+    if (parentRef == kInvalidFSRef)
+      return NS_OK;

Another place for the proper invalidity check.

+NS_IMETHODIMP nsLocalFile::InitWithPath(const nsAString& filePath)
...
+  // First attempt to convert directly to an FSRef

Say in your comment why this might legally fail.

+  // Argh - since no rtti, we have to use a static cast.
+  // But, we can be sure that this is the only object implementing nsILocalFile.
+  nsLocalFile *asLocalFile = NS_STATIC_CAST(nsLocalFile*, aFile);

You could QI to nsILocalFileMac as a further check.

+  mFSRef = asLocalFile->mFSRef;
+  mNonExtantNodes = asLocalFile->mNonExtantNodes;
+  mTargetFSRef = asLocalFile->mTargetFSRef;
+  mIdentityDirty = PR_TRUE;

Couldn't you factor this code and the copy ctor code using
a shared method?

+  *_retval = PR_Open(path.get(), flags, mode);
...
+  *_retval = fopen(path.get(), mode);

Do NSPR and MSL handle UTF-8 paths OK?

+  // XXX - Because we define DARWIN, we can't use CFURLGetFSRef. This blows.

Why not?


+/* void setFileTypeAndCreatorFromMIMEType (in string aMIMEType); */
+NS_IMETHODIMP nsLocalFile::SetFileTypeAndCreatorFromMIMEType(const char *aMIMEType)
+{
+  // XXX - This should be cut from the API. Would create an evil dependency.
+  NS_ERROR("NS_ERROR_NOT_IMPLEMENTED");
+  return NS_ERROR_NOT_IMPLEMENTED;
+}
+
+/* void setFileTypeAndCreatorFromExtension (in string aExtension); */
+NS_IMETHODIMP nsLocalFile::SetFileTypeAndCreatorFromExtension(const char
*aExtension)
+{
+  // XXX - This should be cut from the API. Would create an evil dependency.
+  NS_ERROR("NS_ERROR_NOT_IMPLEMENTED");
+  return NS_ERROR_NOT_IMPLEMENTED;
+}

Is returning errors here going to cause stuff (like file downloading) to fail?
Maybe LXR for the callers to see, and return NS_OK.

Okay, lots of comments from Simon. We're blocking the major QA certification
push waiting for this as it is the biggest change we're looking at by far. Given
the feedback so far, is the ETA for this still today (Tues.)? 
New patch coming up to address comments:

First Pink's:
1. you set an arbitrary 512 max length on paths, is there any way to remove that

Changed to PATH_MAX which on OSX is 1024. While HFS+ may be capable of unlimited
paths lengths, we do have to feed paths to BSD routines, to which I think that
limit would apply. Anyway, 1024 is pretty darn long.

2. +  // XXX - Because we define DARWIN, we can't use CFURLGetFSRef. This blows.
file a bug for us to turn that off

Filed bug 154232. I was able to do what I needed to without them, though.

3. +  // XXX - This should be cut from the API. Would create an evil dependency.
how do the other platforms handle it?

Suffix alone is all they need. I think that nsLocalFile should not depend on
InternetConfig to make those mappings. Higher level code which saves files can
depend on that, but not xpcom.

4. +  // *outIsPackage = ((mCachedCatInfo.dirInfo.ioFlAttrib &
kioFlAttribDirMask) &&
+  // (mCachedCatInfo.dirInfo.ioDrUsrWds.frFlags & kHasBundle));
if we're not going to check the bundle flag

Fixed it to check the bundle flag.

And Simon's:

1. Can you add a comment here that this won't always work,

Did it.

2. I don't think the 'wasChanged' param means that the file is an alias.

Discussed on AIM. The code is OK afterall.

3. This is our chance to have a nice human-readable name for Mac OS X.

The file names are nicer now and use .regs as suffix (IMO)

4. kTextEncodingUnknown? Some source examples use kTextEncodingUnicodeDefault.

MoreFilesX uses kTextEncodingUnknown. It doesn't matter to HFS+, only used as a
hint for the Finder. Besides, these strings only contain ASCII chars.

5. Maybe more nicely written as:

Did that.

6. NS_LITERAL_STRING("") might be slightly more efficient than nsString()

I don't think so. Especially since the Mach-0 build doesn't have L"" string
constants and the NS_LITERAL_STRING macro has to create a temp object to convert.

7. Why the #ifdefs here?

Only OSX supports that. Initially, I wanted to make this patch for OSX Mach-0
and CFM. Eventually it will be so the #ifdefs will be useful then. There's also
comments: // XXX - Needs work for CFM 

8. Should we implement this for OS X? Should we also consider an alias to be a
symlink?

Implemeted it. Like nsLocalFileMac, I considered aliases to count as symlinks.
symlinks seem to be implicitly resolved by HFS+

9. Use the MoreFilesX routine to test for a valid FSRef here.

Did that.

10. Do paths to directories always end in a separator? If not, then this
comparison might erroneously say that "foo/bar/baz" contains "foo/bar/bazooka/quux".

You're right. Fixed that.


11. Oh lordy. It seems very wrong to have code here that is

 i) Debug only
ii) Pandering to some idiosyncracy of the component manager.

Yeah, I agree. But it works and changing the component mgr in order to avoid
this at this point, I don't think is a good idea.

12.
+    if (parentRef == kInvalidFSRef)
+      return NS_OK;

Another place for the proper invalidity check.

Did that.

13. +NS_IMETHODIMP nsLocalFile::InitWithPath(const nsAString& filePath)
...
+  // First attempt to convert directly to an FSRef

Say in your comment why this might legally fail.

Did that.

14. +  // Argh - since no rtti, we have to use a static cast.
+  // But, we can be sure that this is the only object implementing nsILocalFile.
+  nsLocalFile *asLocalFile = NS_STATIC_CAST(nsLocalFile*, aFile);

You could QI to nsILocalFileMac as a further check.

Did that.


15. Couldn't you factor this code and the copy ctor code using
a shared method?

I ended up changing this routine a little (in order to get around warning from
gcc - see the comment) Because of that, it's less similar to the copy constructor.

16. Do NSPR and MSL handle UTF-8 paths OK?

For Mach-0, there is no MSL. But, under CFM it doesn't support them and was the
main reason I decided to make this Mach-0 only. On Mach-0, NSPR (because Apple's
BSD is underneath) handles it well. All char* mathods on the BSD layer take
*only* UTF-8. I tested this by putting the app in a path with Japanese chars
when the system locale was not set to Japanese (the tough case) and it worked

17. +  // XXX - Because we define DARWIN, we can't use CFURLGetFSRef. This blows.

Why not?

Those routines are inside #ifndef DARWIN in the CoreFoundation headers.

+/* void setFileTypeAndCreatorFromExtension (in string aExtension); */
+NS_IMETHODIMP nsLocalFile::SetFileTypeAndCreatorFromExtension(const char
*aExtension)
+{
+  // XXX - This should be cut from the API. Would create an evil dependency.
+  NS_ERROR("NS_ERROR_NOT_IMPLEMENTED");
+  return NS_ERROR_NOT_IMPLEMENTED;
+}

18. Is returning errors here going to cause stuff (like file downloading) to
fail? Maybe LXR for the callers to see, and return NS_OK.

I did. Only one of those methods is called by anything and the one caller
ignores the error.
Attached patch revised to address comments (obsolete) — Splinter Review
The patch is a concatenation of 2 separate patches: 1 from modules/libreg and
the other from xpcom/io. The rest was not changed.
Comment on attachment 89217 [details] [diff] [review]
revised to address comments

sr=sfraser
Attachment #89217 - Flags: superreview+
Comment on attachment 89217 [details] [diff] [review]
revised to address comments

r=pink
Attachment #89217 - Flags: review+
Checked into CHIMERA_M1_0_BRANCH. Leaving this open until I get it into the
trunk for Mach-0 Seamonkey.
No longer blocks: 147975
Depends on: 154597
Depends on: 154815
Today I tried the latest nightly build (Gecko/20020730) of URL
http://ftp.mozilla.org/pub/mozilla/nightly/latest/MacMozilla-MachO.dmg.gz and
discovered, that the meanwhile applied patches seem to be at least sufficient,
to solve the (see bug 86538) problem of the Aqua build of Mozilla to run from an
UFS partition. On the other hand the trunk nightly build with the usual name
still didn't work at my last trial --- so the difference is probably at least
also these patches were applied to above, but not to the regular pre-release
build yet? So far I had no crashes and most preferences were used from the 1.0
release running in UNIX manner on top of XDarwin, when I checked them. Well
done... My test is insofar conclusive, because I have absolutely no HFS(+)
partition around, but only UFS (and via NFS other case sensitive VFS
implementations, especially ReiserFS and XFS). Of course I leave it to you
active Mozilla developers to close this case as resolved bug, when you think it
is right...
> Today I tried the latest nightly build (Gecko/20020730) of URL
http://ftp.mozilla.org/pub/mozilla/nightly/latest/MacMozilla-MachO.dmg.gz and
discovered, that the meanwhile applied patches seem to be at least sufficient,

It's not in that build - the only build with this code is Chimera.
I was directed to this bug from #mozilla by pinkerton with a request for 
help with a Mac OS 8/9 issue. However, I am not able to determine from 
the report what the problem is. Please forgive me if I'm being dense, but 
please let me know how I can help make this work on Mac OS 8/9. 
Thanks.
The problem was to have an nsLocalFile impl which supported the nsILocalFileMac
interface. For Mach-0, on the trunk anyway, we're using the Unix nsLocalFile
impl which does not support nsILocalFileMac and so does not interface well with
file pickers, drag and drop, aliases, etc.

For OS 8/9, we use the impl in xpcom/io/nsLocalFileMac so I'm not sure what the
relevance of this is to OS 8/9.
i thought the relevance was using the HFS+ api's which would require OS9 for
TARGET_CARBON. additionally, would this impact non TARGET_CARBON at all?
It won't affect non TARGET_CARBON at all. This is only for Mach-0. If we want to
use this impl on CFM TARGET_CARBON, that's a separate issue.
Blocks: 169613
Attached patch additional diffs for trunk (obsolete) — Splinter Review
nsILocalFileMac.idl was changed to add new routines for FSRefs and CFURLs.
Since nsLocalFileMac descends from that, it needs those methods. Only supported
for TARGET_CARBON and that's noted in the .idl

Need review on this portion. The trunk patch is more or less the same as the
patch reviewed already and checked into Chimera - along with the fixes made to
on the Chimera branch.
Comment on attachment 100033 [details] [diff] [review]
additional diffs for trunk

r=sdagley

I'll note a minor nit in that I'm not fond of multiple returns like:

+      if (!parentURL)
+	 return NS_ERROR_FAILURE;
Attachment #100033 - Flags: review+
+    FSRef fsRef;
+    if (::CFURLGetFSRef(aCFURL, &fsRef))

I'd prefer 

  if (::CFURLGetFSRef(aCFURL, &fsRef) != noErr)

and a comment about when this fails (file doesn't exist?).

+      CFURLRef parentURL = ::CFURLCreateCopyDeletingLastPathComponent(NULL,
aCFURL);
+      if (!parentURL)
+        return NS_ERROR_FAILURE;

Should this iterate over all non-root patch components, or is it OK to fail if
more than the leaf is missing?
> +    if (::CFURLGetFSRef(aCFURL, &fsRef))
> 
> I'd prefer 
> 
>  if (::CFURLGetFSRef(aCFURL, &fsRef) != noErr)

Then the logic would be wrong. CFURLGetFSRef doesn't return an OSErr. It returns
a Boolean for success. I could say
if (::CFURLGetFSRef(aCFURL, &fsRef) == PR_TRUE)
to make that more clear.


> Should this iterate over all non-root patch components, or is it OK to fail if
more than the leaf is missing?

It probably should but, with HFS, you run into problems like components other
than the leaf being > 31 chars, nodes having different encodings (Kanji &
MacRoman in one path), etc. Considering the limitations of HFS, I think this
limitation is OK.
> It probably should but, with HFS, you run into problems...

If we only deal with missing leaves, we need to document that in the header that
exposes InitWithCFURL. If InitWithCFURL is called internally from any of the
other init methods that are XP, then we should deal with more than missing leaves.

With that, sr=sfraser
> If we only deal with missing leaves, we need to document that in the header

I added a comment to the header. That method isn't called internally. It's not
even called anywhere at this point. I added it for the sake of the file picker
and drag 'n' drop based on http://developer.apple.com/technotes/tn/tn2022.html.
Attachment #88792 - Attachment is obsolete: true
Attachment #89068 - Attachment is obsolete: true
Attachment #89217 - Attachment is obsolete: true
Attachment #100033 - Attachment is obsolete: true
* The version of FSCopyObject has the fix from the Chimera branch which allows
the object to be renamed when copying.
* MoreFilesX is the same.
* nsLocalFileOSX has fixes made on the Chimera branch.
Comment on attachment 100421 [details] [diff] [review]
latest full patch[changed files]

sr=sfraser, bringing r= forward
Attachment #100421 - Flags: superreview+
Attachment #100421 - Flags: review+
Comment on attachment 100422 [details] [diff] [review]
latest full patch[new files]

sr=sfraser, bringing r= forward
Attachment #100422 - Flags: superreview+
Attachment #100422 - Flags: review+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: