208903 - download manager doesn't work with non-ASCII file names.

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Jungshik Shin]

The patch for bug 175881 introudced this problem. (attachment 124826 [details] [diff] [review]).
The patch has the following line 

-    gStatusBar.label = getSelectedItem().id;
+    gStatusBar.label = getFileForItem(getSelectedItem()).path;

The noticable change introduced by this is that the status bar shows 'garbage'
characters for non-ASCII file names. Going back to 
|getSelectedItem().id| (chopping off 'file://' at the beginning if necessary)
will solve this 'UI' regression. However, that's not touching the root cause of
the problem because none of command buttons is functional (all of them is dimmed
out) when a non-ASCII file is  selected , with or without the change. The file
operation in other parts of Mozilla work fine so that nsIFile and nsILocalFile
are not to blame. [1]

getFileForItem() is defined in 
http://lxr.mozilla.org/seamonkey/source/xpfe/components/download-manager/resources/downloadmanager.js#357
as
357 function getFileForItem(aElement)
358 {
359   var itemResource = gRDFService.GetUnicodeResource(aElement.id);
360   var fileResource = gDownloadView.database.GetTarget(itemResource,
gNC_File, true);
361   fileResource =
fileResource.QueryInterface(Components.interfaces.nsIRDFResource);
362   return createLocalFile(fileResource.Value);
363 }
364 
365 function createLocalFile(aFilePath) 
366 {


I added a couple of 'dump()' statements in createLocalFile(aFilePath) and
printed out |aFilePath).  aFilePath is "zero-extended UTF-8". For instance, 
U+00A0 (in UTF-8, it's 0xC2 0x80), aFilePath should have 'U+00A0'. Instead, it
has 'U+00C2 U+0080'. This symptom suggests that somewhere in RDF-related code,
ACString is used where AUTF8String should have been used. 

[1] Recently, I've been working on enabling 'Unicode' file I/O in Windows (bug
162361) and with that enabled, Mozilla-Windows has the same problem as Linux
under UTF-8 locale. Therefore, I think this is an XP issue althouhg I'm filing
this under Linux for now.

Comment 1

[[e-mail]]

Just to narrow down the problem slightly, is the correct value stored in
downloads.rdf in the profile?

Comment 2

[Jungshik Shin]

This is to show that aElement.id has the correct value for U+AC00 and U+AC01
while aFilePath in  'zero-extended' UTF-8 for two characters.

in getFileForItem : aElement.id=        
66, 69, 6C, 65, 3A, 2F, 2F, 
2F, 68, 6F,6D, 65, 
2F, 6A, 75, 6E,
67, 73, 68, 69, 6B, 
2F, AC00, AC01

in createLocalfile : aFilePath = 
2F, 68, 6F, 6D, 65, 
2F, 6A, 75, 6E, 67, 73, 68, 69, 6B, 
2F, EA, B0, 80, EA, B0, 81

