107558 - Unable to add LDAP directory

Status: VERIFIED WORKSFORME

(MailNews Core :: LDAP Integration, defect, P1)

Description

[Eric Hedström]

Steps:
Edit/Preferences
Mail & Newsgroups/Addressing
click "Edit Directories.."
click Add
type in the information for an LDAP server and click OK
(your new server shows up in the list)
click OK
(the new server doesn't make it back to the main Addressing window)
click Edit Directories again
(the new server is gone)

Also, I just imported this profile from NS4.77, and it did not bring over the
LDAP information from that, while it has in the past.

build 2001103003, win32-installer, windows 2000

Comment 1

[Srilatha Moturi]

Taking this. This is a regression caused by the checkin for bug # 106159

Comment 2

[Srilatha Moturi]

Attachment: patch v1

Comment 3

[Srilatha Moturi]

Attachment: patch v2

Comment 4

[Srilatha Moturi]

In the new patch, I have createChildList as a method in the interface 
nsILdapPrefsService. and in pref-directory.js I'm calling createChildList on 
nsILdapPrefsService. 

Comment 5

[Chip Clark]

in nsLDAPPrefsService.prototype.createChildList

(last 4 lines)
  aCount.value = count;
  if (!count)
    return null;
  return childList;

You assign aCount.value to count ... but then if no count return null - 
Put the return before you do the assignment to aCount.value
   (this will avoid js errors)

Comment 6

[Brian Nesse (gone)]

You probably don't want to call a local variable gPrefInt either :)

Comment 7

[Srilatha Moturi]

Attachment: patch v3

Comment 8

[Chip Clark]

r=[e-mail]  looks good to me

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

I think I see some issues with this patch; hope you don't mind me
chiming in with some review comments.  In particular...

a) since |createChildList| is only ever called with "ldap_2.servers" as
the |prefRoot|, is there any reason not to define this as a local |const|
var in the .js file rather than making it a param?  If you do this,
|parent| looks like it could be const as well.

b) when implementing XPCOM interfaces in JS, the way to indicate
errors to the caller is to throw an nsresult rather than returning
null.

   throw Components.results.NS_ERROR_FAILURE;

c) the regexp |re| currently in use doesn't look quite right: [.] will
match any character, not just a dot.  But regexps are more heavyweight
than necessary for this job anyway; instead of

   string = directoriesList[i].substr(parent.length, 
                                      directoriesList[i].length);
   strLen = string.search(re);

you might want to consider using

   strLen = directoriesList[i].indexOf(".", parent.length);

d) can you add @arg, @return, and @exception doxygen comments
documenting the possible arguments, the return code, and the possible
exceptions that can be thrown by the implementor?

Comment 10

[Alec Flett]

Comment on attachment 56243 [details] [diff] [review]
patch v3

documentation, documentation documentation!
What is createChildList doing? we need better variable names than 
"string" - what does "string" represent? I guess I'm just confused 
because I can't tell exactly what the point of createChildList is.. how does
it differ from getChildList()? 

Also, why create a seperate array and return that? Why not just muck 
with the original "directoriesList" and
remove the entries that don't match your regular expression?

Also, the varable "regexp" is just "", but it sure doesn't look like a 
regular expression to me.. it looks like some kind of match string.

Maybe all these questions would be cleared up if I had an explanation of what
 this function is doing.

Comment 11

[Srilatha Moturi]

Setting milestone to mozilla 0.9.6.

Comment 12

[Srilatha Moturi]

Attachment: patch v4

Comment 13

[Alec Flett]

Comment on attachment 56634 [details] [diff] [review]
patch v4

does this patch even work?
This line:
+      strLen = directoriesList[i].indexOf(".", parent.length);

will always return a value of 5, because parent (which is ALWAYS "ldap_2.servers.")
has a "." in the 5th position in the string... which means that the following if() statement will ALWAYS be true, and you'll always be truncating directoriesList[i] down to the "servers." portion of the string..

What I think dmose was saying with the whole const isssue, is that
if createChildList will always pass in "ldap_2.servers",
why not just change the api to:
void getServerList(out unsigned long aCount, [array, size_is(aCount), retval] out string aChildArray);
and then internally always use "ldap_2.servers."

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

alecf: actually, it won't always return 5; the second parameter to indexOf means
"start searching at this index", so this should actually work ok.  you are right
as to my original intent on the const stuff, but it looks like srilatha's new
patch may do better than that (I don't know the code well enough to be sure)
since it takes an existing nsIPrefBranch rather than creating a whole new one.

Comment 15

[Srilatha Moturi]

In the new patch, in createChildList I just assume ldap_2.servers. Actually in 
patch v3 you could pass any root and get the child List.
dmose is right, about indexOf. 
Alec: the answer to your question, how does createChildList differ from 
getChildList.

The array that is created by getChildList looks like this:
ldap_2.servers.Netscape.description
ldap_2.servers.Netscape.filename
ldap_2.servers.Netscape.replication
ldap_2.servers.Netscape.uri
ldap_2.servers.test.description
ldap_2.servers.test.filename
ldap_2.servers.test.replication
ldap_2.servers.test.uri

The arrray that is created by createChildList looks like this:
ldap_2.servers.Netscape
ldap_2.servers.test

Comment 16

[Srilatha Moturi]

Attachment: patch v5

Comment 17

[Srilatha Moturi]

chip, alec please review the last patch.

Comment 18

[Srilatha Moturi]

In the last patch I changed the api name to getServerList. Also I am using 
only one array directoriesList.

Comment 19

[Srilatha Moturi]

Attachment: the two array solution

Comment 20

[Srilatha Moturi]

After talking to Alec over aim, we decided to go with the two array solution 
since it is much cleaner.

Comment 21

[Alec Flett]

Comment on attachment 56775 [details] [diff] [review]
the two array solution

sr=alecf, with a few extra lines of comments that srilatha and I discussed

Also, I would like r=dmose on this

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 56775 [details] [diff] [review]
the two array solution

OK, two minor nits:

In the doxygen comments:

@aChildArray        Receives the array with "ldap_2.servers.<server-name>".

Shouldn't that really be

@param	aChildArray	stuff...

Also

+  try {
+    directoriesList = prefBranch.getChildList(prefRoot, prefCount);
+  }
+  catch (ex) {
+    directoriesList = null;
+  }

Instead of setting directoriesList to null here and executing a bunch more code, either throw a new NS_ERROR_FAILURE, or else remove the try/catch wrapper completely and allow the exception from nsIPref to propagate up the stack.

Fix those, and you've got r=dmose.

Comment 23

[Srilatha Moturi]

Fix checked in

Comment 24

[Luke Morey]

I'm seeing this again 20001112603, Windows XP Pro

Comment 25

[Brian Nesse (gone)]

WFM Mac OSX 2001112705 build.

Comment 26

[scottputterman]

Moving to 0.9.8.  This worksforme on the 11/26 build on Win 2000.  yulian, are
you able to reproduce this?

Comment 27

[scottputterman]

sorry, changed my mind.  If we can reproduce we should fix it soon.

Comment 28

[yulian chang]

Works for me with 2002010703 Wins build on NT.

Comment 29

[Srilatha Moturi]

Worksforme on windows XPPro with 2002010703. Please reopen this bug if you are 
still seeing the problem

Comment 30

[yulian chang]

Verified with Jan-7-2002 Wins build.