nsLocalFileMac doesn't work for file names > 31 characters

RESOLVED FIXED in mozilla0.9.8

Status

()

P3
normal
RESOLVED FIXED
18 years ago
17 years ago

People

(Reporter: mscott, Assigned: ccarlen)

Tracking

({dataloss})

Trunk
mozilla0.9.8
PowerPC
Mac System 8.5
dataloss
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

18 years ago
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?
(Reporter)

Comment 1

18 years ago
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

Comment 2

18 years ago
Note that nsIFile has a CreateUnique() which should do the right thing.
Component: Browser-General → XPCOM
Hardware: PC → Macintosh
(Reporter)

Comment 3

18 years ago
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
(Assignee)

Comment 4

18 years ago
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?
(Reporter)

Comment 5

18 years ago
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

18 years ago
(Sparing rayw ...)
QA Contact: rayw → jrgm

Updated

18 years ago
Whiteboard: [rtm need info]
mscott, can i help you with this bug?

Comment 8

18 years ago
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.
(Reporter)

Comment 9

18 years ago
Created attachment 17462 [details] [diff] [review]
proposed fix to CreateUnique
(Reporter)

Comment 10

18 years ago
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
(Reporter)

Comment 11

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

Comment 12

18 years ago
The patch looks good. r=sfraser
(Assignee)

Comment 13

18 years ago
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.
(Reporter)

Comment 14

18 years ago
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

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

Comment 16

18 years ago
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

18 years ago
rtm-, we don't appear to be pursuing this for its own sake anymore.
Whiteboard: [rtm need info] → [rtm-]
(Reporter)

Updated

18 years ago
Target Milestone: --- → Future

Updated

18 years ago
Keywords: nsenterprise

Comment 18

18 years ago
adding nsenterprise+
Keywords: nsenterprise → nsenterprise+
Target Milestone: Future → mozilla0.9.4
Keywords: nsrtm
Whiteboard: [rtm-]
(Reporter)

Comment 19

18 years ago
Not sure why enterprise wants this but over to the nslocalFileMac owners. Conrad?
Assignee: mscott → ccarlen
Status: ASSIGNED → NEW
(Assignee)

Comment 20

18 years ago
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

Comment 21

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

Comment 22

17 years ago
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
(Reporter)

Comment 23

17 years ago
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

17 years ago
marking nsenterprise-.
Keywords: nsenterprise+ → nsenterprise-

Comment 25

17 years ago
Pls provide an ETA, if you believe we should fix this on the branch.
Whiteboard: PDT → [PDT] [ETA ?]
(Assignee)

Comment 26

17 years ago
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. 
(Reporter)

Comment 27

17 years ago
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.
(Reporter)

Comment 28

17 years ago
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

17 years ago
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 ?]
(Assignee)

Comment 30

17 years ago
-> 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
(Assignee)

Comment 31

17 years ago
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
(Assignee)

Comment 32

17 years ago
Adding dep.
Depends on: 95481
(Assignee)

Comment 33

17 years ago
Created attachment 64402 [details] [diff] [review]
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.
Attachment #17462 - Attachment is obsolete: true

Comment 34

17 years ago
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+

Comment 35

17 years ago
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
(Assignee)

Comment 36

17 years ago
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

17 years ago
You'd need to use CharacterByteType and/or FillParseTable. See the Script
Manager documentation for details.
(Assignee)

Comment 38

17 years ago
Created attachment 64569 [details] [diff] [review]
truncates without splitting multibyte chars

Thanks - FillParseTable seemed like the way to go.
Attachment #64402 - Attachment is obsolete: true

Comment 39

17 years ago
Hrmm, I think that the ellipsis character will show up as something else in 
Japanese. ftang?
(Assignee)

Comment 40

17 years ago
Created attachment 64576 [details] [diff] [review]
fixed blatant error in last patch

sTable[charPos] should be sTable[aNode[charPos]]
Attachment #64569 - Attachment is obsolete: true
Blocks: 115634
Keywords: nsbeta1
(Assignee)

Comment 41

17 years ago
> 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...
(Assignee)

Comment 42

17 years ago
Created attachment 64938 [details] [diff] [review]
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=
Attachment #64576 - Attachment is obsolete: true

Comment 43

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

r=sdagley
Attachment #64938 - Flags: review+

Comment 44

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

Looks good. sr=sfraser
Attachment #64938 - Flags: superreview+
(Assignee)

Comment 45

17 years ago
Checked in. Thanks.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 46

17 years ago
Adding AOLTW plus.
Whiteboard: [PDT-] [ETA ?] → [PDT-] [ETA ?],AOLTW+

Comment 47

17 years ago
Minusing
Whiteboard: [PDT-] [ETA ?],AOLTW+ → [PDT-] [ETA ?],AOLTW-
You need to log in before you can comment on or make changes to this bug.