Closed Bug 1555294 Opened 6 years ago Closed 6 years 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: 6 years 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.

Attachment

General

Created:
Updated:
Size: