189301 - Can't save any file to Japanese directory with "Open a progress dialog"

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Hideo Oshima]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030115
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030115

If you set download method as "Open a progress dialog"
you cannot any file to Japanese directory.
Setting "Open the download manager", it's OK.

Reproducible: Always

Steps to Reproduce:
1.create Japanese diretory like テスト
2.Open Prefereneces/Navator/Downloads 
  and select "Open a progress dialog"
3.Download any file to the Japanse directory such as テスト

Actual Results:  
Nothing is saved.

Expected Results:  
The file you select is saved.

Comment 1

[Owen Marshall (Not reading bugmail)]

This isn't DOM Load and Save. -->Browser-General for triage, but it should 
probably go to Download Manager... I will let someone else make that call ;)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Does the progress dialog actually come up?  Does the transfer complete?

This sounds like a download manager bug....

Comment 3

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

as this is a progress dialog bug, not a download manager one (per comment 0),
this is file handling.

Comment 4

[HARUNAGA Hirotoshi]

Confirming with 2003011603/MacOS9.2.2.
Japanese reporters say that also 2003011412/Win98 and 2003011508/WinNT
have the same problem.
Progress dialog doesn't come up, and nothing happens.
Regression.

Comment 5

[HARUNAGA Hirotoshi]

2002122308 has no problem, 2003010503 has this problem.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Are there any errors in the JS console?  Since I have no Japanese directories
and don't know how to create any, someone's going to have to help me reproduce
this...

biesi, I still do not see any evidence that this is not a bug in the download
manager back end (which drives both the progress dialog and the download manager
UI).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Also, having a smaller period to search for regressions in (2 weeks is pretty
long) would be helpful.

That said, of the checkins in that 2-week period the one that looks most
suspicious is Simon's checkin from Dec 30 ("Fix the Downlaod Manager to not be
totally hoarked [sic] on Mac").  It does not have a bug# listed,
unfortunately... But it _did_ change from using native paths to using the UTF8
version of the path in nsDownloadManager.cpp... that could well cause some sort
of issues if someone else somewhere is still using native paths...

Comment 8

[Boris Zbarsky [:bzbarsky]]

And in fact, that _is_ the checkin that broke this... see the code in Init() in
nsDownloadProxy.h

Could someone who builds and can reproduce the problem change that code to use
GetPath and NS_ConvertUCS2toUTF8 and see whether that helps?

If no one seeing this problem builds, I say we just check in that change and let
people test.... ;)

Comment 9

[Simon Fraser [no longer active]]

Hrm, I thought I was fixing i18n issues, not creating them. The old code used to
get native paths (in the file system encoding) and stuff them into RDF, and the
new code gets UTF-8 paths.

Does this bug happen on Mac?

Comment 10

[Simon Fraser [no longer active]]

Oh, I probably forgot to make the respective changes to the progress dialog JS.

Comment 11

[Simon Fraser [no longer active]]

This WFM on Mac OS X; I created a folder with a japanese name, and can d/l into
it just fine. The Japanese name shows up in the download dialog.

The download dialog is using unicode paths, so no problem there.

 83       rv = aTarget->GetNativePath(path);
 84       if (NS_FAILED(rv)) return rv;
 85       return dm->OpenProgressDialogFor(path.get(), nsnull);

As bz says, I think the bug is here.

Comment 12

[Simon Fraser [no longer active]]

Index: src/nsDownloadProxy.h
===================================================================
RCS file: /cvsroot/mozilla/xpfe/components/download-manager/src/nsDownloadProxy.h,v
retrieving revision 1.14
diff -u -r1.14 nsDownloadProxy.h
--- src/nsDownloadProxy.h	8 Jan 2003 23:19:08 -0000	1.14
+++ src/nsDownloadProxy.h	17 Jan 2003 19:50:14 -0000
@@ -79,10 +79,12 @@
     if (behavior == 0)
       return dm->Open(nsnull, this);
     if (behavior == 1) {
-      nsCAutoString path;
-      rv = aTarget->GetNativePath(path);
+      nsAutoString path;
+      rv = aTarget->Path(path);
       if (NS_FAILED(rv)) return rv;
-      return dm->OpenProgressDialogFor(path.get(), nsnull);
+
+      NS_ConvertUCS2toUTF8 utf8Path(path);
+      return dm->OpenProgressDialogFor(utf8Path.get(), nsnull);
     }
     return rv;
   }

This may fix it. However, when doing downloads to a dir with a japanese, name,
I'm getting lots of assertions:
U+0080/U+0100 - U+FFFF data lost: 'legalRange', file xpcconvert.cpp, line 788
which suggest that some unicode/ascii data whackage is going on somewhere. I
think this is XUL template stuff, but my fu there is weak. I'm passing in UTF8
everywhere, so am not sure where the problem lies.

Comment 13

[bobj]

ftang, can you help simon figure out what's going on with these assertions.

Comment 14

[Simon Fraser [no longer active]]

Attachment: More complete patch:fixes param names in IDL

Comment 15

[Boris Zbarsky [:bzbarsky]]

Any reason not to just make the IDL take AUTF8String instead of string?

Comment 16

[Simon Fraser [no longer active]]

Could do, yes.

