Closed Bug 56295 Opened 24 years ago Closed 23 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) (deleted) — 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
Attached patch patch to truncate name to 31 chars (obsolete) (deleted) — Splinter Review
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.
Attached patch truncates without splitting multibyte chars (obsolete) (deleted) — Splinter Review
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?
Attached patch fixed blatant error in last patch (obsolete) (deleted) — Splinter Review
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...
Attached patch Gets ellipsis from itl4 tables (deleted) — Splinter Review
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: 23 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.

Attachment

General

Created:
Updated:
Size: