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

RESOLVED FIXED

Status

defect
--
major
RESOLVED FIXED
18 years ago
11 years ago

People

(Reporter: yulian, Assigned: standard8)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: nab-ldap)

Attachments

(2 attachments, 3 obsolete attachments)

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
Keywords: nsbeta1
Whiteboard: nab-ldap
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.
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.
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.
No longer blocks: 125821
attempting to update the summary, something bad is going on, not in srilatha's 
code, but in mine.
Assignee: srilatha → sspitzer
Summary: Changes to LDAP properties in Address Book window not reflected by Quick search until restart → Changes to LDAP hostname in Address Book window not reflected by Quick search (if you do the same qs) until restart
Severity: normal → major
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
something very bad is going on, marking 0.9.9, this bug has just exposed the 
issue.
Keywords: nsbeta1nsbeta1-
Target Milestone: mozilla0.9.9 → mozilla1.2
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  
Step 5. from #6 behaves as expected if not using turbo.
Bug 132382 is a duplicate of this bug.
Summary: Changes to LDAP hostname in Address Book window not reflected by Quick search (if you do the same qs) until restart → Changes to LDAP hostname/Base DN in Address Book window not reflected by Quick search (if you do the same qs) until restart
*** Bug 132382 has been marked as a duplicate of this bug. ***
OS: Windows NT → All
Hardware: PC → All
*** Bug 131800 has been marked as a duplicate of this bug. ***
QA Contact: yulian → gchan
*** Bug 202901 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
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.
Assignee: sspitzer → dmose
Status: ASSIGNED → NEW
Target Milestone: mozilla1.2alpha → ---
*** Bug 299965 has been marked as a duplicate of this bug. ***
Note that bug 297549 may be related.
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.
Assignee: dmose → nobody
I have a WIP patch for this.
Assignee: nobody → bugzilla
QA Contact: grylchan → ldap-integration
Posted patch WIP Patch v1 (obsolete) — Splinter Review
WIP patch - I think this does everything correctly, but I still need to check it through again before submitting for review.
Posted patch Patch v2 (diff -w) (obsolete) — Splinter Review
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.
Attachment #264406 - Flags: review?(neil)
Blocks: 297549
Posted patch Patch v2 (full) (obsolete) — Splinter Review
Attachment #264117 - Attachment is obsolete: true
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).
Attachment #264406 - Attachment is obsolete: true
Attachment #264407 - Attachment is obsolete: true
Attachment #267819 - Flags: review?(neil)
Attachment #264406 - Flags: review?(neil)
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).
Attachment #267819 - Flags: review?(neil) → review+
Attachment #267819 - Flags: superreview?(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 :-()
Attachment #267819 - Flags: superreview?(bienvenu) → superreview+
(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.
Patch checked in with comments addressed -> fixed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 339330
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.