419157 - Filename from HTTP headers with 8bit characters and no charset information truncated on Mac

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P2)

Description

[Michael Donat]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022204 Minefield/3.0b4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022204 Minefield/3.0b4pre

If this is not a bug, please revise FX 3.0 FFT - Downloading #45.

Reproducible: Always

Steps to Reproduce:
1. Set "Always ask me where to save files".
2. Click on http://www.mic.go.kr/secureDN.tdf?seq=940&idx=1&board_id=P_03_01_05 and save the file.

Actual Results:  
Note that when the save as dialog appears, the filename is blank.


Expected Results:  
A suggested filename should appear in the dialog.

Comment 1

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

Attachment: Screenshot

Comment 2

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

Sorry; it helps to read the steps to reproduce.

Yeah, I can confirm this with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022204 Minefield/3.0b4pre.

Michael: do you have any idea when this regressed?

Comment 3

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

Comment on attachment 305174 [details]
Screenshot

Ignore this!

Comment 4

[Michael Donat]

Stephen: No idea when this regressed - sorry.

The test results for Testcase ID #4607 suggest this is a platform issue. I'm surprised there's a failure recorded for the Mac and Solaris given the positive results on Linux.

Comment 5

[Simon Montagu :smontagu]

Are the positive results reported for Linux correct? It fails for me on all platforms. Note that the correct file name is IT839전략(국문)2.pdf (with Korean characters) -- saving the file as IT839µ(m8)2.pdf should be considered a failure even if the download succeeds.

Comment 6

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

* Linux trunk:

** Opening dialog:
http://img205.imageshack.us/my.php?image=picture8dn2.png

** Download manager:
http://img403.imageshack.us/my.php?image=picture2zf7.png

** On disk:
http://img235.imageshack.us/my.php?image=picture5eg6.png

* Mac trunk:

** "Opening..." dialog:
http://img146.imageshack.us/my.php?image=picture3su1.png

** Download manager:
http://img207.imageshack.us/my.php?image=picture7vu8.png

** On disk:
http://img253.imageshack.us/my.php?image=picture6ir0.png

Comment 7

[Simon Montagu :smontagu]

I need to back down from comment 5: the filename maybe *should* be IT839전략(국문)2.pdf, but it's IT839µ(m8)2.pdf already in the Content-Disposition header, so saving it that way is not a failure after all.

Comment 8

[Shawn Wilsher :sdwilsh]

I really have *no* idea about this bug.  I'm probably not the best assignee for it.

Comment 9

[Simon Montagu :smontagu]

This should be fixed by bug 393488.

Comment 10

[Simon Montagu :smontagu]

Attachment: Patch

On Mac 0x04 is a legal character in a filename, so we can fix this even without bug 393488.

Shawn, can you give me some pointers on how to write a testcase for this?

Comment 11

[Shawn Wilsher :sdwilsh]

32-bit or 64-bit?

In all seriousness though, I thought about this and wasn't so sure.  You'd have to use a mochitest I think that redirected to a file with the right headers.  I haven't actually done this before, but I think it exists elsewhere in the codebase.  Waldo might be useful to consult here, or sayrer.

Comment 12

[Simon Montagu :smontagu]

I know how to trigger a download with the right headers from a mochitest. The part I don't know how to do is (a) get past the "what do you want to do with this file" dialog and (b) retrieve the suggested filename from the filepicker.

Comment 13

[Shawn Wilsher :sdwilsh]

(a) should be easy with the window watcher, but I have no idea how to do (b)

Comment 14

[Shawn Wilsher :sdwilsh]

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

Our file pickers are native, so I don't think we can retrieve that information with a test.  I'm OK with a in-testsuite? on this bug.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

that patch does not touch the file picker at all, why do you think it can't be tested/is hard to test?

and why does this fix the bug? surely that patch can't produce the filename in comment 5?

Comment 16

[Simon Montagu :smontagu]

First of all, this bug has morphed and I didn't really realize until rereading it how unclear it is -- sorry about that. Comment 5 is also wrong: see comment 7. The Content-Disposition header from the server has already been through a lossy 16bit to 8bit conversion, so there's no chance of restoring the original Korean filename and the best we can hope for is to save the corrupted name without failing.

The issue originally reported has been fixed in bug 393488; the issue I'm fixing here is that on Mac the filename gets truncated as in the last 3 links in comment 6. This happens because after we check that the header isn't UTF-8 we try to fall back to the native charset, but on Mac NS_CopyNativeToUnicode just does CopyUTF8toUTF16 without any error checking. The patch just adds another level of fallback.

About testing, if there is a scenario where we can do something with the filename from the Content-Disposition header without even triggering any dialogs, we will be fine. 

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

well it seems like you could just test the nsIMIMEHeaderParam behaviour.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

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

r+sr=biesi if you update the IDL for this interface to document this new behaviour and add a unit test as mentioned in my comment above.

Comment 19

[Simon Montagu :smontagu]

Attachment: Patch for checkin

Comment 20

[Simon Montagu :smontagu]

Checked in

Comment 21

[(no longer active)]

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

Verified.