166369 - download "foo.binary" fails at 100% "because an unexpected error occurred" [@ nsIFile.leafName]

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Dennis Stillwaggon]

User-Agent:       Mozilla/4.5 (compatible; iCab 2.8.1; Macintosh; I; PPC)
Build Identifier: ftp://ftp.mozilla.org/pub/mozilla/releases/mozilla1.1/mozilla-mac-11-full.bin



Reproducible: Always

Steps to Reproduce:
1.click on download web page button
2.just as download is done
3.error message appears

Actual Results:  
icon gone, tried several times

Expected Results:  
put .bin file on disk.

Comment 1

[Robert Wall]

*** Bug 166370 has been marked as a duplicate of this bug. ***

Comment 2

[Daniel Wang]

did mozilla crash? What's the error message?

Comment 3

[Frankie]

"/Users/fuy/Desktop/[name_of_download] could not be opened, because an
unexpected error occurred.
Sorry about that. Try saving to disk first then opening the file."

I get this error on a wide range of downloads. Some of the recent ones were:
Apple X11 0.2: http://wsidecar.apple.com/cgi-bin/xii/nph-x11.hqx
Fink 0.5.1:
http://telia.dl.sourceforge.net/sourceforge/fink/Fink-0.5.1-Installer.dmg
two or three others that I forget

The exact symptoms are as follows:
1: click on a download link
2: download window opens
3: observe file named "[random_string].binary" appear on the Desktop (this bug
occurs if and only if the temporary download name ends in .binary)
4: download progress reaches 100%, Mozilla prepares to hand off to Stuffit Expander
5: above error message appears
6: when user clicks OK, the download disappears

Now here's where it gets really weird. If you switch to the Finder without
closing the error message, "foo.binary" changes into the appropriate file name
and icon. I was able to successfully open the .dmg (with verified checksum) and
extract its contents.

Also, if you obey the error message and use the right-click "Save Link Target
As..." instead of an ordinary click, the download works fine (although you need
to manually run the helper app).

Comment 4

[Frankie]

Attachment: screenshot of me dodging the error and saving a download

Oh forgot to mention, I've seen this error in release 1.2.1, nightly 20021220,
nightly 20030203, nightly 20030207, and various others.

OS X 10.2.3, 640MB RAM, 100bT LAN, 20GB HD with 2 HFS+ partitions

Comment 5

[Frankie]

Another unknown error at
http://prdownloads.sourceforge.net/fire/Fire.app0.32.b.2.dmg?download

You can definitely dodge the bug by switching to the Finder when the error
dialog pops up, then closing the dialog after you have finished installing the
download.

Comment 6

[Frankie]

Further info. A trio JS Console errors appear during the download process:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIFile.path]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
file:///Applications/Mozilla/Mozilla.app/Contents/MacOS/components/nsHelperAppDlg.js
:: anonymous :: line 416"  data: no]

Error: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIFile.leafName]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
file:///Applications/Mozilla/Mozilla.app/Contents/MacOS/components/nsProgressDialog.js
:: anonymous :: line 374"  data: no]
Source File:
file:///Applications/Mozilla/Mozilla.app/Contents/MacOS/components/nsProgressDialog.js
Line: 374

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIFile.leafName]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
file:///Applications/Mozilla/Mozilla.app/Contents/MacOS/components/nsProgressDialog.js
:: anonymous :: line 374"  data: no]

FWIW, this happened while downloading Adobe's SVG Viewer in Mach-O 20030212.

Comment 7

[Frankie]

