56295 - nsLocalFileMac doesn't work for file names > 31 characters

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Scott MacGregor]

This bug spun out of Bug #55628.

Go to www.mozilla.org. On the nightly builds part of the page, click on the
Linux link. This trys to download the following file:
"mozilla-i686-pc-linux-gnu.tar"

We fail to download this file.

On the mac, we have a nsLocalFileMac object which points to the destination
directory we want to start writing our temp file to. Let's pretend it's
"HD:DesktopFolder" We append the temp file name to this object.

In my case, I expect to get:
HD:DesktopFolder:mozilla-i686-pc-linux.gnu.tar

Unfortunately appending a file name that's greater than 31 characters on the Mac
causes a corruption to occur so the file object now points to a random location.

For me, I kept getting:
"HD:Seamonkey:view-debug" which is completely bogus. It lost all path information.

As a result the helper app content is never downloaded.

We need to add some code to nsLocalFileMac to properly "fixup" long file names
with the following constraints:
1) preserve the file extension
2) ensure uniqueness of the file so we don't stomp on an exisiting file when we
generate this new file name.
3) any other constraints?

Comment 1

[Scott MacGregor]

changing to browser general I didn't mean to select xp-com as the component.

adding rtm and data loss keywords as this problem prevents us from downloading
files from the web > 31 characters.

I can probably come up with a hack for the file download case which might work
for RTM but we might want the Mac owner for nsLocalFileMac to investigate a long
term solution there.

Comment 2

[Simon Fraser [no longer active]]

Note that nsIFile has a CreateUnique() which should do the right thing.

Comment 3

[Scott MacGregor]

Changing my mind a little bit, I'd be happy just having a generic routine anyone
can call (off nsILocalFile???) that would cannocalize a file name making sure it
met the Mac requirement of file length.

I'm sure I'm not the only user of nsLocalFileMac that has file names that could
be >= 31 characters.

Comment 4

[Conrad Carlen (not reading bugmail)]

Isn't there some existing convention that we could use in nsLocalFileMac? Isn't
there something in NSPR made to deal with java class names on the Mac? If so, it
should be brought over to nsLocalFileMac. Depends on what we want to happen when
a > 31 char name is used and then somebody calls nsIFile::GetPath(). I suppose
we return the shortened path?

Comment 5

[Scott MacGregor]

Apparently this used to work with the old nsFileSpec code:
1) Append the word "dummy" to your file spec.
2) Then call MakeUnique passing in the REAL file name you want as the suggested
file name. MakeUnique would properly truncate your suggested name and replace
"dummy" with a truncated suggested name.

I'm looking at nsIFile::CreateUnique right now and it currently completely
ignores the passed in suggested file name. But it did have code to truncate a
suggested name if it was too long.

I'm going to try to modify nsLocalFile::CreateUnique to behave like the old
nsFileSpec::MakeUnique used to.

This could be a work around we could use for rtm....

Apparently we've had this problem in mail and other places before in code that
still uses nsFileSpec. That code uses this dummy technique I described above.

Comment 6

[John Morrison]

(Sparing rayw ...)

Comment 7

[Jean-Francois Ducarroz]

mscott, can i help you with this bug?

Comment 8

[selmer (gone)]

JF, we discussed making this fix be Mac-specific since other platforms are not
affected by it.  (Do we have any 8.3 problems to worry about?  Phil filed a bug
that may boil down to this problem.)  If you can find a small, safe patch it
will help things along.

Comment 9

[Scott MacGregor]

Attachment: proposed fix to CreateUnique

Comment 10

[Scott MacGregor]

Okay here's a fix to CreateUnique that makes things better. I modified create
unique to do a couple of things:

1) If a suggested name is passed in, then we should honor it!! Before, this
parameter was completely ignored. So the only way to pass in your suggested name
was to Append the name and then call create Unique. This is what I was doing in
the exthandler. And Append barfs big time with long file names on the mac.

2) For the Mac only, truncate the suggested file name or the leaf name if the
non extension part is over 27 characters. This code was already there before.

Now in the uriloader I can call CreateUnique directly with a long suggested file
name and on the mac, we'll properly truncate the suggested name into a valid
file name.

Comment 11

[Scott MacGregor]

Simon and Conrad, can I get code reviews from you guys for this patch? 

Comment 12

[Simon Fraser [no longer active]]

The patch looks good. r=sfraser

Comment 13

[Conrad Carlen (not reading bugmail)]

I'm not sure about this. If suggestedName != nsnull, it always enters the loop.
So, if you wanted a file named "foo", passed that as suggestedName, and even if
"foo" didn't exist in that dir, you'd get foo-1.

Comment 14

[Scott MacGregor]

oh good catch your right, that's not the behavior I intended. I'll change the
loop so we test for existence with the suggested name before attempting to add
uniqueness code. 

Comment 15

[Jason Eager]

No comments for a week now. Is this dead for rtm?

Comment 16

[selmer (gone)]

Seems like a new patch is required.  Is this happening soon?  I'd like to see 
this get into rtm+ state ASAP or moved to rtm- if we've given up.

Comment 17

[selmer (gone)]

rtm-, we don't appear to be pursuing this for its own sake anymore.

Comment 18

[scottputterman]

adding nsenterprise+

Comment 19

[Scott MacGregor]

