124553 - Changes to LDAP hostname/Base DN in Address Book window not reflected by Quick search (if you do the same qs) until restart

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[yulian chang]

2002020803 builds on NT and Linux.

If you change the LDAP properties in Address Book window, the changes are not
reflected by Quick search until restart N6 and Address Book window.
But autocompletion honored the changes immediately without restart though.

1. launch Address Book window
2. select one LDAP dir and perform quick search
3. double click the selected LDAP dir to open Directory Server Properties dislog
4. change the Hostname to unknown one
5. do quick search against this LDAP 

Actual result: quick search still works
Expected result: quick searh should not work against unknown Hostname

Comment 1

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

does the properties dialog alter the hostname pref, or the uri prefs?

if the uri pref, this should work.  I remember testing this when I exposed LDAP 
to the addressbook.

Comment 2

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

when I change the basedn, it works.  but when I change just the hostname, it 
doesn't.

I'm not sure what the js for this dialog does, but it must not be updating the 
uri pref.

note, we shouldn't be poking prefs directly *anywhere*.  we need to add a modify
() to nsIAddressbook.

we'll need this, to fix the issue where changing the pretty name doesn't update 
and to add File | Rename (if the spec calls for that) to the addressbook.

Comment 3

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

I think I see part of the problem.

if I just change the hostname, and I do the same query ("test"), it behaves 
like yulian reported.

if I change the hostname, and change the query ("test2"), it works!

its as if we use the same connection, or at least the same nsAbLDAPDirectory, 
and the connection is established.

Comment 4

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

attempting to update the summary, something bad is going on, not in srilatha's 
code, but in mine.

Comment 5

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

something very bad is going on, marking 0.9.9, this bug has just exposed the 
issue.

Comment 6

[yulian chang]

2002021803 Win2K

1. do quick search with the following strings: 
   yu
   sri

   It wroks as expected.

2. Change to unknown Hostname and use the same strings from Step 1. to search.
It still gets the same search results which are not expedted.
3. But if searching with other strings as following, no search performed as
expected.
   cath 
   wang

4. Restart N6 and launch Address Book window
5. quick search with "yu" and "sri" 

Actual result: get the same search results as Step 1
Expected result: shouldn't find any matching ones against unknown Hostname

6. search with "cath" and "wang". No matching found.
7. change the Hostname back to valid one as in Step 1
8. quick search works normally  

Comment 7

[yulian chang]

Step 5. from #6 behaves as expected if not using turbo.

Comment 8

[yulian chang]

Bug 132382 is a duplicate of this bug.

Comment 9

[yulian chang]

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

Comment 10

[John G. Myers]

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

Comment 11

[Phil Gregory]

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

Comment 12

[Mark Banner (:standard8)]

As of Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.8b4) Gecko/20050913
SeaMonkey/1.0a, this still applies, and still includes BaseDN as a problem area
(see also duplicates of this bug).

A while ago Seth told me he's not really working on bugs anymore, so reassigning
to default, Seth if you are still working on this, please reassign back to yourself.

Comment 13

[Mark Banner (:standard8)]

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

Comment 14

[Mark Banner (:standard8)]

Note that bug 297549 may be related.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Assigning bugs that I'm not actively working on back to nobody; use SearchForThis as a search term if you want to delete all related bugmail at once.

Comment 16

[Mark Banner (:standard8)]

I have a WIP patch for this.

Comment 17

[Mark Banner (:standard8)]

Attachment: WIP Patch v1

WIP patch - I think this does everything correctly, but I still need to check it through again before submitting for review.

Comment 18

[Mark Banner (:standard8)]

Attachment: Patch v2 (diff -w)

This patch fixes this bug and bug 297549.

I've moved the values that were stored in nsAbLDAPDirectory so that they are created/gathered mainly by nsAbLDAPDirectoryQuery when a search is performed. This means they aren't cached and therefore we can change them whilst running the app.