Here's the relevant section of nsProgressDialog.js

            // Target is the "preferred" application.  Hide if empty.
	    if ( this.MIMEInfo && this.MIMEInfo.preferredApplicationHandler ) {
                var appName = this.MIMEInfo.preferredApplicationHandler.leafName;
                if ( appName == null || appName.length == 0 ) {
                    this.hide( "targetRow" );
                } else {

Apparently leafName does not exist for certain mime types.

Comment 8

[Torben]

I see this too (tested with 1.4a and 1.4rc1) but only if I try to save to a
location _not_ on the system disk.

Comment 9

[Pete Hammer]

I get this too, even on the system disk.  Mac OSX, build 2003052703.  Hadn't
tried  the sneak-in-and-use-it-anyway-before-accepting-the-error trick...

Save Target As generally works, assuming you can get to a link that has the
file.  Auto (scripted) downloads without a direct link are a problem.

a .dmg file was my latest attempt (the Keynote update):
http://wsidecar.apple.com/cgi-bin/kn/nph-KeyNote.hqx

the autostart download fails at 99%, right clicking and doing save target as on
the link: 
http://a1408.g.akamai.net/7/1408/1388/20030603/akamai.info.apple.com/Keynote/SBML1/061-0483.20030603.MPkt8/KeynoteUpdate1.1.dmg

works.

I don't see a mime type for the .dmg type in my prefs.

Comment 10

[Len Laughridge]

I was just able to reproduce this again with the 19 June RC_3  version of Mozilla.

The key for me is that it ONLY happens when logged in as a user with an NFS
homedir and downloading to the NFS volume.  Logged in as a local user with an
HFS+ home directory and saving to an HFS+ volume works fine.  This is consistant
and repeatable.  I've been noticing it since about RC_1.  

This seems to be a regression to a bug that existed in 1.2.1 but which has been
fixed for a while.  Please let this block 1.4 until it is resolved.  Thank you.

Comment 11

[Frankie]

Still happens to me on the system partition of an HFS+ drive. Typically affects
about half of the .dmg files that I download. Sneaking to the Finder is a
consistent workaround.

Comment 12

[Asa Dotzler [:asa]]

mozilla1.4 shipped. unsetting blocking1.4 request.

Comment 13

[Len Laughridge]

Still happens with 1.5a.  Both with browser downloads and email attachments. 
Re-reqesting blockage, for 1.5b this time.

Comment 14

[chris hofmann]

blake can you have a look?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Someone with Mac file clue needs to look.  Why would a mac nsIFile throw on 
accessing leafName?  leafName should NEVER throw for an initialized nsIFile... 
It looks like the code in nsLocalFile::GetNativeLeafName (the OSX version will 
throw if the leafname happens to be empty, and I bet the code just doesn't deal 
with some difference in directory separators or something...)

Comment 16

[Steve Dagley]

I think the problem may be that MIMEInfo isn't valid althought I don't know why
the test for that would fail.  See
http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/mac/nsOSHelperAppService.cpp#78
for a workaround I did for Camino in the case of not finding a MIMEInfo for a
helper app.  If MIMEInfo is valid and it's just preferredApplicationHandler
that's uninitialized Conrad's comment was "the easy thing to do would be to make
getLeafName return an empty string if uninitialized"

Comment 17

[Boris Zbarsky [:bzbarsky]]

Well, in this case there is a non-null nsIFile there.  I don't see any 
codepaths that would stick an uninited nsIFile there, and an unitited one would 
throw NS_ERROR_NOT_INITIALIZED (see 
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileOSX.cpp#528), not 
the NS_ERROR_FAILURE that we do throw.

If someone seeing this problem could catch the exception (with venkman or by 
directly editing the chrome) and post what the filepath is when things break 
(assuming that .path works and all), that would be great.

Comment 18

[Conrad Carlen (not reading bugmail)]

Attachment: patch

I think this is wallpaper but, if that's what the other impls do...

Comment 19

[Allan Sanderson]

Am wrestling with something similar from what I read here.

Am working on an nfs mounted home folder and am seeing this:
When first launched (having deleted previous prefs) all downloads work for that
launch. On subsequent launches I get download errors. Most obvious thing that I
see is that on launch 1 the download manager pops up and asks if I would like to
open the target download or save it to disk, subsequent launches do not pop up
this window they jump straight to asking where I would like to save the file. It
is when this happens that I get the error "<path_to_download> could not be
saved, because an unknown error occurred. Sorry about that. Try saving to a
different location" if I then select a location on the internal HD then the
download works fine. However if I select anywhere in the nfs mount then it
fails. Only way that I can download successfully is to Control-Click on the link
and select "Save Link Target As..." this works every time.

I have tried this when running with a home folder on the HD and all is fine, but
when working from a network home folder (be that AFP or NFS) then I get these
same download issues.

Comment 20

[Allan Sanderson]

Should have included this info, sorry!

Mozilla 1.5a
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5a) Gecko/20030721

Mac OS X 10.2.6 (6L60)
Apple PowerBook 17", 1GHz, 512MB RAM, 60GB HD.

Comment 21

[Conrad Carlen (not reading bugmail)]

> and an unitited one would 
> throw NS_ERROR_NOT_INITIALIZED (see 
> http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileOSX.cpp#528), not 
> the NS_ERROR_FAILURE that we do throw.

Boris, nsLocalFileOSX was pretty much re-written Apr 18 06:50. Before that, when
comment 6 was written, the old impl would return NS_ERROR_FAILURE when asked for
the leafname of a non-extant file.

Comment 22

[Darin Fisher]

Comment on attachment 129830 [details] [diff] [review]
patch

sr=darin

i agree that it is good to make the OSX impl behave like the other impls, and
yeah this is probably just wallpaper in this case.

Comment 23

[Frankie]

Confirming that the Javascript Console no longer reports any error in nsiFile or
leafName. Currently using Moz 1.4 final.

Downloads to my startup drive (HFS+, OSX 10.2.6) still fail with disappointing
regularity; Fire.app from Sourceforge is a repeatable testcase.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Could we please try to find the real problem in addition to the nsIFile 
change?  It's likely that a problem like this is a symptom of correctness 
issues, if the MIMEInfo is bogus...

Comment 25

[Conrad Carlen (not reading bugmail)]

Frankie: Fire.app from Sourceforge WFM.

> The exact symptoms are as follows:
> 1: click on a download link
> 2: download window opens

In #2, which window? One asking what to do with the file? If so, does it claim
that it's "Untyped Binary Data?" Or, do you mean the download progress window? 

For me, it's untyped binary data. Once in that position, either of the two
possible options (open it with, or save to) should provide the info needed
without relying on MIME info.

Sounds like in your case, it thinks it has valid MIME info, so it doesn't ask,
but the MIME info is not valid. If you have IE around, can you use it to look at
 Internet Config's MIME info for .dmg and see if there's a bogus entry?

Comment 26

[Frankie]

Conrad, yes, I meant the progress window (or download manager, depending on
prefs). Mozilla rarely gives me a file dialog for a download; just once in a
while with some PDFs. My default location is ~/Desktop/

On both of my Macs (OSX 10.2.6, lots of RAM, yadda yadda), Internet Config has
no entry at all for dmg. It skips from dl to doc.

Comment 27

[Asa Dotzler [:asa]]

1.5b shipped. moving blocking request forward.

Comment 28

[Christian Reis]

over to patch author

Comment 29

[Asa Dotzler [:asa]]

Not going to block 1.5 for this bug but we would consider approving a fully
reviewed and tested patch if the reviewers think it's safe enough for this late
stage in 1.5.

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 129830 [details] [diff] [review]
patch

Darin says this is good for 1.5. a=asa (on behalf of drivers) for checkin to
Mozilla 1.5. Time is short so let's get this in quickly. Thanks.

Comment 31

[Darin Fisher]

see also bug 215089.  nsLocalFileOSX.cpp:MoveTo fails if there is not a
"Temporary Items" folder on the volume.  my guess is that this bug is a
duplicate, but folks will need to confirm that.

Comment 32

[Darin Fisher]

please test this bug against tomorrow's build.  it might be fixed.

Comment 33

[Dave Miller [:justdave]]

Per Darin's request to test in comment #32, this is NOT fixed in build 20030914.

Comment 34

[Hiro]

This bug is still reproduced.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; ja-JP; rv:1.5b) Gecko/20030914
Firebird/0.6.1+

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 129830 [details] [diff] [review]
patch

This never got checked in on trunk or on the branch... should it have been? 
Does it actually fix the issues people are seeing?  Could someone with a mac
build please test?

Comment 36

[Paul]

in reply to #35, yes this is still a bug, see bug 222712 (blocked bug at top).

Comment 37

[Boris Zbarsky [:bzbarsky]]

I realize that this is still a bug.  My question was whether someone on Mac
could please test the patch in this bug and see whether it helps.

Comment 38

[Paul]

is it possible to patch a nightly build?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Not with this patch.... you need to build from source to apply it.

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: other patch to try

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: missed a spot

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 129830 [details] [diff] [review]
patch

This is not actually needed... if nothing else, Windows will throw on
GetLeafName calls in similar circumstances.  The real issue here is the
uninitialized localfile hanging around....

Comment 43

[Dave Miller [:justdave]]

Comment on attachment 133644 [details] [diff] [review]
missed a spot

doing some testing with bz and beisi in IRC, we discovered (at least in my
case) the problem was leftover helper-app selections from when I used to run
CFM builds.  My pdf helper was set to "Jaguar:Applications:Acrobat Reader 5.0".
 Re-selecting the application changed it to "/Applications/Acrobat Reader 5.0".
 Going on that lead, beisi cooked up this patch, which correctly reports "the
helper application you chose can't be found" instead of "unknown error". 
tested and works on mine.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133644 [details] [diff] [review]
missed a spot

Thanks to justdave for testing!  This patch is money.  What's going on here is
the following:

1)  User has a profile that was used with a CFM build once upon a time.  The
mimeTypes.rdf file contains paths using ':' as the separator.

2) When we read this file and create mimeInfo objects, we call
GetFileTokenForPath() on the paths.  The Mac version of this creates a local
file, inits it with a path, and returns it.  Without this patch, it does not
check whether the init succeeded, but nsLocalFileOSX::InitWithPath throws if
the path does not start with '/'.  Hence our uninitialized nsLocalFileOSX
object.

