Closed Bug 56295 Opened 21 years ago Closed 19 years ago

nsLocalFileMac doesn't work for file names > 31 characters

Categories

(Core :: XPCOM, defect, P3)

PowerPC
Mac System 8.5
defect

Tracking

()

RESOLVED FIXED
mozilla0.9.8

People

(Reporter: mscott, Assigned: ccarlen)

References

Details

(Keywords: dataloss, Whiteboard: [PDT-] [ETA ?],AOLTW-)

Attachments

(1 file, 4 obsolete files)

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?
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.
Component: XPCOM → Browser-General
Keywords: dataloss, rtm
Note that nsIFile has a CreateUnique() which should do the right thing.
Component: Browser-General → XPCOM
Hardware: PC → Macintosh
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.
Hardware: Macintosh → PC
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?
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.
(Sparing rayw ...)
QA Contact: rayw → jrgm
Whiteboard: [rtm need info]
mscott, can i help you with this bug?
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.
Attached patch proposed fix to CreateUnique (obsolete) — Splinter Review
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.
Status: NEW → ASSIGNED
Simon and Conrad, can I get code reviews from you guys for this patch? 
The patch looks good. r=sfraser
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.
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. 
No comments for a week now. Is this dead for rtm?
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.
rtm-, we don't appear to be pursuing this for its own sake anymore.
Whiteboard: [rtm need info] → [rtm-]
Target Milestone: --- → Future
Keywords: nsenterprise
adding nsenterprise+
Target Milestone: Future → mozilla0.9.4
Keywords: nsrtm
Whiteboard: [rtm-]
Not sure why enterprise wants this but over to the nslocalFileMac owners. Conrad?
Assignee: mscott → ccarlen
Status: ASSIGNED → NEW
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.
Status: NEW → ASSIGNED
This hasn't been touched in a month. Is this still an enterprise show stopper?
Is there a current workaround that we can apply to the branch, like we did for
previous release.
Whiteboard: PDT
Target Milestone: mozilla0.9.4 → mozilla0.9.5
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-.
marking nsenterprise-.
Pls provide an ETA, if you believe we should fix this on the branch.
Whiteboard: PDT → [PDT] [ETA ?]
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. 
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.
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.
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-
Whiteboard: [PDT] [ETA ?] → [PDT-] [ETA ?]
-> 0.9.7
Pushing off since this it is unlikely, if not impossible, to encounter this problem.
Target Milestone: mozilla0.9.5 → mozilla0.9.7
While not a dup of bug 95481, will be fixed (on OS X anyway) by that.
-> 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Adding dep.
Depends on: 95481
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.
Attachment #17462 - Attachment is obsolete: true
Comment on attachment 64402 [details] [diff] [review]
patch to truncate name to 31 chars

r=sdagley (yes, please change the hardcoded ... to a const)
Attachment #64402 - Flags: review+
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).
OS: Windows NT → Mac System 8.5
Hardware: PC → Macintosh
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.
You'd need to use CharacterByteType and/or FillParseTable. See the Script
Manager documentation for details.
Thanks - FillParseTable seemed like the way to go.
Attachment #64402 - Attachment is obsolete: true
Hrmm, I think that the ellipsis character will show up as something else in 
Japanese. ftang?
sTable[charPos] should be sTable[aNode[charPos]]
Attachment #64569 - Attachment is obsolete: true
Blocks: 115634
Keywords: nsbeta1
> 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...
This should be it. Gets the tokenEllipsis from the itl4 tables, resorts to
ASCII "..." if that cannot be found. Tested, need r=/sr=
Attachment #64576 - Attachment is obsolete: true
Comment on attachment 64938 [details] [diff] [review]
Gets ellipsis from itl4 tables

r=sdagley
Attachment #64938 - Flags: review+
Comment on attachment 64938 [details] [diff] [review]
Gets ellipsis from itl4 tables

Looks good. sr=sfraser
Attachment #64938 - Flags: superreview+
Checked in. Thanks.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Adding AOLTW plus.
Whiteboard: [PDT-] [ETA ?] → [PDT-] [ETA ?],AOLTW+
Minusing
Whiteboard: [PDT-] [ETA ?],AOLTW+ → [PDT-] [ETA ?],AOLTW-
You need to log in before you can comment on or make changes to this bug.