465535 - Cannot delete a contact from a mailing list within the quick search results

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

1) Set up a mailing list with several contacts
2) Do a quick search within the contact list
3) Attempt to delete a card.

Result: Doesn't work

Expected Result: Delete the card.

Vague description of actual problem:

The locally stored prefs id isn't initialised for a nsAbMDBDirectory that is a search query. When we try and keep a reference to the nsIAddrDatabase in nsAbMDBDirectory::DeleteCards, it fails, because we haven't got the prefs, and hence we can't get the nsIAddrDatabase. We can't even get the non-search database, because mailing lists still don't know about the locally stored prefs.

Comment 1

[[:Aureliano Buendía]]

Probably I understand the question but here seems that work fine.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091110 Lightning/1.0pre Shredder/3.0pre ID:20091110032315

I have create a tesing mailing list with 5 cards (new cards for testing) into one address book that contains others cards: it is this the right STR?

Comment 2

[emeadows]

I was able to reproduce the problem using her data on a sandbox system.  Go into a sub-address book (a "list"), search on a name, select the name, click the delete button or right-click > delete, and nothing happens.

Works fine in the main address book, but not in the sub-address books or "lists".

I have a user I support that has hundreds of contacts in a dozens of lists and relied on the search function to find them quickly for deletion or changes.  Now to do deletes, she has to scroll up and down manually to find them, which takes considerable time.  This is impacting her efficiency and is complaining to me to fix it. 

This used to work in Thunderbird 2.x and I just migrated her recently to 7 through the sequential updates.  She was quite surprised to find that it no longer worked and is quite vocal about it... 

I see that this problem is quite old and not assigned to anyone.  I would be very appreciative if you could give this problem some of your time.

Comment 3

[emeadows]

Sorry for my disjointed entry... by "her data" I am referring to the user who is reporting the problem.  Couldn't find a way to edit my entry so I'm making my "correction" by adding this additional comment.(In reply to emeadows from comment #2)
> I was able to reproduce the problem using her data on a sandbox system.  Go
> into a sub-address book (a "list"), search on a name, select the name, click
> the delete button or right-click > delete, and nothing happens.
> 
> Works fine in the main address book, but not in the sub-address books or
> "lists".
> 
> I have a user I support that has hundreds of contacts in a dozens of lists
> and relied on the search function to find them quickly for deletion or
> changes.  Now to do deletes, she has to scroll up and down manually to find
> them, which takes considerable time.  This is impacting her efficiency and
> is complaining to me to fix it. 
> 
> This used to work in Thunderbird 2.x and I just migrated her recently to 7
> through the sequential updates.  She was quite surprised to find that it no
> longer worked and is quite vocal about it... 
> 
> I see that this problem is quite old and not assigned to anyone.  I would be
> very appreciative if you could give this problem some of your time.

Comment 4

[jirousek]

Cannot delete a contact from a mailing LIST within the quick search results.
Works fine in the main address book, but not in the sub-address books or "lists".

Go into a sub-address book (a "list"), search on a name, select the name, click the delete button or right-click > delete, and nothing happens.

Comment 6

[Rainer Bielefeld]

Still REPRODUCIBLE with  English SeaMonkey (unofficial by frg) 2.47a1  (NT 6.1; Win64; x64; rv:50.0)  Gecko/20100101 Firefox/50.0 Build 20160628114904  (Default Classic Theme)  on German WIN7 64bit:

1. Launch Addressbook-Editor via icon in Application Bar (Status Bar)
2. Click arbitrary address Book  → New Lisr (icon) → Name = Testlist → 
   Add 3 email-addresses to list like [e-mail], [e-mail], [e-mail] → [ok]
3. Click Testlist → into quick search bar above contacts pane type "@"
   » Nothing changes, all items contain "@"
4. Click "[e-mail]" → rightclick → Delete → Convirm Warning with [ok]
   Bug: deletion will fail

a) already REPRODUCIBLE with  DE  SeaMonkey 2.0  (Windows NT 6.1; WOW64; 
   rv:1.9.1.4)  Gecko/20091017  Build 20091017081335  (Classic Theme) 
   on German WIN7 64bit
a1) so definitely different to "Bug 1270651 - contacts cannot be deleted when 
    they have been found through a search", what started with SM 2.42 
    (Version 45)
b) <del> key does not work, either.

Comment 7

[Konstantin K]

Still reproducible at changeset 0d0988b6c3a1 on Windows 7

I tracked down the bug to a problem in this part of the mailnews module:
mailnews\addrbook\src\nsAbMDBDirectory.cpp(420):NS_IMETHODIMP nsAbMDBDirectory::DeleteCards(nsIArray *aCards)

When the mIsQueryURI==true branch is taken with a m_IsMailList==true the GetDatabase(...) call on line 431 fails with NS_ERROR_NOT_INITIALIZED

