1522453 - Not all members of mailing lists are displayed after restart of TB

Status: RESOLVED FIXED

(MailNews Core :: 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/71.0.3578.98 Safari/537.36

Steps to reproduce:

I used the Geoffs adressbook example webextension to create 5 contacts, 4 with emails 1 without. I also created a mailing list and added the 5 contacts in the following order:

User1WithEmail
User2WithEmail
UserWithoutEmail
User3WithEmail
User4WithEmail

Actual results:

The WebExtension can handle this just fine at first, all members are displayed inside the webextension. After restarting TB, only the first 2 members are displayed.

Expected results:

All members should be displayed. Furthermore, members should not be deleted without users consent.

MORE DETAILS:

The main TB address book is also affected. After the members have been added (before the restart) and I open the main TB address book and select the list, only the first two entries are displayed. If I EDIT the list, all 5 entries are displayed. After the restart also the edit dialog only shows two entries.

I encountered this issue while playing around with mailing lists. I use the example webextension in this report, because it allows to reproduce this issue very easy.

MY OWN FINDINGS:

While encountering this issue in my own AddOn, I saw that the childCards enumarator of the list directory throws on hasMoreElements() if the next card has no email. In my own AddOn I therefore switched to use the addressLists nsIMutableArray to loop over the members.

STATEMENT:

I know that the main use of mailing lists in TB is to group members with email and that the TB address book itself does not allow to add members without emails. But the list is a normal directory and there is no real reason to not allow members without email.

I run into this while working on TbSync and syncing iCloud contact groups into TB mailing lists: iCloud contact groups can include members without email.

Comment 1

[Geoff Lankow (:darktrojan)]

I can confirm, but this is a problem with the core code not the API. It doesn't make sense for a mailing list to have members without email addresses, but it certainly shouldn't throw NS_ERROR_FAILURE when it finds one.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1522453-maillist-iterator-1.diff

This doesn't prevent addressLists from containing cards without email addresses until a restart (as it's a memory-only list), nor does it stop that from happening, but it does prevent the childCards enumerator from failing if there's a card without an address.

Comment 3

[John Bieling (:TbSync)]

I am happy to test this as soon as it lands in daily!

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9039002 [details] [diff] [review]
1522453-maillist-iterator-1.diff

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

Great, thx! r=mkmelin

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/aea0a726ce6b
Prevent mailing list member iteration from failing if a member has no email address. r=mkmelin

Comment 6

[John Bieling (:TbSync)]

Can this be backported to TB 60, please?

Comment 7

[Jorg K (CEST = GMT+2)]

We'd have to prepare a patch that applies to TB 60 ESR.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1522453-maillist-iterator-esr-1.diff

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8a2899d4937d4813433ee2858c7bd0385b3e7281

Comment 9

[John Bieling (:TbSync)]

Before you actually backport this: Did it land in Daily already? I tried with:

67.0a1 (2019-02-09) (64-bit)

and the behaviour is somewhat strange:

I again added user1, user2, userWithOutEmail, user3, user4 to a list.
The results pane of the address book (list is selected) shows user 1-4, but not userWithoutEmail
The list edit dialog shows all

After the restart, the results pane still shows user 1-4, but not userWithoutEmail
The list edit dialog only shows user 1&2.

So it looks like the data is there, but the list edit dialog somehow does not get it?

Is there any chance, we can get userWithoutEmail also shown in the results pane?

Comment 10

[John Bieling (:TbSync)]

Attachment: Situation before restart of TB

Comment 11

[John Bieling (:TbSync)]

Attachment: Situation after restart

I added two screenshots.

Comment 12

[Jorg K (CEST = GMT+2)]

Yes, it landed on Daily, see comment #5. Sadly no one requested beta uplift, so it didn't get included in the beta we're preparing at time of writing. It would need to go into a later TB 66 beta to be included in TB 60.6 in March.

Comment 13

[John Bieling (:TbSync)]

Ok, but that means that I indeed tested the proposed fix with my Daily and it still fails, right?

Comment 14

[Jorg K (CEST = GMT+2)]

Absolutely. So no harm done in the missing backports/uplifts.

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1522453-addrbook-reload-1.diff

There's no way to simulate a restart in the address book. So I made one.

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 1522453-maillist-noemail-1.diff