3)  Once issue #2 is fixed, the LaunchAppWithTempFile code needs to be fixed to
return the right error code so that a useful error is reported to the user.

With this patch, instead of getting "unknown error" the user gets a useful
error message (see bug 222501 on making that error message even more useful).

Simon, would you sr?  Also, should we be attempting to handle ':'-separated
paths in this code?  If so, how can we do it?

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

also from me much thanks to bz and justdave for the help with this bug

taking bug

Comment 46

[Steve Dagley]

For compatability with paths created in the CFM build it wouldn't be
unreasonable to have nsLocalFileOSX in Mach-O builds deal with paths containing
a ":".  In nsLocalFile::InitWithNativePath we call
::CFURLCreateWithFileSystemPath() with kCFURLPOSIXPathStyle as the pathStyle
parameter.  If that call fails we could try repeatng the call with
kCFURLHFSPathStyle as the pathStyle parameter before we give up and return
NS_ERROR_FAILURE.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Attachment: Like this?

Totally untested; not even compiled (since I have no Mac to do so on).	Could
someone try this out?

Marking biesi's patch obsolete, since I checked it in an hour ago.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Attachment: Fixed so it compiles

justdave says this works like we want it to...

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133662 [details] [diff] [review]
Fixed so it compiles

Thoughts?

