393488 - Special characters in filename block the download function

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P2)

Description

[Gwen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6

The file download function did not work no more since I try to download a file whose filename is "A9�(8)2.pdf". The error console shows : 
Error: not well-formed Source File: file:///C:/Documents%20and%20Settings/jopo/Application%20Data/Mozilla/Firefox/Profiles/pttxa21n.default/downloads.rdf
Line: 54, Column: 67
Source Code:
    <RDF:Description RDF:about="C:\DOCUME~1\jopo\LOCALS~1\Temp\A9�(8)2.pdf"
------------------------------------------------------------------^

Reproducible: Couldn't Reproduce

Steps to Reproduce:
1.
2.
3.



Nothing warns there is a problem or the downloading of new file do not start.

Comment 1

[Shawn Wilsher :sdwilsh]

Stephen - can you check this with trunk?  I'm betting this is fixed on trunk by using sqlite

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

Gwen, do you have a public URL to a download with that name that you could put into either the URL field of this bug, or paste inline here?  Thanks!

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

(I tried both searching Google and locally renaming other PDF files to "A9�(8)2.pdf", with no success; XP SP2 and Mac OS X 10.4 wouldn't let me name them as such.

Comment 4

[Gwen]

It concerns a filename using Korean characters but I did not manage to reproduce the bug. The same kind of problem appear with other links (found on the same server where i got the bug i reported). Here is an example :

The direct link is http://www.mic.go.kr/secureDN.tdf?seq=940&idx=1&board_id=P_03_01_05

(obtained on the board  http://www.mic.go.kr/user.tdf?a=user.board.BoardApp&c=2002&board_id=P_03_01_05&seq=940&mc=P_03_01_05 , click on the PDF link)

The filename should be "IT839전략(국문)2.pdf"
But it is recognized as "IT839?(m8)2.pdf" and cannot be download by Firefox. 
No error message appears but I checked the error console and it shows the following message : 
"Error: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsILocalFile.isDirectory]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Firefox%202%20Beta%202/components/nsHelperAppDlg.js :: anonymous :: line 285"  data: no]
Source File: file:///C:/Program%20Files/Mozilla%20Firefox%202%20Beta%202/components/nsHelperAppDlg.js
Line: 285"


Other Korean files can be downloaded, you can check with this example :

Direct link to the PDF file "인도시장진출전략-LG(96년).pdf"  : http://pds16.cafe.daum.net/download.php?grpid=xWM5&fldid=4365&dataid=1&fileid=1&regdt=20050428231226&disk=19&grpcode=lgindo&dncnt=Y&.pdf&filename=%C0%CE%B5%B5%BD%C3%C0%E5%C1%F8%C3%E2%C0%FC%B7%AB-LG%2896%B3%E2%29.pdf&nenc=uJYY2uU6SKFqrB2TUW-itg00&fenc=gEX31rE7tkk0

Korea Web page containing the link the PDF file "인도시장진출전략-LG(96년).pdf" : http://cafe337.daum.net/_c21_/pds_v3check?grpid=xWM5&fldid=4365&dataid=1&fileid=1&regdt=20050428231226&nenc=uJYY2uU6SKFqrB2TUW-itg00&fenc=gEX31rE7tkk0

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screenshot; 'works' in Fedora Core 7

Just a datapoint that this works in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007082604 Minefield/3.0a8pre.

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

Confirming the bug on Windows Vista with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007082618 Minefield/3.0a8pre.

Mac saves it as "IT839"

So, broken on Windows, but works on Mac and Linux.

Comment 7

[Stephen Donner [:stephend] Not actively reading bugmail]

http://litmus.mozilla.org/show_test.cgi?id=4607

in-litmus+

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

In the course of figuring out what was going on here, I spun off a patch to fix some minor API problems internal to nsLocalFileWin and put that in bug 216821.  

It looks like ::CreateFileW is aborting with ERROR_INVALID_NAME.  The path itself looks like this

"C:\Documents and Settings\Administrator\Desktop\IT839µ(m8)2.pdf"

All the parent directories are valid and extant.  jshin, any theories on why Windows might be getting upset?

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: handle filesystem incompatible chars in filename, v1

After reading through a bunch of stuff on MS dev site, it seems pretty clear that there are going to be cases where we'll have characters in our filename that the underlying filesystem is going to barf on, and no way to tell that a priori.  So we are going to need to handle that case better in general, rather than just fixing the specific problem here.  

I've pulled the patch mentioned previously back into this bug, added a test for it, added a patch for toolkit, and a test-in-progress for toolkit as well.  There are multiple problems with the toolkit test, including bug 299372.  So I'm going to shelve this for now and come back to it after bug 299372.  Work-in-progress patch attached.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Should see if this could be a regression from bug 278161 (as bug 409796 appears to be).

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

qawanted added, as per comment 11.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Ignore comments 11 & 12; I was confused.

Comment 14

[Shawn Wilsher :sdwilsh]

talking to dmose - don't think there is a dependency on the previously mentioned bug (should only be the case for save page as downloads).  However, he's kinda sorta busy with some other important work.  Jimm - do you have any spare cycles to look at this by chance?

Comment 15

[Jim Mathies [:jimm]]

Can't reproduce this. multiple saves preserve the filename and append (x) on the end. This is on Vista Home Premium.

Comment 16

[Jim Mathies [:jimm]]

(That was using trunk from today)

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Maybe this is XP only?  I was definitely able to reproduce it on XP.

Comment 18

[Jim Mathies [:jimm]]

Maybe Stephen can confirm. I don't have access to my xp images this week.

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

I couldn't reproduce with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012804 Minefield/3.0b3pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008012904 Minefield/3.0b3pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012904 Minefield/3.0b3pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008012904 Minefield/3.0b3pre

I have screenshots, which I can attach if needed, but downloads on all of these OSs worked, and I could also successfully open the file.

Comment 20

[Shawn Wilsher :sdwilsh]

I'm wondering if maybe it matters if you have a non-en windows build?

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Screenshot with German WinXP

It happens for me with a German XP and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020104 Minefield/3.0b3pre ID:2008020104

I even don't see a failure. But Minefield wants to save the file with a strange character encoding. I think that I don't have installed the unicode fonts. Perhaps the following error appears in the Error Console?

Error: uncaught exception: unknown (can't convert to string)

Comment 22

[Simon Montagu :smontagu]

The real filename from comment 4 is IT839\uC804\uB7B5(\uAD6D\uBB38)2.pdf (using javascript escapes for the non-ASCII characters). Comment 8 and the screenshot in attachment 301009 [details] shows that the name is going through a lossy 16-bit to 8-bit conversion. Unfortunately there are still a number of places where we do that with filenames. Bug 411511 will fix some of them but I'm not sure whether what will help this bug or not.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

With a few exceptions, I'm mostly focused on MailCo-related hacking now.  Reassigning a bunch of bugs to default component owners.  I'm happy to help with brainstorming/advice as needed, however.  

Search for the string MAILMONKEY to delete any bugmail generated by this change.

Comment 24

[Simon Montagu :smontagu]

It turns out that this isn't us doing a lossy conversion: the filename is already incorrect in the Content-Disposition header.

I think we can fix this just by extending the FILE_ILLEGAL_CHARACTERS macro to include all characters < 0x20.

Comment 25

[Simon Montagu :smontagu]

Attachment: Patch

dmose, what were the problems with your toolkit test apart from bug 299372?

Comment 26

[Stephen Donner [:stephend] Not actively reading bugmail]

Might be what Marcia sees from time to time on the debug build in the lab:

marcia_debug: WARNING: Illegal character in window name Download:Manager: file /Users/debug/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp, line 1372; but that assertion's filename doesn't match the patch in comment 25

Comment 27

[Simon Montagu :smontagu]

I may be missing something, but that warning looks kind of bogus to me. Steps to reproduce and/or a stack would be helpful.

Comment 28

[Shawn Wilsher :sdwilsh]

That warning has nothing to do with it.  It'll happen anytime the download manager is opened (addons manager too!)

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

For an explanation just take a look at bug 258480. Could be the same reason.

Comment 30

[Simon Montagu :smontagu]

Right, so either the name "Download:Manager" should be changed, or the warning should be changed. Either way it's not relevant to this bug.

Comment 31

[Stephen Donner [:stephend] Not actively reading bugmail]

Sorry Simon/Shawn: I was just grasping for straws, and saw that warning come up when Marcia was downloading a PDF at work in her debug build.

Comment 32

[Shawn Wilsher :sdwilsh]

Simon - is this patch good to go?

Comment 33

[Simon Montagu :smontagu]

Yeah, except for lack of testcase. The problems are the same as bug 419157. I tried doing it another way for this bug by just creating a file in xpcshell with this filename and then checking the name on disk, but that doesn't go through any codepath that filters illegal characters.

Comment 34

[Simon Montagu :smontagu]

BTW, is P4 the right priority here? It seems too low to me.

Comment 35

[Shawn Wilsher :sdwilsh]

Probably at least P3, maybe P2.

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Shawn, right now P3 means that we won't block final release on it. If you think it needs to be in final, move to P2. Hard for me to tell how common a case this will be, but it feels like we should probably cover the bases.

Comment 37

[Shawn Wilsher :sdwilsh]

This won't be an issue for english, but I bet it'll come up more for non-english.  Given our market share stats, I think it's a good idea to block on.

Comment 38

[Mike Connor [:mconnor]]

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

Shawn, can you review?  This is in litmus at least, and we need to get it landed.

Comment 39

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh, but I'm not a suitable reviewer for this.  Asking bsmedberg to take a look as well.

Comment 40

[Benjamin Smedberg]

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

Why is CONTROL_CHARACTERS a multiline string? I think I'd prefer a concatenated string of the form

"\001\002\003\017" \
"\020\021\022"

r=me with that change

Comment 41

[Simon Montagu :smontagu]

Attachment: Patch for checkin

Comment 42

[Simon Montagu :smontagu]

Checked in

Comment 44

[Stephen Donner [:stephend] Not actively reading bugmail]

With trunk builds on Windows, Linux, and Mac OS X 10.4, I see:

IT839_µ(m8)2.pdf; is that correct?

Comment 45

[Simon Montagu :smontagu]

(In reply to comment #44)
 
> IT839_µ(m8)2.pdf; is that correct?

Yes :) (see bug 419157 comment 7 -- the filename should really be IT839전략(국문)2.pdf, but the Content-Disposition header gives it as IT839µ(m8)2.pdf, so IT839_µ(m8)2.pdf is the expected result, with the 0x04 character replaced by "_")

Comment 46

[Stephen Donner [:stephend] Not actively reading bugmail]

Thanks, Simon.

Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042704 Minefield/3.0pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre

-and-

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008042704 Minefield/3.0pre

Comment 47

[BugzillaJade.5.JadeNB]

This seems to be broken in RC2 on Mac OS X 10.4PPC.  On beta 5, [/] or [:] characters were silently replaced with [_] (even replacing [::] with [_] rather than [__], which was confusing) -- maybe not the ideal functionality, but at least nothing was lost.  On RC2, the download silently fails if the file name contains [/] or [:].  The .part file remains in the download directory, but the complete file does not seem to be downloaded.

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Works excellent for me with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9) Gecko/2008053008 Firefox/3.0

Do you have an example URL of such a broken download manager behavior?

Comment 49

[BugzillaJade.5.JadeNB]

(In reply to comment #48)
> Works excellent for me with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4;
> en-US; rv:1.9) Gecko/2008053008 Firefox/3.0
> 
> Do you have an example URL of such a broken download manager behavior?

It seems unlikely that this would explain the difference, but I'm on a PPC Mac, not an Intel Mac.

The behaviour isn't URL-specific -- if I try to save any downloaded file (whatever its original name) with a [/] in the file name, then the download fails in the manner described in <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=393488#c47">comment #47</a>.  (The mention of [:] in that comment was wrong -- that character is translated in file dialogues by the OS, not Firefox, into a [-].)

Comment 50

[BugzillaJade.5.JadeNB]

Hmm, I'm sorry.  On further testing, what I wrote isn't true -- if I try to save this page, for example, it works fine.  I can reproduce the error when trying to save a PDF -- for (a random) example, http://www.google.com/press/guides/desktop_mac_overview.pdf, trying to save it as (say) a/b.pdf.