I think preventing contacts without email addresses being added to mailing lists is the best thing to do here. It reflects the current reality – mailing lists are really email address lists – and while we might want that to change, this isn't the time.

This patch doesn't fix existing lists with email-less members, as what's causing the problem is a failure when adding the member. A count of members is kept, and that is broken by adding an email-less member. The broken count prevents iterating over all the members, so we can't even correct it.

Comment 17

[Geoff Lankow (:darktrojan)]

Comment on attachment 9039002 [details] [diff] [review]
1522453-maillist-iterator-1.diff

This can go on beta. It's not wrong, it's just not complete.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9039002 [details] [diff] [review]
1522453-maillist-iterator-1.diff

OK, but we'll uplift it together with the other parts, I assume.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 9042861 [details] [diff] [review]
1522453-addrbook-reload-1.diff

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

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +137,5 @@
>    nsresult rv = observerService->AddObserver(this, "profile-do-change", false);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = observerService->AddObserver(this, "addrbook-reload", false);
> +  NS_ENSURE_SUCCESS(rv, rv);

Don't you have to unregister it? Check profile-do-change in the file.

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: 1522453-addrbook-reload-2.diff

Yes, yes I do.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4e9848c3a12e
Allow tests to reload the address book with a notification. r=mkmelin,jorgk
https://hg.mozilla.org/comm-central/rev/ecb7722a8a77
Prevent cards without an email address being added to a mailing list. r=mkmelin

Comment 22

[John Bieling (:TbSync)]

Thanks for your work. Not being able to store contacts without email address at all now is of course not what I had in mind, when I created this bug report.

Do I have to re-file this as a feature request now? How? We need this if we want to support more Google features as stated in Magnus tasks email.

Comment 23

[Geoff Lankow (:darktrojan)]

Yes, please do. The reason I made no attempt to fix that here is because an unknown number of things rely on an address list being a list of addresses. Given how much needed to be done just to fix the original implementation I don't have a lot of confidence that trying to do something new will go smoothly…

Comment 24

[Magnus Melin [:mkmelin]]

But this is only preventing contacts without email address from being added to our mailing list structure. Contacts as such still don't need to have an email.

Comment 25

[John Bieling (:TbSync)]

That is true, but the way „lists“ are shown in Thunderbird is very similar to the „group“ feature of Google, iCloud and so on. Currently gContactSync and TbSync sync theses groups as lists into Thunderbird, because we have no other option. We currently add dummy emails to contacts part of groups but without email. This is of course ugly.

But even besides these syncing issues, many users I have contact with want to use the Thunderbird lists to group the address book and the underlying implementation looks like that should work (lists are directories). Before I created this bug, this „almost“ worked. Now not at all.

How to I file a feature request?

Comment 26

[Magnus Melin [:mkmelin]]

I see, thanks for the explanation.
We really should support adding tags to our contacts instead (bug 75711). I'm not sure it's worth allowing an API to ride along mailing lists this way without proper backend support.

A feature request is just a bug with severity=enhancement

Comment 27

[John Bieling (:TbSync)]

I added the tagging feature with my CategoryManager AddOn, which allows to create overlapping groups using the category property. However this is not identical to the above group feature. Specifically the carddav / vCard standard has an Implementation for both and a server may send real groups or category tags.

The carddav API of Google sends real groups. gContactSync is using the google contact API (which is different from Google carddav API) and that is also sending real groups. Exchange Active Sync is sending category tags. OwnCloud is sending category tags.

This is all so complicated, but I would suggest to pimp the Thunderbird lists to support any kind of contact as that is closer to the server implementation. For category tags the CategoryManager could be used.

Comment 28

[Jorg K (CEST = GMT+2)]

TB 66 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/f3b778e7c52ffb688f3e3a574fa0ee9347ba2f4a
https://hg.mozilla.org/releases/comm-beta/rev/89ebd1b371c673db7506a7ab80df43549629f6c2
https://hg.mozilla.org/releases/comm-beta/rev/415d630ef78a7911889452dd8350d92fb6e457c3

Comment 29

[Jorg K (CEST = GMT+2)]

TB 66 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/571b2d4921ab9572ea2b9d695e7325a489762432
The new import syntax doesn't work on mozilla66 and will also not work on ESR 60.

Comment 30

[Jorg K (CEST = GMT+2)]

