134890 - nsIWebBrowserPerstist should not truncate filenames to 20 characters or otherwise mangle the filename

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Charles Manske]

nsWebBrowserPersist::MakeFilenameFromURI contains a kMaxFileNameLength value
that is set to 20.
That is very short! We should at least not do that for Windows and Linux.

Comment 1

[Adam Lock]

Ideal solution would be a nspr version of MAX_PATH. I don't want to pepper the
code with platform specific #ifdef's if possible.

Comment 2

[Jaime Rodriguez, Jr.]

Removing adt1.0.0 because there is no patch, nor reviews. Pls work with your
manager to get this nsbeta1+. From the information in the bug, we can not tell
why this is an ADT2. What effect does this have on the user, and how many users
are effected.

Comment 3

[Charles Manske]

I discovered this bug during publishing testing. If any image filenames are > 20,
the names used to upload the files are wrong, you get a rash of errors, all the
urls are changed in the page to the wrong, truncated urls, which "busts" the 
images in the page. And the user is totally confused.

Comment 4

[Bradley Baetz (:bbaetz)]

For publish, doesn't this depend on the server OS, not the client? I can publish
to an 8.3 msdos server, even though mozilla doesn't run on that.

Comment 5

[Charles Manske]

Another flaw in the MakeFilenameFromURI() method is that it truncates filenames
that contain an internal "." in them. As far as I know, "foo.bar.jpg" is a valid 
JPEG filename, but this is getting converted to "foo.jpg" by this code.
This has the same effect during publishing: the destination filename is converted
into a non-existing filename and completely confuses the upload of an image file.

Comment 6

[Adam Lock]

MakeFilenameFromURI may not have to do anything at all for upload - just use
whatever URL the user supplied.

Kathy can you comment?

This function is primarily for download to local disk to make sane filenames
from URLs containing illegal characters or extreme length, e.g.

http://foo/path/some-image.cgi?blah=%20lalal&blah2=x
http://foo/path/some\image.jpg
http://foo/path/90324898475984379584379857349579345709238409238490238490238409238490823409823904809234902839490823094823048902384029384903028409238409234092380498234.gif
etc.

Comment 7

[Adam Lock]

Attachment: Simple patch

This patch increases the max file name size. It goes up to 22 on the mac and 55
for other platforms. The max filename length on the Mac is 31, but 22 allows
space for appending the mime type extension.

I don't think this is an ideal solution but it should lessen the short term
issue on Unix/Win32.

A more involved fix would involve adding a datapath arg to MakeFilenameFromURI
and having platform specific code to query the maximum filename size for the
target volume.
Kathy, what's the situation with this increase and uploading? Are just assuming
that the destination server should be able to cope with whatever length
filename we feed to it?

Comment 8

[Adam Lock]

Attachment: Work in progress

This is a better solution but still work in progress and more complex.

This code waits until after the extension has been appended before checking if
the filename exceeds the max length and for duplicate filenames. If necessary
it will truncate the file name at that point, allowing the file name length to
be 31 on Mac and possibly up to 250 on other systems. It will also try
different filenames if one with the same name already exists.

The code has some XXXX remarks because the duplicate code is broken and I've
artificially restricted the max filename length to test the code out.

Comment 9

[Adam Lock]

Attachment: Patch

This patch is ready for review. Kathy, can you take a look to see what you
think?

I've moved the length checking until after the extension has been appended. The
business of making a unique filename happens in CalculateUniqueFilename(). This
method truncates the filename if necessary to fit in the max length (31 for
mac, 64 otherwise) and then ensures it's not a duplicate. Finally, the new name
(if it needed changing at all) is resynced with the uri/localfile it came from.
The old dupe checking code has been deleted.

Comment 10

[Adam Lock]

Attachment: Patch

This is a slightly updated patch. It moves the needToChop calculation out one
scope to save two identical comparisons but is otherwise the same. The patch
still calculates the base & extension for itself rather than using methods on
nsIURL. This is to save mess later on where we'd have to write

if (!ext.IsEmpty()) {
  newFileName.Append(".");
  newFileName.Append(ext);
}

three seperate times for the sake of saving a few lines in the current
implementation.

Comment 11

[Charles Manske]

Adam: Does this patch also address problem when filename contains > 1 "." 
character?

Comment 12

[Charles Manske]

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

Comment 13

[Adam Lock]

If a file contains more than one dot then the behaviour is the same as before,
e.g. foo.xxx.gif is saved as foo.gif.

