1555294 - Too many onItemRemoved notifications are sent, if the AB contains lists.

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[John Bieling (:TbSync)]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.169 Safari/537.36

Steps to reproduce:

Add a contact to an empty addressbook
Add 2 lists to the same addressbook, but keep the lists empty (no members)
Delete the just created contact again

Actual results:

The onItemRemoved AB listener gets notified 3 times:

• 1x for the original contact
• 1x for each list (aItem being the contact and aParentDir being the list with .isMailList = true) even though the contact was not part of that list.

Expected results:

The onItemRemoved AB listener should only get notified for the true deletion in the addressbook and for the removal from a list, if he really was part of a list.

Alternativly, a new addrbook-list-member-removed observer would solve this.

Comment 1

[Geoff Lankow (:darktrojan)]

This seems to be the culprit. It needs some sort of check that aCard is a member of this, but that may not be straightforward.

https://searchfox.org/comm-central/rev/2ddfe0d5bd910e7e7bdbdebb5b826b4863dfd880/mailnews/addrbook/src/nsAbMDBDirectory.cpp#768

Comment 2

[John Bieling (:TbSync)]

I traced it into here:
https://searchfox.org/comm-central/source/mailnews/addrbook/src/nsAddrDatabase.cpp#1509

If I comment out that line, I do not get any notifications from the MLs. So it must be something inside DeleteCardFromAllMailLists.

Or is that the wrong path?

Comment 3

[John Bieling (:TbSync)]

Not true, testing tricked me.

Comment 4

[Geoff Lankow (:darktrojan)]

We can't check a mailing list to see if the card is in it, because it isn't. The card has already been removed. That's a problem in my plan to fix this bug.

Comment 5

[John Bieling (:TbSync)]

The main reason I need this fixed, is to be able to learn if a member has been removed from a list. That is currently not possible.

How about adding a new dedicated observer notification, like addrbook-list-member-removed, which could be send for each ML the card is part of, before the card is actually deleted? Would that be possible?

Comment 6

[John Bieling (:TbSync)]

Attachment: bug1555294.patch

I tried :-)

I did move the call to DeleteCardFromAllMailList a bit upwards, so I can use the nsIAbCard (it gets reset later on).

If that is not possible, than I would just pass on the UID of the card include that in the observer notification, instead of the card itself.

Comment 7

[Geoff Lankow (:darktrojan)]

Comment on attachment 9068912 [details] [diff] [review]
bug1555294.patch

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

I like the approach, we added the other observer notifications to avoid interfering with the existing listeners, so this goes well with that. I did find a problem when testing, so I'll upload my fixed version, with the test.

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ +1465,5 @@
> +              if (observerService) {
> +                nsCOMPtr<nsIAbCard> listcard;
> +                CreateABListCard(pListRow, getter_AddRefs(listcard));
> +                nsAutoCString listUID;
> +                listcard->GetUID(listUID);

Getting the UID in this way seems a bit odd, and also it crashes in testing.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1555294-list-member-removed-2.diff

Comment 9

[Geoff Lankow (:darktrojan)]

Comment on attachment 9069299 [details] [diff] [review]
1555294-list-member-removed-2.diff

I'm not sure I'm allowed to be the sole reviewer for stuff here, and besides it's C++, which I'm never quite sure about.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9069299 [details] [diff] [review]
1555294-list-member-removed-2.diff

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

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ +1440,4 @@
>    if (!m_mdbEnv) return;
>  
> +  mdb_id cardRowID;
> +  aCard->GetPropertyAsUint32(kRowIDProperty, &cardRowID);

should rv = 
... and check the rv

@@ +1535,5 @@
>  }
>  
>  nsresult nsAddrDatabase::DeleteCardFromListRow(nsIMdbRow *pListRow,
> +                                               mdb_id cardRowID,
> +                                               bool *cardFound = NULL) {

it's nullptr nowadays, no?

but it's a bool, so you shouldn't set it to null! just set it to false

@@ +1540,5 @@
>    NS_ENSURE_ARG_POINTER(pListRow);
>    if (!m_mdbStore || !m_mdbEnv) return NS_ERROR_NULL_POINTER;
>  
>    nsresult err = NS_OK;
> +  if (cardFound != NULL) *cardFound = false;

... and then this is also not needed

@@ +1556,5 @@
>  
>      err = GetIntColumn(pListRow, listAddressColumnToken, (uint32_t *)&rowID, 0);
>  
>      if (cardRowID == rowID) {
> +      if (cardFound != NULL) *cardFound = true;

please just check truthy/falsy

::: mailnews/addrbook/src/nsAddrDatabase.h
@@ +388,5 @@
>    nsresult CreateCard(nsIMdbRow *cardRow, mdb_id listRowID, nsIAbCard **result);
>    nsresult CreateCardFromDeletedCardsTable(nsIMdbRow *cardRow, mdb_id listRowID,
>                                             nsIAbCard **result);
> +  nsresult DeleteCardFromListRow(nsIMdbRow *pListRow, mdb_id cardRowID, bool *cardFound);
> +  void DeleteCardFromAllMailLists(nsIAbCard* aCard);

star to the right (for now), or lint would not have it

::: mailnews/addrbook/test/unit/test_notifications.js
@@ +203,5 @@
> +  abObserver.maxResults = 1;
> +  abObserver.result = [];
> +
> +  cardsToDelete = Cc["@mozilla.org/array;1"]
> +                    .createInstance(Ci.nsIMutableArray);

can't you just clear the original array?

Comment 11

[John Bieling (:TbSync)]

Regarding the *cardFound bool, it is an optional parameter, so it might not be present, can I still assign a value to it, without causing a crash? That is why I checked for NULL and did not touch it, if it is NULL.

Are we using C++ 11?

Geoff, do you handle this?

Comment 12

[Magnus Melin [:mkmelin]]

It's never null, since you assign it a value in the method signature. That's the
bool *cardFound = false) part

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1555294-list-member-removed-3.diff

I finally have a working clang-format again, and it didn't like my formatting!

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9069827 [details] [diff] [review]
1555294-list-member-removed-3.diff

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

LGTM, r=mkmelin

Comment 15

[John Bieling (:TbSync)]

Thanks for your help, Geoff!

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069827 [details] [diff] [review]
1555294-list-member-removed-3.diff

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

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ +1501,5 @@
>    err = m_mdbStore->GetRow(m_mdbEnv, &rowOid, &pCardRow);
>    NS_ENSURE_SUCCESS(err, err);
>    if (!pCardRow) return NS_OK;
>  
> +  // delete the person card from all mailing list

Please use full English sentences in your comments if possible, so starting with upper case and ending with a full stop. I'll fix the two "offending" new comments for now.

Comment 17

[Jorg K (CEST = GMT+2)]

Bonus points for apparently having run the clang-formatter before submitting the patch !

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0f025fb08630
Adding addrbook-list-member-removed observer notification (test by darktrojan). r=mkmelin

Comment 19

[John Bieling (:TbSync)]

Comment on attachment 9069827 [details] [diff] [review]
1555294-list-member-removed-3.diff

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069827 [details] [diff] [review]
1555294-list-member-removed-3.diff

I'll put this into TB 68 beta 2.

Comment 21

[Jorg K (CEST = GMT+2)]

TB 68 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/dd314c2df68a5c00f363b4a8a0b21961b046cea1