aFilePath in createLocalfile is obtained in getFileForItem(). See my bug report.
In that function, fileResource.Value (nsIRDFResource.Value) is 'string' (not
'wstring'). I tried to change that to Literal (which has wstrng Value) but then '
gDownloadView.database.GetTarget' fails because of the type mismatch. 

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Could not convert JavaScript argument arg 0
[nsIRDFCompositeDataSource.GetTarget]"  nsresult: "0x80570009
(NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame ::
chrome://communicator/content/downloadmanager/downloadmanager.js ::
getFileForItem :: line 367"  data: no]
************************************************************


I think we have to change the way ??? are built from downloads.rdf.

> is the correct value stored in downloads.rdf in the profile?

  Yes, it is.(I've just checked to make sure) Otherwise, filenames wouldn't be
correctly displayed in the download manager. 

Comment 3

[Jan Varga [:janv]]

The problem is that there is resource (string) and file property (wstring).
File property is displayed in the download manager IIRC.
So maybe we should just use the file property in the status bar too.

Comment 4

[Jungshik Shin]

It looks like Jan's patch for bug 208113 will fix this problem as well because
it switches to RDFLiteral from RDFResource. Then, I guess can take Value(wstring). 
I'll try it.

Jan, the status bar is just a part of the problem. Even if the status bar
correctly displays the file name (as was the case until your patch for bug
175881 went in), that doesn't solve the problem with none of command functional
(because file.Exists() fails). 

As it stands, utf8Path is stored in nsDownloadManager.cpp, but it's stored in
RDFResource and later retrieved via 'GetValue' (which is just string). Actually,
a bigger question to ask is why Value (nsIRDFResource)is 'string' in the first
place. Shouldn't it have been AUTF8String? 

However, your patch for bug 208113 looks promising. The downside of that patch
is that (I'm not sure, though) downloads.rdf may not be in 'nice' UTF-8. It'd be
in UTF-16, wouldn't it? 

Comment 5

[Jan Varga [:janv]]

>However, your patch for bug 208113 looks promising. The downside of that patch
>is that (I'm not sure, though) downloads.rdf may not be in 'nice' UTF-8. It'd be
>in UTF-16, wouldn't it? 

no, I think it'd be in UTF-8
I checked in nsRDFXMLSerializer.cpp::rdf_BlockingWrite()

Comment 6

[Ian Neal]

Looking at a file called %E9t%E942.txt which was downloaded in the RDF shows up
as  ASCII codes C3A974C3A9.

Comment 7

[Jungshik Shin]

Attachment: a patch

Jan's comment made me think again about what to do and came up with a different
approach than I mentioned. I just used file property everywhere. Is there any
problem with this? speed  ?

Comment 8

[Jungshik Shin]

> Looking at a file called %E9t%E942.txt which was downloaded in the RDF 
> shows up as  ASCII codes C3A974C3A9.

 That's  the very  symptom I described. 0xE9 is interpreted as ISO-8859-1
(because your locale's codeset is ISO-8859-1) and then converted to and stored
in UTF-8 (0xC3 0xA9). Now it should come up as U+00E9 (a single character), but
it come up as two characters (U+00C3 and U+00A9) after crossing the XPConnect
boundary (each byte in UTF-8 is zero-extended and converted to UTF-16 )  because
'Value' of RDFResource is string(instead of AUTF8String) that knows nothing
about UTF-8. 

Anyway, attachment 125311 [details] [diff] [review] solves the problem.

Comment 9

[Ian Neal]

I'm sure this patch would mean the status bar regressed back to showing file://
when it doesn't need to but that should be simple to fix.

Comment 10

[Jan Varga [:janv]]

>I'm sure this patch would mean the status bar regressed back to showing file://
>when it doesn't need to but that should be simple to fix.

No, that's not true. The problem here is that RDF itself changes these file
properties to file:/// because they are asserted as resources. My patch makes it
to assert as literals, so then they are not changed.
There's only one problem, we need to figure out how to fix an existing
downloads.rdf file.

Comment 11

[Jan Varga [:janv]]

grr, my bugmail order is messed up
forget my previous comment

Comment 12

[Jungshik Shin]

Attachment: a new patch (with file:// handling)

I got rid of 'file://' in the status bar.

Comment 13

[Jungshik Shin]

Comment on attachment 125390 [details] [diff] [review]
a new patch (with file:// handling)

asking r/sr

Comment 14

[[e-mail]]

Comment on attachment 125390 [details] [diff] [review]
a new patch (with file:// handling)

>+    gStatusBar.label = getSelectedItem().id.replace(/^file:\/\//,'');
Except that this doesn't properly convert urls back into paths...

Comment 15

[Jungshik Shin]

>+    gStatusBar.label = getSelectedItem().id.replace(/^file:\/\//,'');
> Except that this doesn't properly convert urls back into paths...

  Did you mean there could be url-escaped characters in paths obtained that way?
I wonder why I didn't see any url-escaped characters in my test. Anyway, if
that's an issue, it's easy to fix with nsISubURItoText  if not simple
url-unescaping does not work. 

Or, did you mean something else? 

  

Comment 16

[[e-mail]]

I mean that the URL to path conversion is platform-specific.

Comment 17

[Jungshik Shin]

Thanks. The following should work across platforms, right?   gStatusBar.label = createLocalFile(getSelectedItem().id).path;   

Comment 18

[[e-mail]]

Sounds good to me.

Comment 19

[Jungshik Shin]

Comment on attachment 125390 [details] [diff] [review]
a new patch (with file:// handling)

>Index: xpfe/components/download-manager/resources/downloadmanager.js

>+  if (selectionCount == 1) {
>+    gStatusBar.label = getSelectedItem().id.replace(/^file:\/\//,'');

With the above line (and one more line like it) replaced by 


gStatusBar.label = createLocalFile(getSelectedItem().id).path;

can I get r/sr, jan, neil, jag? It's so simple to fix but it will make the
download managers usable for a large portion of users. Thanks.

Comment 20

[Jan Varga [:janv]]

Comment on attachment 125390 [details] [diff] [review]
a new patch (with file:// handling)

r=varga, but please test across all platforms

Comment 21

[Jungshik Shin]

Thank you for r, Jan.

I've just tested on Win32 (with my work-in-progress for bug 162361 for all
Unicode chars on Win2k/XP. without it, just the locale charset). I can't put my
hand on Mac (OS/2 uses '\' like Windows does, right?), but it should work as
well as the current trunk code because both the current code and my patch rely
on createLocalFile() that makes use of nsILocalFile interface. My patch just
eliminates the indirection via getFileforItem(). If somebody could test it on
Mac, it'd be nice, but I'm quite sure that it doesn't introduce a regression on Mac.

jag, can you sr? TIA,

Comment 22

[Jungshik Shin]

> I've just tested on Win32

This doesn't mean that I tested only on Win32. I also tested on Linux. More
importanly, as I wrote, my patch can't possibly introduce a regression as far as
converting URLs to the OS-specifc path  is concerned because I didn't touch any
of nsILocalFile implementation that's responsible for the job.

I think this also has to go in for 1.4branch. Anyway, it'd be nice to get sr
soon. jag? Thanks.
 

Comment 23

[Jan Varga [:janv]]

I'll test on Mac ASAP

Comment 24

[Jan Varga [:janv]]

I just tested on Mac, it works fine.
You have my double r= :)

Comment 25

[Jungshik Shin]

Thanks, Jan, for your testing on Mac and double r :-).
jag, can I get sr?

Comment 26

[Ian Neal]

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

Comment 27

[Ian Neal]

Adding keywords and severity from dupe and setting OS to All. Would be nice if
this could be got into the branch as well as the trunk.

Comment 28

[Jungshik Shin]

Attachment: 125390 + comment #19

To expedite sr from jag, I'm attaching what I'm planning to land (attachment
125390 [details] [diff] [review] + comment #19). 

Ian, I'm going to ask for a1.4 once I check in the patch for the trunk.

Comment 29

[Samir Gehani]

adt: nsbeta1+/adt2

Comment 30

[Jungshik Shin]

Comment on attachment 126367 [details] [diff] [review]
125390 + comment #19

assuming Jan's r to be standing, I'm explicitly asking jag for sr.

Comment 31

[jag (Peter Annema)]

Comment on attachment 126367 [details] [diff] [review]
125390 + comment #19

>Index: xpfe/components/download-manager/resources/downloadmanager.js
>===================================================================
>@@ -131,8 +131,8 @@
>     gStatusBar = document.getElementById("statusbar-text");
>   
>   var selectionCount = gDownloadView.treeBoxObject.selection.count;
>-  if (selectionCount == 1)
>-    gStatusBar.label = getFileForItem(getSelectedItem()).path;
>+  if (selectionCount == 1) 

Nit: you added a space at the end there.

sr=jag

Comment 32

[jag (Peter Annema)]

Comment on attachment 126367 [details] [diff] [review]
125390 + comment #19

>Index: xpfe/components/download-manager/resources/downloadmanager.js
>===================================================================
>@@ -131,8 +131,8 @@
>     gStatusBar = document.getElementById("statusbar-text");
>   
>   var selectionCount = gDownloadView.treeBoxObject.selection.count;
>-  if (selectionCount == 1)
>-    gStatusBar.label = getFileForItem(getSelectedItem()).path;
>+  if (selectionCount == 1) 

Nit: you added a space at the end there.

sr=jag

Comment 33

[Jungshik Shin]

Comment on attachment 126367 [details] [diff] [review]
125390 + comment #19

Thanks all. Fix was just checked in with a spurrious space removed.  Now asking
for a1.4.  This is to fix a regression due to a patch committed  to both 1.4
branch and trunk in early June. A lot of Mozilla users are affected because
download manager does not work for even Latin-1 chars  in filename (let alone
Cyrillic/Greek/Asian and other chars). The patch (JS only) is rather simple and
has been tested on all three major  platforms.

Comment 34

[Ian Neal]

Jungshik Shin if you want to get this into 1.4 you'll probably have to lobby Asa
(or one of the other drivers) directly.

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 126367 [details] [diff] [review]
125390 + comment #19

moving approval request forward.

Comment 36

[Mike Kaply [:mkaply]]

Comment on attachment 126367 [details] [diff] [review]
125390 + comment #19

a=mkaply for 1.4

Comment 37

[Jungshik Shin]

Thanks for a1.4.x. I've just checked in the patch to 1.4.x branch 
 

Comment 38

[OstGote!]

Ehmm ... Where can I download new 1.4.x builds? I don't see a latest-1.4 dir.
The latest 2003-07-xx-yy-1.4 dirs contain only OS/2 builds.