There are some issues around changing that behaviour so I'd rather keep that as
a seperate bug so they don't get mixed up with this patch.

Comment 14

[Charles Manske]

ok, I reopened bug 138832

Comment 15

[Adam Lock]

Attachment: Patch

Remove unecessary newfilename variable but otherwise the same using filename
instead

Comment 16

[Kathleen :Brade]

Comment on attachment 81904 [details] [diff] [review]
Patch

r=brade

Comment 17

[kinmoz]

Comment on attachment 81904 [details] [diff] [review]
Patch

sr=[e-mail]

Comment 18

[Adam Lock]

I've run into a problem with the patch on OS X which is causing truncated
filenames images to be saved in the wrong dir.

I'm investigating it still which is why this hasn't gone in yet.

Comment 19

[Adam Lock]

Attachment: Patch

Updated patch ensures directory folders are checked for duplicates too. I still
have to figure out what the problem is on the Mac or even if it is my patch
causing it.

Comment 20

[Adam Lock]

The Mac is truncating filenames in nsLocalFileMac and putting ellipsis
characters in the middle. This horks all my careful filename truncation code so
I'm going to have to think about this a bit more.

I will probably have to add a filename member to the OutputData structure to
ensure I can get the long filename through to the uniqueness / duplicate
checking code without the file object mangling it first.

Comment 21

[Adam Lock]

Attachment: Work in progress

I think this patch will fix the Mac problems with long names, but my Mac is
playing up so I have yet to verify. Patch is the same as the last one except
that MakeFilenameFromURI() restricts the length to that specified by
kDefaultMaxFilenameLength. This should prevent the Mac nsILocalFile impl from
truncating the name with ellipsis because its too long.

Comment 22

[Adam Lock]

Attachment: Patch

This patch fixes the problems on the Mac problem and is ready for review.

I've put in code to ensure that the filename length never exceeds
kDefaultMaxFilenameLength at any point. The Mac impl of nsLocalfile is a lot
more picky about the filenames you pass into it and will slice and dice any
which exceed 31. Two places in the code could trigger this behaviour, first
when a filename was made from a >31 chars URI and second when the extension was
appended making the name >31 chars. I put checks into those places to stop this
happening.

Comment 23

[Adam Lock]

If this is a beta candidate, can I have some additional testing to make sure it
doesn't break anything. Particularly for composer & uploading. Thanks

Comment 24

[Charles Manske]

This problem is disturbing! So the Mac limitation means that a file on a 
Unix server can have a filename with > 31 characters and can be displayed 
perfectly fine in a Mac browser, but when they download this file to a Mac,
we must truncate to 31 characters? Or putting it another way, any file created
on a Mac is subject to this limitation, independent of whether or not it is 
being published.
Do we avoid this problem if we edit a remote file directly? We shouldn't have 
to use nsILocalFile in that case, just nsIURI, right?

Comment 25

[Adam Lock]

The problem with uploading to remote servers is there is no obvious way to
*know* what file length the server accepts - Unix mightn't care much (as long as
its less than 256 chars), but the server might be OS/2, Mac, Novell, Vax etc.
where there is a limit of some kind.

Filename length (and legal characters) can also vary, both remotely and locally
depending on the volume you save to. Since we have no way to query that easily
or in an cross platform manner, I have picked some "reasonable" limits, both for
picking filenames and their maximum length.

I hope the length picking can be made more sophisticated but I believe this
improvement is "good enough" for now, especially considering the issues to be
overcome to make it any better.

Perhaps later iterations could be smarter about the max length depending on
whether it's for upload or local consumption and also the volume its being saved to.

Comment 26

[Charles Manske]

Thanks, Adam, that's very useful. It sounds like we should impose limits and/or 
give warnings about filename lengths in the Composer UI so users don't encounter 
the side effects we've seen in publishing.

Comment 27

[Adam Lock]

Can I get a review on this please?

Comment 28

[Kathleen :Brade]

Comment on attachment 85616 [details] [diff] [review]
Patch

r=brade

Comment 29

[kinmoz]

Comment on attachment 85616 [details] [diff] [review]
Patch

sr=[e-mail]

Just fix the indention.

Comment 30

[Adam Lock]

Fix is checked into trunk

Comment 31

[Jaime Rodriguez, Jr.]

benc - can you pls verify the fix on the branch? thanks!

Comment 32

[Blake Ross]

Presumably jaime means the trunk...

Comment 33

[Paul Wyskoczka]

Addting adt1.0.1-.  Let's try to get it into the next release.