For the connection, its still cached, however I've added some checking in so that if anything vital changes it'll drop the old connection and start a new one. This also required implementing nsLDAPURL::Equals.

For testing I've left the printfs in (which I'll remove before checkin), its easiest to open up the address book window, then also open prefs and edit directories. You can then edit the directory in there whilst playing around with the address book window.

Note that if you clear the search then you'll drop the connection anyway, but if you search for "a" then "ab" you'll keep the connection if nothing has changed which would cause it to be dropped.

Comment 19

[Mark Banner (:standard8)]

Attachment: Patch v2 (full)

Comment 20

[Mark Banner (:standard8)]

Attachment: Patch v3 (diff -w)

fixed bitrot. Neil, can you try this patch again? Ian told me that he had a problem with crashing which was fixed after a clean build (may have been something to do with the ldap change to version 6 a while ago).

Comment 21

[Mark Banner (:standard8)]

Attachment: Patch v3 (full)

Comment 22

[[e-mail]]

Comment on attachment 267819 [details] [diff] [review]
Patch v3 (diff -w)

OK, I clobbered and rebuilt directory/c-sdk and this patch seems to work (after changing the host name to an invalid name further quick searches fail).

Comment 23

[David :Bienvenu]

Comment on attachment 267819 [details] [diff] [review]
Patch v3 (diff -w)

thx for the patch, Mark!

-    NS_IF_ADDREF(*result = card);
-    return NS_OK;
+  NS_IF_ADDREF(*url = result);
+  return rv;

Can use .swap here?

+  if (!mConnection || !mDirectoryUrl)
+  {
+    printf("No connection and/or dir url\n");
+    mDirectoryUrl = currentUrl;

Can you remove the printfs, or replace them with a PRLog or #ifdef DEBUG them?

+      searchFilter = NS_LITERAL_CSTRING("(&") + urlFilter;

In the xulrunner world, won't this code need to use the same string api as the mailnews backend, e.g., .Appends instead of '+'? If so, might want to do that now.

+      if (thisSpec == otherSpec && mOptions == otherOptions)
+      {
+        eq = PR_TRUE;
+      }

In this code, 'eq' is hardly needed - you could just use *_retval.  You could simple say *_retval = thisSpec == ..., and init it to false at the top of the method. 

You should make sure this doesn't cause us to leave SSL connections open and/or leak them (any more than we do now :-()

Comment 24

[Mark Banner (:standard8)]

(In reply to comment #23)
> (From update of attachment 267819 [details] [diff] [review])
> thx for the patch, Mark!
> 
> -    NS_IF_ADDREF(*result = card);
> -    return NS_OK;
> +  NS_IF_ADDREF(*url = result);
> +  return rv;
> 
> Can use .swap here?

Fixed

> +  if (!mConnection || !mDirectoryUrl)
> +  {
> +    printf("No connection and/or dir url\n");
> +    mDirectoryUrl = currentUrl;
> 
> Can you remove the printfs, or replace them with a PRLog or #ifdef DEBUG them?

Yeah I was going to remove them anyway, just was leaving it to checkin.

> +      searchFilter = NS_LITERAL_CSTRING("(&") + urlFilter;
> 
> In the xulrunner world, won't this code need to use the same string api as the
> mailnews backend, e.g., .Appends instead of '+'? If so, might want to do that
> now.

Done
> +      if (thisSpec == otherSpec && mOptions == otherOptions)
> +      {
> +        eq = PR_TRUE;
> +      }
> 
> In this code, 'eq' is hardly needed - you could just use *_retval.  You could
> simple say *_retval = thisSpec == ..., and init it to false at the top of the
> method. 

done

> You should make sure this doesn't cause us to leave SSL connections open and/or
> leak them (any more than we do now :-()

This doesn't actually change the connections caching - just the caching of the ldap url and how we reinitialise things. I did check that we drop the old connection if needing to create a new one though.

Comment 25

[Mark Banner (:standard8)]

Patch checked in with comments addressed -> fixed.