Not sure why enterprise wants this but over to the nslocalFileMac owners. Conrad?

Comment 20

[Conrad Carlen (not reading bugmail)]

Accepting. I'd like to go with the original idea:

> 1) preserve the file extension
> 2) ensure uniqueness of the file so we don't stomp on an exisiting file when we

making sure it's applied to anything which can affect the leafname of the file.

Comment 21

[Jaime Rodriguez, Jr.]

This hasn't been touched in a month. Is this still an enterprise show stopper?

Comment 22

[Jaime Rodriguez, Jr.]

Is there a current workaround that we can apply to the branch, like we did for
previous release.

Comment 23

[Scott MacGregor]

I really don't believe this is an enterprise stopper. I'd like to see some
justificaiton as to why they think this is nsEnterprise+. In the mean time this
should be PDT-.

Comment 24

[grega]

marking nsenterprise-.

Comment 25

[Jaime Rodriguez, Jr.]

Pls provide an ETA, if you believe we should fix this on the branch.

Comment 26

[Conrad Carlen (not reading bugmail)]

mscott, is this problem even possible when downloading files anymore? The temp
file has a salted name < 31 chars and then, when it comes to picking the final
location, the file picker UI (on the OS level) will ensure that the file name is
not too long. Not saying this isn't a problem but just one that's not likely
(possible) to be encountered and therefore not PDT. 

Comment 27

[Scott MacGregor]

conrad: your exactly right which is why I recommended a minus for this bug. I
think grega made it a minus and it's no longer nominated for the branch.

Comment 28

[Scott MacGregor]

Just read Jaime's comment. Jaime why do you think you are interested in this
bug? It's not even nominated for the branch. I'm not sure where the [PDT] came
from. Maybe that's how it accidentally got on their radar.

Comment 29

[Jaime Rodriguez, Jr.]

Looks like the PDT was added when this was a enterprise+, to keep it on the
radar. Since it is now a enterprise+, and not a stopper = PDT-

Comment 30

[Conrad Carlen (not reading bugmail)]

-> 0.9.7
Pushing off since this it is unlikely, if not impossible, to encounter this problem.

Comment 31

[Conrad Carlen (not reading bugmail)]

While not a dup of bug 95481, will be fixed (on OS X anyway) by that.
-> 0.9.8

Comment 32

[Conrad Carlen (not reading bugmail)]

Adding dep.

Comment 33

[Conrad Carlen (not reading bugmail)]

Attachment: patch to truncate name to 31 chars

The patch limits the length to 31 chars, truncating it from the middle if need
be. It's done on Append() and SetLeafName(). It's not done on InitWithPath()
because, since that's a native path, it's required to be valid. BTW, I'll
replace the ellipsis as character constant with kEllipsisChar = 0X?? as soon as
I can find its value. Works when used with Steve's patch for bug 60203 and
downloading Linux nightly from mozilla.org.

Comment 34

[Steve Dagley]

Comment on attachment 64402 [details] [diff] [review]
patch to truncate name to 31 chars

r=sdagley (yes, please change the hardcoded ... to a const)

Comment 35

[Simon Fraser [no longer active]]

Hrmm. I think truncating a char* representation of the filename like that may do 
bad things on systems with a non-Roman system script (e.g. Japanese).

Comment 36

[Conrad Carlen (not reading bugmail)]

Looking at the Script Mgr and others, I can't yet find a way to determine
character boundaries in a sequence of multibyte chars. Anybody know? Also, is
the HFS name length limit 31 bytes or 31 characters? Anyway, if we don't
truncate a file name > 31 bytes on a MacRoman system, that file operation is
hosed anyway.

Comment 37

[Simon Fraser [no longer active]]

You'd need to use CharacterByteType and/or FillParseTable. See the Script
Manager documentation for details.

Comment 38

[Conrad Carlen (not reading bugmail)]

Attachment: truncates without splitting multibyte chars

Thanks - FillParseTable seemed like the way to go.

Comment 39

[Simon Fraser [no longer active]]

Hrmm, I think that the ellipsis character will show up as something else in 
Japanese. ftang?

Comment 40

[Conrad Carlen (not reading bugmail)]

Attachment: fixed blatant error in last patch

sTable[charPos] should be sTable[aNode[charPos]]

Comment 41

[Conrad Carlen (not reading bugmail)]

> I think that the ellipsis character will show up as something else in
> Japanese.

It will. Although, it is possible to look up the string which is smTokenEllipsis
in the intl4 table. Working on that...

Comment 42

[Conrad Carlen (not reading bugmail)]

Attachment: Gets ellipsis from itl4 tables

This should be it. Gets the tokenEllipsis from the itl4 tables, resorts to
ASCII "..." if that cannot be found. Tested, need r=/sr=

Comment 43

[Steve Dagley]

Comment on attachment 64938 [details] [diff] [review]
Gets ellipsis from itl4 tables

r=sdagley

Comment 44

[Simon Fraser [no longer active]]

Comment on attachment 64938 [details] [diff] [review]
Gets ellipsis from itl4 tables

Looks good. sr=sfraser

Comment 45

[Conrad Carlen (not reading bugmail)]

Checked in. Thanks.

Comment 46

[Michael Buckland]

Adding AOLTW plus.

Comment 47

[Michael Buckland]

Minusing