Closed Bug 1555294 Opened 1 year ago Closed 1 year ago

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

Categories

(Thunderbird :: Address Book, defect)

defect
Not set
normal

Tracking

(thunderbird68 fixed, thunderbird69 fixed)

RESOLVED FIXED
Thunderbird 69.0
Tracking Status
thunderbird68 --- fixed
thunderbird69 --- fixed

People

(Reporter: TbSync, Assigned: TbSync)

Details

Attachments

(1 file, 2 obsolete files)

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: To many onItemRemoved-notifications are send, if the AB contains lists. → Too many onItemRemoved notifications are sent, if the AB contains lists.

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

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?

Not true, testing tricked me.

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.

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?

Attached patch bug1555294.patch (obsolete) — Splinter Review

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.

Attachment #9068912 - Flags: review?(geoff)
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.
Attachment #9068912 - Flags: review?(geoff) → review+
Attachment #9068912 - Attachment is obsolete: true
Assignee: nobody → john.bieling
Status: NEW → ASSIGNED
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.
Attachment #9069299 - Flags: review?(mkmelin+mozilla)
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?
Attachment #9069299 - Flags: review?(mkmelin+mozilla)

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?

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

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

Attachment #9069299 - Attachment is obsolete: true
Attachment #9069827 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9069827 [details] [diff] [review]
1555294-list-member-removed-3.diff

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

LGTM, r=mkmelin
Attachment #9069827 - Flags: review?(mkmelin+mozilla) → review+

Thanks for your help, Geoff!

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.

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

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 69.0
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):
Attachment #9069827 - Flags: approval-comm-beta?
Comment on attachment 9069827 [details] [diff] [review]
1555294-list-member-removed-3.diff

I'll put this into TB 68 beta 2.
Attachment #9069827 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.