135231 - LDAP Quick search results keep duplicating with searches in Address Book, Select Addresses dialog and Sidebar

Status: RESOLVED FIXED

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

Description

[yulian chang]

20020402 trunk build on Win2K.

1. Open up Address Book window, Select Addresses dlg and Sidebar
2. Choose the same LDAP Directory server for AddressBook, Select Addresses dlg
and Sidebar
3. do Quick search in Address Book window
4. do Quich search with the same search string in Sidebar

Actual results: notice search results duplicate in Address Book window

5. do Quich search with the same search string in Select Addresses dlg

Actual results: notice search results duplicate in Address Book window and Sidebar

6. Clear and do Quick search with the same search string in Address Book window

Actual results: notice search results duplicate in Select Addresses dlg and Sidebar

Comment 1

[Heiki Kask]

Just noticed and planned to report exactly the same bug using

Mozilla 1.1
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.1) Gecko/20020826

Comment 2

[Heiki Kask]

... and LDAP quick search results in Address Book duplicate also when trying to
print whole LDAP addressbook.

Comment 3

[Gervase Markham [:gerv]]

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

Comment 4

[Mark Banner (:standard8)]

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

Comment 5

[Mark Banner (:standard8)]

This actually happens for any search strings - just open up 2 windows and enter some searches and you'll see the results get put in both windows.

Comment 6

[Mark Banner (:standard8)]

The problem is this line:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsAbLDAPDirectory.cpp&rev=1.26&root=/cvsroot#474

The notify item added is normally used to notify that an item has been added to a directory. In this case it hasn't - its a search result, but as we treat it as if the item has been added, then that causes problem across the different displays.

Now we just have to think of a fix ;-)

Comment 7

[Mark Banner (:standard8)]

Added LDAP to the title to make it clearer.

Comment 8

[Mark Banner (:standard8)]

I've just spent some time looking at fixing this bug, I think what we can do is modify the nsIAbView and nsIAbDirectory interaction so that rather than just blindly putting out results, the nsIAbDirectory is told about the nsIAbView when doing a search, and therefore puts the results straight into the nsIAbView.

We will have to be careful to watch for reference loops etc, but I think its not too hard to do.

Comment 9

[Mark Banner (:standard8)]

Attachment: WIP Patch

Work in progress patch - needs a lot of work, but basically GetChildCards will return NS_AB_ERROR_SEARCH_REQUIRED if a search is required, prompting the caller to call StartSearch with an observer.

Comment 10

[Mark Banner (:standard8)]

Looks like we're going to need something like this (the attached patch) for writeable ldap directories - we need a listener mechanism for the write results and I think adapting the patch would be the best way to go. Adding deps.

Comment 11

[Mark Banner (:standard8)]

Attachment: WIP Patch v2

Unbit-rotted, reworked missing files, got it working again etc. I think there may be a couple of ref count issues though, as SM didn't shut down properly when I tried it just now.

David/Ian could you both take a look at this please, I'd like to get some feedback on what you think of the interfaces especially. Thanks.

Comment 12

[David :Bienvenu]

+    abcard->card = addedCard;
+    NS_IF_ADDREF(abcard->card);

