139360 - Save to Disk with file name >31 characters silently fails

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Steve Dagley]

Now that the Nav Services 3.0 APis are used for the Mac FilePicker code it's
possible to specify a file name >31 characters whereas the older Nav Services
API enforced the pre-HFS+ file system limit of 31 characters.  Since our file
handling code isn't yet using the HFS+ APIs we need to ensure that, when
presented with a file name >31 characters, the Save to Disk code path properly
truncates the name to a legal one as the code to pass the file off to a helper
app does.

Logged as a File Handling bug since it's not specific to the Download Manager

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Steve Dagley]

Here's the site of the problem:

<http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsFilePicker.cpp#601>

Since the name field of an FSSpec is a Str63 we're initializing the
nsLocalFileMac's FSSpec with the first 63 characters of reply.saveFileName. 
Unfortunately, as noted before, our Mac file system code is currently limited to
31 character names so this is a Bad Thing™.  Changing nsLocalFileMac's
InitWithFSSpec() method to simply truncate the name isn't acceptable as we want
the smart middle truncation of reply.saveFileName provided by the
TruncNodeName() utility func.  Patch to do this coming up.

Comment 3

[Steve Dagley]

Attachment: Limit output of PutLocalFile to 31 character file names

Comment 4

[Steve Dagley]

Attachment: Same as before but fixes a bug in TruncNodeName

This is basically the same patch as before but ccarlen and I discovered there
was a bug in TruncNodeName() which caused it to truncate to 30, not 31,
characters.

Comment 5

[Steve Dagley]

r/sr please

Comment 6

[Conrad Carlen (not reading bugmail)]

Comment on attachment 80789 [details] [diff] [review]
Same as before but fixes a bug in TruncNodeName

> %{C++
>-extern "C" NS_EXPORT nsresult
>-NS_NewLocalFileWithFSSpec(FSSpec* inSpec, PRBool followSymlinks, nsILocalFileMac* *result);
>+extern "C"
>+{
>+NS_EXPORT const char* TruncNodeName(const char *aNode, char *outBuf);

You should put an NS_ prefix on this routine. Also, since it's out in public
now, a description of what it does, and that it's a temporary measure until HFS
is gone.

>+
>+NS_EXPORT nsresult NS_NewLocalFileWithFSSpec(FSSpec* inSpec, PRBool followSymlinks, nsILocalFileMac* *result);
>+}
> %}

Comment 7

[Conrad Carlen (not reading bugmail)]

> Also, since it's out in public now, a description of what it does

Particularly that the ptr it returns is either one of the input ptrs and should
not be freed.

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 80789 [details] [diff] [review]
Same as before but fixes a bug in TruncNodeName

+	     if (truncNameLen)
+	     {
+	       BlockMoveData(truncName, &theFSSpec.name[1], truncNameLen);
+	     }

lose the braces. they're inconsistant with the rest of the file to have them
for only one line.

+NS_EXPORT const char* TruncNodeName(const char *aNode, char *outBuf);

if we're exporting it, it should be prefixed with NS_ to avoid naming conflicts
with embedding

r=pink

Comment 9

[Steve Dagley]

Attachment: addressing comments on previous patch

Updated patch to address comments from ccarlen and pinkerton.

scc, sr= please.

Comment 10

[Scott Collins]

Comment on attachment 80821 [details] [diff] [review]
addressing comments on previous patch

sr=scc

Comment 11

[Steve Dagley]

Fix checked in to the trunk.  Now to see about geting it nominated for branch

Comment 12

[Steve Dagley]

FYI, thanks to the 'security' fix in #116938 for what I'd consider a Win32
specific problem the suggested file name in the save file dialog will now get an
extra extension added driving a lot more file names past the Mac 31 character
limit.  If we don't get this one fixed on the branch soon we're gonna get a lot
more complaints about this bug.

Comment 13

[Josh Whitver]