Comment 50

[Brendan Eich [:brendan]]

Just wondering whether the comment at
http://lxr.mozilla.org/mozilla/source/xpcom/string/obsolete/nsString.cpp#344 is
obsolete, and ReplaceSubstring works in all cases, or at least all used cases.

/be

Comment 51

[Steve Dagley]

Based on ccarlen's comment on the crash he discovered when he wrote
nsLocalFileOSX it worked in this usage.  Hopefully he'll see the activity on
this bug tomorrow and can verify.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Replacing that makes the string shorter seems to work reasonably well in
general... see also the comment at
http://lxr.mozilla.org/mozilla/source/xpcom/string/obsolete/nsString.cpp#362
(which is NOT the case this falls into).

Comment 53

[Conrad Carlen (not reading bugmail)]

This won't work.

CFURLRef aURLRef = CFURLCreateWithFileSystemPath(nil, pathAsCFString,
                                                kCFURLPOSIXPathStyle, false);

As a test, I inited pathAsCFString to an HFS path, yet
CFURLCreateWithFileSystemPath happily returned a non-null CFURLRef.
CFURLCreateWithFileSystemPath does not check the validity of the given path, as
that would make it really expensive. Likewise, we can't afford to stat in
InitWithNativePath. It requires a POSIX path, and thats that.

There are a few consumers which may have HFS paths stored on disk. They are
fairly few, but they need to do the HFS fallback themselves. See
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsURLHelperOSX.cpp#177.
Notice it does a quick string comparison on the given path against a list of
volume names to avoid doing a stat on every given path.
  

Comment 54

[Boris Zbarsky [:bzbarsky]]

ccarlen, thanks for the advice.  So we should do this at the helperappservice
level, I guess....  I could try to work on that for 1.6b if no one else will,
but I have no way to even compile the code, much less test it, so if someone
with a Mac system could take over this mac-only issue, that would be very much
appreciated.... ;)

