79869 - LDAP: multibyte directory name not displayed correctly.

Status: VERIFIED FIXED

(MailNews Core :: Internationalization, defect, P1)

Description

[ji]

*****Observed with linux 05/07 trunk build*****

If I enter non-ASCII chars for directory name and base DN, it's not displayed correctly after I save and come back to the edit window.

Steps to reproduce:
1. Launch Mail with a profile file which already setup a mail account.
2. Select Edit | Mail/News Account Settings.
3. Select Server Settings under an account.
4. Check on "Override my global LDAP directory server..." 
5. Click on Edit Directories
6. Click on Add.
7. Enter non-ASCII characters for Name and base DN like o=<non-ascii chars>, and click on OK button to save it.
   You'll see the direcotry name is displayed grabled on the list and if you click on Edit to open it, both directory name and base DN are garbled.

Comment 1

[ji]

Not sure if it's save problem. Changed the summary.

Comment 2

[nhottanscp]

Xianglan, please attach a screen shot.

Reassign to putterman.

Comment 3

[scottputterman]

reassigning to srilatha

Comment 4

[ji]

Attachment: A screenshot of Edit window.

Comment 5

[nhottanscp]

Xianglan, for the test case of the screen shot, what characters did you actually
input?

Comment 6

[ji]

Since I'm on linux, it's EUC. 

Comment 7

[ji]

Same problem on Windows, which uses Shift-JIS.

Comment 8

[nhottanscp]

I think this is also reproducible with Latin1. Looks like raw UTF-8 is diaplayed.

Comment 9

[ji]

Latin-1 seems okay. 

Comment 10

[Hong Kwon]

This bug depends on the wstring landing in 71247.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Actually, I think this two separate bugs.  One, the base DN not being displayed
(or used!) correctly is really just an instance of bug 71427.  The other, the
directory name, is something else.  I'm narrowing this bug to just be about the
directory name, removing the dependency on 71247, making block 17880, the master
autocomplete tracking bug.

Comment 12

[ji]

Is directory name saved into the same file as base DN?

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

They're both saved into prefs.js, I believe.

Comment 14

[ji]

There are three visual problems here:
1. The directory name is garbled on server settings window.
2. The directory name is garbled on LDAP directory server list window.
3. Both directory name and base DN are garbled on Edit window.
Do you think I need to file a seperate bug for base DN display problem on #3?

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

I think annotating 71247 with a reminder to be sure and verify that the base DN
problem is fixed when that bug is being verified should be sufficient.

Comment 16

[Jussi-Pekka Mantere]

PDT+ per 6/12 mtg.

Comment 17

[Srilatha Moturi]

Attachment: patch v1

Comment 18

[Mitesh Shah]

- CopyUnichar and SetUniChar are depricated. They are replaced by
Get/SetComplexValue. In fact, the whole nsIPref interface is depricated and is
replaced by nsIPrefService and nsIPrefBranch. I think it's better if you replace
it nsIPref with nsIPrefService. CCing bnesse for his view.

-  You may want to change CopyCharPref to CopyUniCharPref for 'dn' attribute at
line:116 in pref-directory.js 
  ldapUrl.dn = gPrefInt.CopyCharPref(pref_string + ".searchBase") 
same at line: 128 , setting spec in migrate()

Comment 19

[Srilatha Moturi]

migrate() will be removed from pref-directory.js and moved to 
nsLDAPPrefsService.js (bug # 79252). That is the reason why I did change these 
calls in here.

Comment 20

[Mitesh Shah]

I think it's ok to use nsIPref at this point. It will be converted to 
nsIPrefService during the whole tree cleanup. 

Since migrate() is moving from this file, it should be fine, but still I think 
it's better to convert it to *UniChar* with a comment, in case someone cuts and 
pastes it to the new file. 

Comment 21

[Brian Nesse (gone)]

This is fine if you fix in nsLDAPPrefsService.js, but your final patch in bug
79252 (attachment 38951 [details] [diff] [review]) is still using nsIPref also.

Comment 22

[Mitesh Shah]

with the comments in migrate() code, r=mitesh 

Comment 23

[Srilatha Moturi]

Attachment: patch v2

Comment 24

[Srilatha Moturi]

modified the patch based on comments from mitesh.
ccing Seth for an sr

Comment 25

[(not reading, please use [e-mail] instead)]

two comments:

1)

+    ldapUrl = ldapUrl = ldapUrl.createInstance().
+              QueryInterface(Components.interfaces.nsILDAPURL);

should be

