462792 - Duplicate cards increase in contacts sidebar in compose window when searching on LDAP directory

Status: NEW

(Thunderbird :: Message Compose Window, defect)

Description

[Takeshi Nishimura]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
Build Identifier: 

Sometimes the contacts list in the contacts sidebar in a compose window has duplicate cards and they inclease by some operations. Re-searching restores the normal state. This issue only occurs in viewing any LDAP directory.

Reproducible: Always

Steps to Reproduce:
Assumed that you have any LDAP directory,
1. Click Write button to open a compose window.
2. View / Contacts Sidebar
3. Select the LDAP directory
4. Input keyword (ex. "@") in "Search For:"
5. Click Write button in main window again (open a compose window w/ contacts sidebar)
6. Input same keyword in "Search For:"
Actual Results:  
Contacts list of the first compose window now has one duplicate card for each person.

Expected Results:  
Contacts list of the first compose window remains unchanged, that is, it should have one card for each person.

When |nsAbView| (which controls result tree in contacts sidebar) shares the |nsIAbDirectory|, this issue occurs, since |nsAbView::OnItemAdded()| will be called for what found in searching but |nsAbView| cannot determine it is called for him or for another view.

On 1.8.1 branch, object sharing occurs when |nsAbView| is created with the same keyword for the first time. Newly created |nsAbView| searches in RDF resources by that keyword, and gets the same object which other |nsAbView|s with same keyword have.
After that, object sharing will not occur because |nsAbView| reuses the object forever.

What is worse, because |nsAbView| does not reuse |nsIAbDirectory| on trunk build, this issue will occur whenever you use the same keyword on any two windows (two of compose windows and address book windows).

Comment 1

[Takeshi Nishimura]

Attachment: Solution 1: Check duplicates in adding card (for 1.8.1 branch)

First solution is to check existing duplicates in |nsAbView::OnItemAdded()|. 
When LDAP search finds any entry and calls |OnItemAdded()|, each view checks its current result tree and adds the entry if no card for that entry is found.
This may change current behavior when LDAP directory originally has duplicate entries, but it will not be a real issue.

Comment 2

[Takeshi Nishimura]

Attachment: Solution 1': Check duplicates for trunk

This patch do same thing for trunk.

Comment 3

[Takeshi Nishimura]

Attachment: Solution 2: Clear result tree in existing contacts sidebars which are in search of same keyword (for trunk)

Another solution is to refresh all contacts list of the same keyword when you start searching any keyword.

This solution is for trunk only. This patch uses existing |mCache| in |nsIAbDirectory|. On branch, various search keywords are assigned to one |nsIAbDirectory| object, so |mCache| cannot be useful.

Comment 4

[Takeshi Nishimura]

On trunk, I can think of additional two solutions. I don't want to do these solutions on branch because |nsIAbDirectory| does not correspond to one search keyword.

Solution 3: 
Beyond solution 2, we can use |mCache| to display results. Of course this causes refresh issue. We cannot reload search results if any window displays the same keyword. We should force to do search again after some constant period.

Solution 4:
Though current |nsAbManager| notifies all listeners when an item is added, this can be changed to restrict notifying listeners. For example, |nsAbView| may register a listener with ID and the listener may be called with ID argument. In this way |nsAbView| can determine if it should do some actions when |OnItemAdded()| is called.

Which solution do you like?

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #0)

Thanks for finding the testcase for this, I was sure there was a way to reproduce still, but couldn't find it when I looked a couple of months ago.