This can be NS_IF_ADDREF(abcard->card = addedCard;

The interfaces look OK, from what I can tell.

There's a printf that should probably go away.

+    nsCOMPtr<nsIAbDirectory> newlist(new nsAbMDBDirProperty);
+    if (!newlist)

does this do a QI on the new nsAbMDBDirProperty? It's not a construction pattern I've seen before.

+    // malloc these from an arena
+    AbCard *abcard = (AbCard *) PR_Calloc(1, sizeof(struct AbCard));

maybe this should say // #TODO: malloc these...

Comment 13

[Mark Banner (:standard8)]

Attachment: WIP Patch v3

Another WIP Patch. This one won't actually fix this bug, but will go a long way towards it.

This rearranges the listeners so that nsAbLDAPDirectoryQuery holds a strong ref to nsAbQueryLDAPMessageListener, but nsAbQueryLDAPMessageListener now holds a weak ref to nsAbLDAPDirectoryQuery. This avoids reference cycle problems.

There's also some fairly major re-arrangment of some of the directory query structures. I'll try and make some more detailed notes on it soon.

The patch doesn't quite work at the moment. Queries on local directories seem to use 100% CPU and never return, queries on LDAP directories don't return anything. It probably just needs a couple of tweaks, but with it posted I can take a look at it when I get time.

Comment 14

[Mark Banner (:standard8)]

Attachment: WIP Patch v4

This fixes the searches that were broken on the previous patch. Will only compile on linux at the moment though as I haven't done the outlook or mac osx changes.

I have also broken the inheritance relationship between nsAbLDAPDirectoryQuery and nsAbLDAPDirectory. It was tending to hide the real interactions between the two classes, and hence felt like it was also getting in the way. I'm also considering if it would be a useful in future to have directories separated from queries such that you request a query interface and you don't care what type of query is returned - you just use it.

Still needs some more work on the directory side to fully fix the interfaces for this bug, but the patch is already too big, so once I've added the outlook and osx changes then this will be the first patch for this bug.

Comment 15

[Mark Banner (:standard8)]

Attachment: Part 1 v5 (checked in)

David, please see previous comments, this doesn't fix this bug, but it goes a long way to getting there. I probably need to rework the DoQuery interface a bit more, but that'll come when I fix the bug fully.

I've checked the outlook changes compile. Could you check the Mac OS ones please?

Comment 16

[David :Bienvenu]

I'm building this now. I'll try running it later this afternoon.

This looks like a really useful refactoring of the code. The inheritance relationship between nsAbLDAPDirectoryQuery and nsAbLDAPDirectory always surprises me.

Comment 17

[David :Bienvenu]

Comment on attachment 278218 [details] [diff] [review]
Part 1 v5 (checked in)

-NS_IMPL_ISUPPORTS_INHERITED2(nsAbOSXDirectory,
+NS_IMPL_ISUPPORTS_INHERITED5(nsAbOSXDirectory,

I had to turn it into INHERITED3, not 5 (which makes sense since you've added one new parent interface)

LDAP AB quick search isn't working for me on the mac, but perhaps that's an other issue? Does it work on windows? I didn't try it before applying this patch, and I understand there was an NSPR problem or something? r=bienvenu, if all that's expected, modulo the INHERITED3 thing

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> (From update of attachment 278218 [details] [diff] [review])
> -NS_IMPL_ISUPPORTS_INHERITED2(nsAbOSXDirectory,
> +NS_IMPL_ISUPPORTS_INHERITED5(nsAbOSXDirectory,
> I had to turn it into INHERITED3, not 5 (which makes sense since you've added
> one new parent interface)

Opps my mistake.

> LDAP AB quick search isn't working for me on the mac, but perhaps that's an
> other issue? Does it work on windows? I didn't try it before applying this
> patch, and I understand there was an NSPR problem or something? r=bienvenu, if
> all that's expected, modulo the INHERITED3 thing

Although I can build on windows, I've only got the capability to test ldap on linux at the moment (where it did work).

The mac problem may be bug 374361 - that was possibly fixed on Sunday (26th) so it may depend on when you pulled your tree last.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 278218 [details] [diff] [review]
Part 1 v5 (checked in)

Requesting sr as I think David's problems are just with Mac OS X and are probably due to the other mac os x bug (see previous comment).

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 278218 [details] [diff] [review]
Part 1 v5 (checked in)

Setting r+ per David's comment 26 as that is believed to be a different bug.

Comment 21

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

I've said it once (6/14/2006) and I'll say it again:  "Mark Banner is my hero" 

Comment 22

[Mark Banner (:standard8)]

Attachment: Fixes crash when clearing an ldap search (checked in)

In implementing the next part of this, I found a crasher.

Clearing a quick search on an LDAP directory releases nsAbView's copy of the directory. So when the search data comes in, the LDAP Query and Directory still exist ok until the end of the search, but then, the RemoveObjectAt(i) drops the directory, which drops the query, which attempts to drop the directory again...

This patch ensures we keep an extra refcount to query until we've finished dropping the listeners, we can then tidy up correctly.

Comment 23

[David :Bienvenu]

Comment on attachment 281038 [details] [diff] [review]
Fixes crash when clearing an ldap search (checked in)

this says to me that something's still not good with the ownership model, if we're relying on listeners to hold a reference to us...

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 281038 [details] [diff] [review]
Fixes crash when clearing an ldap search (checked in)

I've checked this in with an XXX comment (i.e. it may just be temporary), I'll think about the relationships again during the week and see what I can come up with.

Comment 25

[Mark Banner (:standard8)]

(In reply to comment #23)
> (From update of attachment 281038 [details] [diff] [review])
> this says to me that something's still not good with the ownership model, if
> we're relying on listeners to hold a reference to us...

(In reply to comment #24)
> (From update of attachment 281038 [details] [diff] [review])
> I've checked this in with an XXX comment (i.e. it may just be temporary), I'll
> think about the relationships again during the week and see what I can come up
> with.

I should have looked into this ages ago. I think the best thing to do is to let this bug be closed, and when we get round to doing async apis, we'll want to revisit how all this works anyway, so we can deal with it then.