By saying the fix is in the trunk, does that mean it'll show up in the Release,
but not in the nightly builds?  I'm using the 2002/04/30 nightly, and the bug is
still there.  (Or maybe I'm just confused as to the trunk/branch differentiation.)

Comment 14

[Jaime Rodriguez, Jr.]

ben - Can you verify this fix on the trunk, and check for any possible regressions?

Comment 15

[benc]

CONFIRMED: Mac OS X RC1

STEPS:
Save any file, use 35 character filename:
12345678901234567890123456789012345

EXPECTED: file should appear in destination selected in Save dialog, or error.
ACTUAL: temp, salted file appears on desktop, but disappears when final copy
shold occur.

-> file manager, per original comments + all similar bugs are over there.

jamie: You really hould make sure bugs "clean" (have steps, correct component)
before accpeting bugs as adt. For example, I test File, but from a QA
perspective, this should be two bugs, a "file stuff should do long files
correctly" and a "save long files breaks" (which would be a depends on the first
bug).

We have very little white-box testing for networking right now, so you want to
use your higher-level QA people to ferret out confirmations and verifications.

Comment 16

[Paul Wyskoczka]

ben - Can you verify this fix on the trunk, and check for any possible regressions?
You verified on RC1 but we need date on whether it is  fixed on the trunk.

Comment 17

[Brad Tiffany]

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

Comment 18

[bruno lapeyre]

Using build 2002050308 under Mac OS X 10.1.4 I tried to download that file again:
http://homepage.mac.com/bostmass/.cv/bostmass/Public/ChangeStartupScript.sit-binhex.hqx
No problem with the new trunk: the file was normally downloaded and the name of
the downloaded file became:
ChangeStartupSc….sit-binhex.hqx

Comment 19

[sairuh (rarely reading bugmail)]

tested using 2002.05.02.03-trunk (commercial) bits on mac 10.1.4 for a couple of
cases --let me know if either of these results are not expected!

a. clicked on
http://ftp.mozilla.org/pub/mozilla/nightly/latest/MacMozillaInstaller.sea.bin,
and was able to save the file with the suggested file name of
"MacMozillaInstaller.sea.bin.binary".

b. selected "save link target" fron context menu for a file, renamed to having
over 40 characters in the filename, and got a truncated name that's 31-char in
length, eg "ifoo123567890...1234567890.html".

Comment 20

[Josh Whitver]

That's great!  

It'd be nicer, naturally, if it would simply save the file with the original,
longer name, since the filesystem can handle it.

The fact that it doesn't throw the file away anymore is nice, but I still
consider it a bug that the name gets truncated needlessly. (But that's just my
opinion, I realize. :) )

Comment 21

[bruno lapeyre]

I do have the same results using 2002050308.

a. clicked on
http://ftp.mozilla.org/pub/mozilla/nightly/latest/MacMozillaInstaller.sea.bin,
and was able to save the file with the suggested file name of
"MacMozillaInstaller.sea.bin" 
(I guess you made a mistake adding ".binary" to the end of the file name)

b. Using more than 31 characters is giving a truncated file name that's 31-char
long.

Comment 22

[Steve Dagley]

Fixing typo somebody introduced in the summary.  As for the comments we should
support file names >31 chacters - there's already a bug filed on that.  Finding
the # for it is left as an exercise for the reader :-)

Comment 23

[bruno lapeyre]

Yes, if some users could admit to download some files with a truncated name, all
users won't and the problem is deeper because what is a partial solution for
file downloading is just a partial solution for *file* downloading: web pages
calling for elements having names of more than 31 characters won't be actually
displayed correctly in Mozilla because the more than 31 characters elements will
be missing. Create an html file just to present a picture with a name of more
than 31 characters, for example, and you will see that the picture will never
appear in the browser...

Comment 24

[scottputterman]

adding adt1.0.0+.  Please get drivers approval and then checkin to the 1.0 branch.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[scottputterman]

changing to adt1.0.1+ for checkin to the 1.0 branch for the Mozilla1.0.1
milestone.  Please get drivers approval before checking in.

Comment 27

[Judson Valeski]

please checkin to the 1.0.1 branch ASAP. once there, remove the mozilla1.0.1+
keyword and add the fixed1.0.1 keyword.

Comment 28

[Steve Dagley]

Fix checked in to MOZILLA_1_0_BRANCH

Comment 29

[sairuh (rarely reading bugmail)]

tested using 2002.06.10.05 comm branch bits on mac 10.1.5.

the test link for (a) in comment 19 is currently dead. so, what i see now is
like test (b): the filename is now truncated. also, i noticed that for long
filenames, the resulting filepicker lops off a good chunk of the suggested
filename (no dataloss, though) --filed that separately as bug 150712.

anyhow, since file saving no longer fails, i'm verifying this particular bug. :)

Comment 30

[Frankie]

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