Comment 55

[Boris Zbarsky [:bzbarsky]]

*** Bug 223316 has been marked as a duplicate of this bug. ***

Comment 56

[Boris Zbarsky [:bzbarsky]]

ccarlen, would it make sense to expose a function which will do this hfs
fallback business on nsILocalFileMac?  That way necko and helper apps could both
just call it....  Copying that necko code over to helper apps seems like a bad
idea, and nsURLHelper is not exactly exported by necko....

Comment 57

[Steve Dagley]

ccarlen is on vacation for a few days so you might not get a response until he's
back next week

Comment 58

[Christian :Biesinger (don't email me, ping me on IRC)]

given that now bz's doing the work here, -> him

Comment 59

[Boris Zbarsky [:bzbarsky]]

justdave, are you actually trying to do this? If so, please take the bug....

Comment 60

[Dave Miller [:justdave]]

yeah.  This would be my first attempt ever at modifying Mozilla code, but I have
an OS X dev environment (which is more than most of the folks on this bug by the
sound of it) and am willing to hack on it with some coaching.  At the moment I
was stalling for response to bz's question of ccarlen, but I guess we'll be a
while to hear back on it.

Comment 61

[Conrad Carlen (not reading bugmail)]

> ccarlen, would it make sense to expose a function which will do this hfs
fallback business on nsILocalFileMac

I don't want to contaminate the nsILocalFileMac API with this sort of thing. If
we had nsILocalFileService maybe, but not as in instance method. If file
consumers have legacy data stored (HFS paths), it's up to them to convert it.
Using an nsILocalFileMac already requires an #ifdef, and the native code for
doing the conversion using CFURL is pretty simple.

Comment 62

[Dave Miller [:justdave]]

ok, so where does this need to happen then?

Comment 63

[Boris Zbarsky [:bzbarsky]]

http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/mac/nsOSHelperAppService.cpp#179

Comment 64

[Dave Miller [:justdave]]

*** Bug 222712 has been marked as a duplicate of this bug. ***

Comment 65

[Boris Zbarsky [:bzbarsky]]

*** Bug 219730 has been marked as a duplicate of this bug. ***

Comment 66

[Dave Miller [:justdave]]

Attachment: CFM->Posix in mac/nsOSHelperAppService

Comment 67

[Boris Zbarsky [:bzbarsky]]

> +    pathAsCFString = ::CFURLCopyFileSystemPath(pathAsCFURL,
kCFURLPOSIXPathStyle);

Don't you need to CFRelease the previous value of pathAsCFString first?  That
release should probably be unconditional, and only the return conditioned on
pathAsCFURL being null.

Similarly, it looks like the release of pathAsCFURL should be unconditional and
that pathAsCFString should be released again after we copy the chars to thePath.

I'd declare all this CF* stuff inside the if (thePath.First() != '/') block.

nsCAutoString pathAsCString;
CopyUTF16toUTF8(thePath, pathAsCString);

is probably shorter and faster as:

NS_ConvertUTF16toUTF8 pathAsCString(thePath);

> +    thePath = nsDependentString(::CFStringGetCharactersPtr(pathAsCFString));

The docs at
http://developer.apple.com/documentation/CoreFoundation/Reference/CFStringRef/Reference/function_group_4.html#//apple_ref/c/func/CFStringGetCharactersPtr
make me a little queasy about this call (in particular the "you can't depend on
this to do anything consistent or anything at all" verbiage)...

But this definitely looks like the right general approach.  I assume it fixes
things in your build?

You may want to hear what sdagley has to say on the various CF* things before
changing things in response to my comments, btw... but do fix the way
pathAsCString is declared.

Comment 68

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 133662 [details] [diff] [review]
Fixed so it compiles

clearing sr request on obsolete patch

Comment 69

[Dave Miller [:justdave]]

Declaring all the stuff at the top is left over from my experience with
Objective C (which requires you to declare everything at the top).  This is the
first time I've touched C++ in about 5 years :)

Yeah, this fixed it on mine.  Rearranged some code on my copy to make it
prettier, but I'll await sdagley's comments before posting a new patch.

Comment 70

[Asa Dotzler [:asa]]

I'm not sure how active sdagly is with Mozilla so perhaps you should get a new
patch up and we should find someone else to take a look at it.

Comment 71

[Dave Miller [:justdave]]

Attachment: another try

This is updated per bz's comments and some of my own.  It's still compiling so
I can't test it yet (I'll post here once I have) but with the code freeze
imminant, I figured it'd be good to get it up here and looked at.

Comment 72

[Darin Fisher]

Comment on attachment 135887 [details] [diff] [review]
another try

>+    thePath = nsDependentString(::CFStringGetCharactersPtr(pathAsCFString));

documentation on developer.apple.com says that CFStringGetCharactersPtr
may return NULL in some cases.	it says that you should call
CFStringGetCharacters instead (or at least do so in cases where 
CFStringGetCharactersPtr returns NULL).

Comment 73

[Conrad Carlen (not reading bugmail)]

Comment on attachment 135887 [details] [diff] [review]
another try

>Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp
>===================================================================

>-nsresult nsOSHelperAppService::GetFileTokenForPath(const PRUnichar * platformAppPath, nsIFile ** aFile)
>+nsresult nsOSHelperAppService::GetFileTokenForPath(const PRUnichar * aPlatformAppPath, nsIFile ** aFile)
> {
>   nsCOMPtr<nsILocalFile> localFile (do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
>   if (!localFile)
>     return NS_ERROR_FAILURE;

    // Create an nsILocalFileMac - it will make things simpler below
    nsCOMPtr<nsILocalFileMac> localFile
(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));

>   
>-  nsresult rv = localFile->InitWithPath(nsDependentString(platformAppPath));
>+  nsAutoString thePath(aPlatformAppPath);
>+
>+  if (thePath.First() != '/') {
>+    // if it doesn't start with a / it's not an absolute Posix path
>+    // let's check if it's a CFM path left over from old preferences
>+    // and convert it to a Posix path via CFURL
>+
>+    // If it starts with a ':' char, it's not an absolute CFM path
>+    // so bail for that, too.
>+    if (thePath.IsEmpty() || thePath.First() == ':')
>+      return NS_ERROR_FILE_UNRECOGNIZED_PATH;
>+
>+    nsCAutoString pathAsCString;
>+    CFStringRef pathAsCFString;
>+    CFURLRef pathAsCFURL;
>+
>+    CopyUTF16toUTF8(thePath, pathAsCString);
>+    pathAsCFString = ::CFStringCreateWithCString(nsnull,
>+                                                 pathAsCString.get(),
>+                                                 kCFStringEncodingUTF8);

No need to convert to a UTF-8 cstring - just use:
I'm not sure if nsCRT is still the way, but...

      CFStringRef pathAsCFString = ::CFStringCreateWithCharacters(NULL,
		      platformAppPath, nsCRT::strlen(platformAppPath));

>+    if (!pathAsCFString)
>+      return NS_ERROR_FAILURE;
>+
>+    pathAsCFURL = ::CFURLCreateWithFileSystemPath(nsnull, pathAsCFString,
>+                                                  kCFURLHFSPathStyle, PR_FALSE);
>+    ::CFRelease(pathAsCFString);
>+    if (!pathAsCFURL)
>+      return NS_ERROR_FAILURE;

Definately don't use CFStringGetCharactersPtr
But, there's no need to. Since you have a CFURL and an nsILocalFileMac,
you can just say:

      localFile->InitWithCFURL(pathAsCFURL);
      ::CFRelease(pathAsCFURL);

InitWithCFURL simply addrefs the given CFURL (Its internal representation is a
CFURL) But, CFURL does not normalize a URL created with an HFSPlus path and,
since we'll only ever ask for its POSIX path, it will be converted internally
each time. That might be fine in this case, since this file will be used once
to launch an app (?)

Or, to get the POSIX path without using CFURLCopyFileSystemPath() followed by
the dreaded CFStringGetCharactersPtr():

      char pathBuf[PATH_MAX]
      ::CFURLGetFileSystemRepresentation(pathAsCFURL, true, (UInt8*)pathBuf,
					 sizeof(pathBuf));
      localFile->InitWithNativePath(nsDependentCString(pathBuf));

>+
>+    pathAsCFString = ::CFURLCopyFileSystemPath(pathAsCFURL, kCFURLPOSIXPathStyle);
>+    ::CFRelease(pathAsCFURL);
>+    if (!pathAsCFString)
>+      return NS_ERROR_FAILURE;
>+
>+    thePath = nsDependentString(::CFStringGetCharactersPtr(pathAsCFString));
>+    ::CFRelease(pathAsCFString);

Need an "else" to avoid falling thru to InitWithPath

>+  }
>+
>+  nsresult rv = localFile->InitWithPath(thePath);
>   if (NS_FAILED(rv))
>     return rv;
>   *aFile = localFile;

Comment 74

[Dave Miller [:justdave]]

Attachment: This looks better

> Create an nsILocalFileMac - it will make things simpler below

Holy cow.  No kidding.	Try this out for size, it looks a LOT cleaner :)
Tested and works on OS X 10.3.1

Valid HFS paths work, invalid ones throw an error (i.e. volume renamed)
Posix paths still work, too.

> Need an "else" to avoid falling thru to InitWithPath

Actually, we don't, because the "else" case is "the filename is Posix format
already" in which case we wanted to go ahead and call InitWithPath on it
without modifying it.  But that's irrelevant because the whole structure
changed anyway with the initWithCFURL revelation :)

Comment 75

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135970 [details] [diff] [review]
This looks better

>Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp
>+  nsCOMPtr<nsILocalFileMac> localFile (do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
>   if (!localFile)
>     return NS_ERROR_FAILURE;

Actually, I'd prefer that this was more like:

nsresult rv;
nsCOMPtr<nsILocalFileMac> localFile
  (do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv));
NS_ENSURE_SUCESS(rv, rv);

>+  if (!pathAsCFString)
>+    return NS_ERROR_FAILURE;

Wouldn't NS_ERROR_OUT_OF_MEMORY be more appropriate?

>+    if (!pathAsCFURL) {
>+      ::CFRelease(pathAsCFString);
>+      return NS_ERROR_FAILURE;

Same.

>+    if (::CFStringGetLength(pathAsCFString) == 0 ||
>+        ::CFStringGetCharacterAtIndex(pathAsCFString, 0) == ':')
>+      return NS_ERROR_FILE_UNRECOGNIZED_PATH;

That leaks pathAsCFString

>+    if (!pathAsCFURL) {
>+      ::CFRelease(pathAsCFString);
>+      return NS_ERROR_FAILURE;

Again, NS_ERROR_OUT_OF_MEMORY

>+  nsresult rv = localFile->InitWithCFURL(pathAsCFURL);

You need to CFRelease the url and string after this call... Otherwise you leak
them.

Comment 76

[Dave Miller [:justdave]]

>>+  if (!pathAsCFString)
>>+    return NS_ERROR_FAILURE;
>
>Wouldn't NS_ERROR_OUT_OF_MEMORY be more appropriate?

how do we know that running out of memory was the reason it failed?

Comment 77

[Dave Miller [:justdave]]

Attachment: with bz's comments addressed

This addresses all of bz's comments.  Tested and still works.

Comment 78

[Boris Zbarsky [:bzbarsky]]

We probably don't (though I suspect we can get the error codes out of the Apple
API if necessary).  But you're basically calling an object constructor and I
don't see many failure modes for it except OOM.

In any case, the memory leak is what I really want to see fixed... ;)

Comment 79

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135973 [details] [diff] [review]
with bz's comments addressed

sr=bzbarsky.

Comment 80

[Conrad Carlen (not reading bugmail)]

> Valid HFS paths work, invalid ones throw an error (i.e. volume renamed)
Posix paths still work, too.

Alright. BTW, comments in the patch mention "CFM" paths. Ain't no such thing ;-)
Please change those comments to "HFS" if you see this before checking it in.

Comment 81

[Asa Dotzler [:asa]]

Comment on attachment 135973 [details] [diff] [review]
with bz's comments addressed

a=asa (on behalf of drivers) for checkin to 1.6 beta.

Comment 82

[Dave Miller [:justdave]]

checked in. (with CFM->HFS in the comments)

Checking in nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/mac/nsOSHelperAppService.cpp,v  <-- 
nsOSHelperAppService.cpp
new revision: 1.29; previous revision: 1.28
done

Comment 83

[Frankie]

thanks

Comment 84

[Frankie]

*** Bug 171381 has been marked as a duplicate of this bug. ***