I wonder if the issue here is that rdf doesn't like the uris we pass to
GetResource. We're just giving it a full path in UTF8, not a URI, so there may
be some URI escaping issues. I'm sure this was broken before my changes too....

Comment 17

[Simon Fraser [no longer active]]

CFM builds fail badly here too. In discussion with Conrad, this seems to be
because nsIFile::GetPath() has issues returning a good unicode path if the
directories have mixed encodings. On MachO, this isn't an issue. I wonder if
Linux suffers in the same way.

The current scheme, of using full paths to identify downloads, is broken anyway,
because they are not guaranteed to be unique (try downloading the same file
twice at the same time). We don't uniquify the download location until after the
download is done, when we rename the file from the salted name. All this is
making me think that we should just assign downloads unique IDs which don't
suffer these issues.

Comment 18

[Simon Fraser [no longer active]]

Oddly, on Linux (RH8), I'm able to save a file to a dir with a non-ASCII name
just fine, using the progress dialog, with and without the patch pasted above.

Comment 19

[nhottanscp]

Simon, is this related to a check in of other bug? Any changes for the download
dialog recently? If so, what is the bug number?

Comment 20

[Simon Fraser [no longer active]]

This regressed when I fixed bug 155426.

Comment 21

[Simon Fraser [no longer active]]

Attachment: Complete patch

This patch fixes a bunch of issues, hopefully including the one this bug is
about.

* nsIDownloadManager was changed to use AUTF8String, and the comments fixed to
  indicate that the methods take utf8 paths, not persistent descriptors. This
  change fixes most of the UTF8 assertions noted above.

* nsDownloadManager.cpp/.h were fixed to use nsACString, to match the IDL. Some

  minor string fumbling had to be added (PromiseFlatCString etc).

* nsDownloadProxy.h was fixed to call OpenProgressDialogFor with a utf8 path,
  as discussed above (the real fix for this bug).

* downloadmanager.js was changed to not allow a progess dialog to be shown
  for completed downloads (since it just sits there, looking broken), and to
  disallow the revealing of unfinished downloads (becasue we can't reveal,
since
  we have the salted filename still).

  Also, gRDFService.GetResource(aElement.id); was changed to
  gRDFService.GetUnicodeResource(aElement.id); to fix more of the UTF8 
  assertions.

* downloadmanager.xul was fixed to add a <stringbundleset> so that string
  bundles are correctly included from overlays.

* resources/mac/jar.mn was fixed so that we actually package the 
  dlmanagermenuoverlay.xul file that I added last time (whoops).

Comment 22

[hirata masakazu]

Is bug 188263 related to this?

Comment 23

[jag (Peter Annema)]

Comment on attachment 111876 [details] [diff] [review]
Complete patch

sr=jag

Comment 24

[Boris Zbarsky [:bzbarsky]]

pkw, this is likely the reason you were seeing that exception -- we're using the
name to key the download and we're using the name from _before_ CreateUnique got
called so we're getting the wrong object....

Simon, I'll try to review this in the next 24 hours.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111876 [details] [diff] [review]
Complete patch

Oh, of course!	When passing a |string| param from JS, XPConnect will strip the
high bit off the chars!  So yeah, that IDL change is really needed to have this
work right.  ;)

>+  <stringbundleset id="stringbundleset">
>   <stringbundle id="dlMgrBundle"
>                 src="chrome://communicator/locale/downloadmanager/downloadmanager.properties"/>
>   <stringbundle id="dlProgressDlgBundle"
>                 src="chrome://global/locale/nsProgressDialog.properties"/>
>+  </stringbundleset>

Please reindent the <stringbundle>s.

Looks great otherwise.

As you mentioned, using strings for this in general is broken; the string we're
using is _not_ the actual location of the file in many cases since we seem to
be grabbing it before the CreateUnique call that the helper app service makes
(so if you try to download the same file twice in a row, you get some nice JS
exceptions out of the download manager the second time).  Could you file a
separate bug on that and cc me and [e-mail]?

Comment 26

[Simon Fraser [no longer active]]

Filed bug 189565. Other bugs that should be tested once this is checked in:
bug 155429 bug 157062 bug 139606 bug 165576 bug 175881 bug 180354 bug 187600 bug
185733 maybe.

Comment 27

[Asa Dotzler [:asa]]

We should definitely try to get this in for 1.3beta. setting blocking1.3a+ flag.

Comment 28

[Simon Fraser [no longer active]]

Checked in. When verifying, please also test the bugs noted in comment 26.

Comment 29

[Hideo Oshima]

I confirmed to fix the problem.
Thanks for great work.

By the way, I think bug 157062 and bug 165576 are also fixed.

Comment 30

[OstGote!]

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

Comment 31

[OstGote!]

Please change someone the OS to All and change the summary to be more generic
because I think this bug is for all intl. characters. The bug occurs not only if
the target directory has intl chars but also if the local file name has intl
chars like in my duped bug 188884. WFM now to save files (Progress dialog) with
German umlauts and I can save to a directory with it. Both bugs from comment 29
WFM too.

Thanks for your work! Although my bug did'nt get so much attention ...

Comment 32

[Chris Petersen]

Verified per last comments.

Comment 33

[R.K.Aa.]

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