TB 60.6 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/361e33494b4d407f0868f7907b73297cf70dabbb
https://hg.mozilla.org/releases/comm-esr60/rev/82a60dbc0d9521e028dec410908ddb882a8066ce
https://hg.mozilla.org/releases/comm-esr60/rev/710e2086db86f72a57a822235ab6d3a4cb055f6a
(no import needed fixing on ESR in last part)

Comment 31

[Jorg K (CEST = GMT+2)]

John, you know how to pick a build/an installer from
https://treeherder.mozilla.org/#/jobs?repo=comm-esr60&revision=62a9df6d2883f2525162b25acd74d08b4d1d2183
?

If not, here are some Windows installers:
32bit: https://queue.taskcluster.net/v1/task/JP7J9qiWTA2IcHiLNttJOw/runs/0/artifacts/public/build/install/sea/target.installer.exe
64 bit: https://queue.taskcluster.net/v1/task/LeqVtkqAReO-QNeVRT1jUA/runs/0/artifacts/public/build/install/sea/target.installer.exe

Can you give them a whirl and make sure they work as expected. That goes for bug 1520737, too. Both are a preview version of TB 60.6 and should be safe to use :-)

Comment 32

[John Bieling (:TbSync)]

Hi, I installed the 64bit version and the patch is working as intended by Geoff:

• contacts without email can no longer be added to a list
• the list is no longer doing strange things

However, if a user adds a contact with email and later removes just the email (which results in a contact without email part of a list), we get the following behaviour: Every contact after the contact without email gets removed from the list after a restart.

I know that you do not want to spend more time on this, but I think this needs a different fix.

a) return to a plain list of emails, not connected to actual contacts (please do not)
b) store lists as a true directories with no restrictions to the contacts properties and using the UID as hook, not the email
- maybe add an indicator to the list icon, if it contains contacts without email
- on list expansion (when writing to a list) just exclude contacts without email

I currently spend all my time on my addons to get them ready for TB68 but I am also trying to get used to working with the thunderbird code. In 2-3 month I may be able to help Geoff with option b) if you decide to go that way.

Comment 33

[John Bieling (:TbSync)]

I will check bug 1520737 on next Tuesday.

Comment 34

[Jorg K (CEST = GMT+2)]

So this is a net improvement worth shipping in the ESR? "Every contact after the contact without email gets removed from the list after a restart" doesn't sound like fun though :-( - Can that be fixed?

Comment 35

[Geoff Lankow (:darktrojan)]

I can't reproduce exactly what John's seeing, but I do see a similar thing: if a list contains a contact with no email, no further contacts can be added. I think it should be possible to fix that.

Comment 36

[John Bieling (:TbSync)]

I think my test also involved editing the list afterwards, like removing the last member from the List and after a restart all members after the member without email got removed. I am currently away from my computer, I will try to make a detailed description tonight or latest next Tuesday.

Comment 37

[John Bieling (:TbSync)]

With 1534175 fixed, I also do not see https://bugzilla.mozilla.org/show_bug.cgi?id=1522453#c32 anymore, but two (severe) new issues.

Issue #1: Cannot remove any contact from list if all members have email, reproducable as follows:

• new list
• add 6 contacts with email via drag n drop
• try to remove a contact from the list using the list edit dialog
  -> nothing happens
• Restart
• try to remove a contact from the list using the list edit dialog
  -> nothing happens

In both cases it works by hitting the "DEL" key in the address book if the list is selected.

Issue #2: Can only remove last contact from list, if at least one email less contact in list, reproducable as follows:

• new list
• add 6 contacts with email via drag n drop
• Restart
• remove email from second contact in list
• Restart
• remove any contact but the last one from list using list edit dialog and close edit list dialog

  • the last(!) contact is removed from list shown in address book

  • list edit dialog still shows all contacts

• Restart
  -> edit dialog now also shows the (wrong/last) contact removed

Comment 38

[John Bieling (:TbSync)]

The above was tested with Daily 67.0a1 (2019-03-11) (64-bit)

Comment 39

[Magnus Melin [:mkmelin]]

Could you please file new bugs for those? Bug numbers are cheap and it's only a few clicks away (try "Clone This Bug" below)

Comment 40

[Jorg K (CEST = GMT+2)]

(In reply to john.bieling from comment #37)

Issue #1: Cannot remove any contact from list if all members have email, reproducable as follows:

Hmm, so no good to ship this fix :-(