(In reply to comment #4)
> Which solution do you like?

I think you've missed the actual problem. The problem is that nsAbLDAPDirectory::OnSearchFoundCard is calling NotifyDirectoryItemAdded for search results.

It shouldn't be doing that - when you search for an item, it hasn't actually been added to the directory, its just a search result.

I've tried fixing this before, but never got around to finishing it.

I think there are several options here (hence cc'ing some folks who may be interested):

Option one
----------

(Taken from the proposals at https://wiki.mozilla.org/MailNews:Address_Book_Interface_Redesign#nsIAbDirectory)

- Implement nsIAbCollection::searchInstance that returns and nsIAbDirectorySearch (based on the current interfaces).
- Extend nsIAbDirectorySearch::startSearch to take an nsIAbDirSearchListener.
- nsAbView would then use a combination of those functions and listeners to get the cards for that specific address book (it would still need to listen for additions etc post displaying results).
- Make nsIAbDirectory::childCards return an error result for LDAP directories, indicating that a search is required.

Option two
----------

Similar to the first option, but:

- drop nsIAbDirectory::childCards so that all callers who want to get cards must implement the search listener. This would force all callers to be compliant with getting data from remote directories or via async methods (which would complicate some code, but also help to make us more consistent).

Option three
------------

- Replace nsIAbDirectory::childCards with something like nsIAbDirectory::findCards which takes an nsIAbDirSearchListener and passes the cards to that listener.

This would be effectively be a reduction of the first two options, such that you don't have to get an nsIAbDirectorySearch instance before searching. Although we would also need a way to implement stopSearch.


I would be prepared to take patches in stages, i.e. implement the new function(s), then remove childCards later.

I certainly would welcome any thoughts on our options.

Comment 6

[Takeshi Nishimura]

Mark,
You are right. It is important restructure the event mechanism as you mention.
But this is apparent appearance bug especially when you are using the extension "Contacts Sidebar" so I was focusing on fixing this bug also in 1.8.1 branch by minimum modification.
(cf. Contacts Sidebar shows all cards in LDAP by default. - Bug 300892)

Though I don't know this should be fixed by modifying Contacts Sidebar's code as a workaround.

Comment 7

[Takeshi Nishimura]

Attachment: Solution 2(for 1.8.1 branch): clear results in other windows

Previous patch is incomplete. Never reuse |nsIAbDirectory| object in order to correctly refresh the search results. I've decided to choose solution 2 for branch too.

Comment 8

[Takeshi Nishimura]

Attachment: Solution 2(for 1.8.1 branch) - whitespace ignored for code reviews

As per previous patch except not showing whitespace changes.

Comment 9

[Joshua Cranmer [:jcranmer]]

(In reply to comment #5)
> I think there are several options here (hence cc'ing some folks who may be
> interested):
> 
> Option one
> ----------

Personally, I prefer this option.

> Option two
> ----------
> 
> Similar to the first option, but:

Even if we decide to go with this option, option one would be an intermediary step.

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #6)
> Mark,
> You are right. It is important restructure the event mechanism as you mention.
> But this is apparent appearance bug especially when you are using the extension
> "Contacts Sidebar" so I was focusing on fixing this bug also in 1.8.1 branch by
> minimum modification.

Takeshi,
Normally non-trunk branches (e.g. 1.8.1) are closed to patches except for security and stability updates.

It is therefore very rare that fixes of the kind in this bug are accepted to those branches because they do not affect security and stability.

If you think it may fall in or close to that category, and still want to try and get this in on branch, then I suggest we cc some of the relevant drivers and ask them for their opinions before we progress further so that we don't waste time unnecessarily in the case the patch isn't accepted.

Comment 11

[Hunter Perrin]

Hello All,

I recently ran into this bug and spent a few days fixing it. I have a workaround, but I believe I have also found the root of the problem.

If you take a look at this file:
http://people.mozilla.com/~chofmann/l10n/tree/mozilla/mailnews/addrbook/src/nsAbView.cpp
Down in the nsAbView::Init method, there's this line:
rv = rdfService->GetResource(nsDependentCString(aURI), getter_AddRefs(resource));

Now, this bug only occurs when the contact sidebars are started with the same search string. IE, if you open a compose window, and search "a" in it, then another and search "a", you will get duplicates in the first. But, if you open a third and search "b", then search "a", you will not get duplicates. I believe the line above is the root cause. If a search has never been done before in a new window (or the directory was just changed), first, the code will look for resources which share the same URI as the current search. This is a problem, since that will return any resource of a contact sidebar where the first search term was the same as the new one. The code should always create a new resource, and not use the URI.

My temporary fix is to add the following:

In /usr/share/thunderbird/chrome/messenger.jar/content/messenger/addressbook/abCommon.js (on Debian and derivatives):

In the SetAbView() function:

Replace:
  if (gAbView && gCurDirectory == GetSelectedDirectory())
  {

With:
  if (gAbView && gCurDirectory == GetSelectedDirectory())
  {
    if (uri.match(/^moz-abldapdirectory:\/\/.*\?.*$/) ) {
      uri = uri.replace(/\)\)$/, ")(URIRandomizer,=,"+Math.floor(Math.random()*100000001)+"))");
    }

This will cause the search to make each URI unique, but it should not be considered permanent because it is a very sloppy workaround. ;)

I hope I have helped.

Comment 12

[Hunter Perrin]

Is anyone working on this bug?

Comment 13

[IagoSRL]

I have the same problem in Thunderbird 3.0 RC2 (Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.9.1.5) Gecko/20091130 Thunderbird/3.0)

Hunter Perrin, I tested your solution (Comment 11) in my Thunderbird 2.0.0.23 (20090812) and works!
Can you create an extension with your solution to be tested by more users?

I am looking for do the same in Thunderbird 3.0RC2, but this change a lot (the SetAbView function now is defined in the file 'abResultsPane.js' in the same folder but is very different)

Comment 14

[IagoSRL]

Hi again!

I try the solution of Hunter Perrin in TB3RC2 by adding the code 

   if (aURI.match(/^moz-abldapdirectory:\/\/.*\?.*$/) ) {
     aURI = aURI.replace(/\)\)$/, ")(URIRandomizer,=,"+Math.floor(Math.random()*100000001)+"))");
   }

in the line 102, before the code:

  var directory = GetDirectoryFromURI(aURI);

in the file /thunderbird/chrome/messenger.jar/content/messenger/addressbook/abResultsPane.js

Works for me.

Comment 15

[Hunter Perrin]

The root cause of this bug has been identified, and it's ready to be fixed by a Mozilla dev. Are any devs interested? It's a very old (and annoying) bug.