Using GetAbDatabase() instead resolves the issue. However, this has the side effect of initializing the mDatabase member for the lifespan of the search query directory object.
If this is not a problem - see the attached card-delete-fix.diff file for a possible solution.

Otherwise, either of these could be done:
    1. Instead of initilizing mDatabase directly, modify GetAbDatabase() so it accepts a database output parameter like GetDatabase() does.This way it could be used instead of the GetDatabase(...) call on line 431 and limit the lifespan of the database object to the mIsQueryURI scope.
    2. Merge the parent directory database search code from GetAbDatabase() into GetDatabase() when using mail lists. All use places have to be carefully reviewed.

Comment 8

[Konstantin K]

Attachment: A possible solution to the issue

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Redirecting to Magnus - I'm afraid I don't have the cycles to do Thunderbird reviews these days.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957535 [details] [diff] [review]
A possible solution to the issue

I'll take the review.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: ignore-white-space.patch

Here's the same patch ignoring white-space just so we can focus on the changes better.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957792 [details] [diff] [review]
ignore-white-space.patch

Review of attachment 8957792 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +425,5 @@
> +  if (!mDatabase)
> +    rv = GetAbDatabase();
> +
> +  if (!NS_SUCCEEDED(rv) || !mDatabase)
> +    return rv;

Please make put the error checking into a block following 

if (!mDatabase)
  ...

@@ +432,5 @@
>      // if this is a query, delete the cards from the directory (without the query)
>      // before we do the delete, make this directory (which represents the search)
>      // a listener on the database, so that it will get notified when the cards are deleted
>      // after delete, remove this query as a listener.
> +    mDatabase->AddListener(this);

Hmm, looking at nsAbMDBDirectory::GetAbDatabase(), that already attaches a listener. Looks like we're doing it twice here then, no?

@@ +440,5 @@
>          do_GetService(NS_ABMANAGER_CONTRACTID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    nsCOMPtr<nsIAbDirectory> directoryNoQuery;
> +    rv = abManager->GetDirectory(mURINoQuery, getter_AddRefs(directoryNoQuery));

Please revert change of variable name, it creates needless noise.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957535 [details] [diff] [review]
A possible solution to the issue

Review of attachment 8957535 [details] [diff] [review]:
-----------------------------------------------------------------

I've already reviewed the other patch. The patch needs more work, so cancelling the review for now.

To avoid confusion: Yes, we need a patch taking white-space/indentation changes into account, for if you re-indent large blocks, it's sometimes easier to review with a patch that ignores white-space.

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +445,2 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    

Please avoid introducing trailing spaces.

@@ +448,2 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    

And here.

Comment 14

[Konstantin K]

Well, I agree the Add/RemoveListener from the old code is no longer necessary as this will be handled by the GetAbDatabase() and the ~nsAbMDBDirectory() destructor.
This is my first contribution to the community, so I'm sorry if I didn't manage to follow all coding style rules.

You may find attached an updated version with 2 diffs - with and without whitespace.
(if you submit code block re-idents on separate commits)

Feel free to use it in any way you like.
I've run the mozmill interactive test suite, tested by hand in some common sense use cases and all seems fine.

Comment 15

[Konstantin K]

Attachment: card-delete-fix-r2.diff

Comment 16

[Konstantin K]

Attachment: card-delete-fix-r2-ignore-ws.diff

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957835 [details] [diff] [review]
card-delete-fix-r2-ignore-ws.diff

This looks very clear now ;-)

Aceman, what do you think about this?

Comment 18

[:aceman]

Comment on attachment 8957835 [details] [diff] [review]
card-delete-fix-r2-ignore-ws.diff

Review of attachment 8957835 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, this works for me.
But why is the mDatabase change done? In the mIsQueryURI branch the mDatabase isn't used. The problem was that database couldn't be get via GetDatabase but users of that were was removed now. Instead you set up the database listener via GetAbDatabase.

I don't know whether we need the database locally-scoped only, as Konstantin discusses in comment 7.

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +424,5 @@
>  
> +  if (!mDatabase)
> +  {
> +    rv = GetAbDatabase();
> +    if (!NS_SUCCEEDED(rv) || !mDatabase)

NS_FAILED(rv)

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: card-delete-fix-r2.diff - traling spaces removed and NS_FAILED() used

I've removed some trailing spaces which had crept in again and I'm using NS_FAILED() now.

Let's see whether this breaks anything:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=99a4fa8236d46701fe2754000836bd21b31d9b65

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957851 [details] [diff] [review]
card-delete-fix-r2.diff - traling spaces removed and NS_FAILED() used

Thanks again, this didn't break any test and works for the deletion from a mailing list after a search.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d6eb1caf9e2d
Fix deletion of contact from a mailing list within the quick search results. r=jorgk