+ ldapUrl = ldapUrl.createInstance().
+   QueryInterface(Components.interfaces.nsILDAPURL);

2)

+ try {
+ var ldapService = Components.classes[...

should be 

var ldapService;
try {
  ldapService = ...

since you refer to ldapService outside the scope of the try {} block.

you did this in two places.  I didn't check to see if moving it out of the try
block would make ldapService defined twice.  since it is a services, perhaps
having and using gLDAPService would be better.  (like you have gPrefInt)

Comment 26

[(not reading, please use [e-mail] instead)]

one last comment:

+ ldapUrl.dn = gPrefInt.CopyUnicharPref(pref_string + ".searchBase");

dn is a string (not wstring) attribute on nsILDAPURL.idl

in other parts of the patch you convert dn by using UTF8toUCS2. (and when going
back, UCS2toUTF8)

did the nsILDAPURL.idl interface change?  

Comment 27

[Srilatha Moturi]

+ ldapUrl.dn = gPrefInt.CopyUnicharPref(pref_string + ".searchBase");
I do not need to convert from UCS2 to UTF8. 
This should be something like
pref_string_content = gPrefInt.CopyUnicharPref(pref_string + ".searchBase");
ldapUrl.dn = ldapService.UCS2toUTF8(pref_string_content);
I did not pay much attention to this code since this part of migrate() and it is 
going to go away. (see bug # 79252)
I will post a new patch will the changes right away

Comment 28

[Srilatha Moturi]

Attachment: patch v3

Comment 29

[Mitesh Shah]

Regarding, 
------------------
+ try {
+   var ldapService = Components.classes[...
should be 
var ldapService;
try {
  ldapService = ...
since you refer to ldapService outside the scope of the try {} block.
---------------------

I think in Javascript there is no block level scope. So if a variable is defined  
anywhere in a function with 'var ...', it's available as a local variable 
throughout the function. 
There are few other places in both the files where 

try {
  var ldapUrl = ...
}
 ldapUrl = ...

syntax has been used.

Comment 30

[Leif Hedstrom]

I saw the other comment Re: changing |DN| in nsILDAPURL to a |wstring|. I need
to talk to dmose about this, I left it |string| intentionally, since other parts
of the URL methods (e.g. |SetSpec| and |GetSpec|) can't be changed to |wstring|.

My patch for this has a doxygen comment mentioning that all |string| in the URLs
are supposed to be UTF8 encoded. Again, let me talk to dmose, and we can decide
if |DN| and |Filter| should be |wstring| in nsILDAPURL.idl.

Thanks,

-- Leif

Comment 31

[(not reading, please use [e-mail] instead)]

are you running with js warnings enabled?

user_pref("javascript.options.strict", true); (in your prefs.js file?)

it looks like you'll get a js warning in pref-directory.js

because ldapService is not declared

I'm not sure about the UTF8toUCS2 and UCS2toUTF8 usage in your patch.

dn is a string (a c string) so setting it to a UTF8 string seems like you are
going to lose the high bits.

dmose / leif, can you verify that the usage in the last is correct?

Comment 32

[Leif Hedstrom]

I believe Srilatha is doing the right thing, |string| (C-strings) can hold UTF8
encoded strings.

-- Leif

Comment 33

[(not reading, please use [e-mail] instead)]

wait, are you sure about that?

I thought that bytes in a UTF8 string can have the high bit set, but when you
cross the XPConnect boundry for ASCII string, we'll loose the high bit.

dveditz / nhotta, can you comment?

Comment 34

[nhottanscp]

As leif mentioned, |string| can hold UTF-8 string like char* can hold Latin1
characters.
Usually problem happens when AssignWithConversion or NS_ConvertASCIItoUCS2 are
used for UTF-8 strings. So it's okay as long as proper UTF-8 to UCS2 conversion
is applied (e.g NS_ConvertUTF8toUCS2).

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

dbradley just verified by reading the code that XPConnect will not munch the
high bit on |string| values, so I think we're good.

Comment 36

[(not reading, please use [e-mail] instead)]

sr=sspitzer on the last patch, but there appears to be some strict js warnings.

feel free to find them and fix them with a new patch now, or land this patch now
and log a bug and fix the strict js warnings after 0.9.2.

Comment 37

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 38

[Srilatha Moturi]

Fix checked in

Comment 39

[ji]

Verified with 06/20 windows and mac trunk build. It's fixed.
Will verify with linux build when a testable build is available.

Comment 40

[ji]

Verified with linux 06-22-11 trunk build. It's fixed.