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

RESOLVED FIXED in Thunderbird 67.0

Status

defect
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: john.bieling, Assigned: darktrojan)

Tracking

Thunderbird 67.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr6066+ fixed, thunderbird66 fixed, thunderbird67 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Reporter

Description

5 months ago

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.

Assignee

Comment 1

5 months ago

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.

Status: UNCONFIRMED → NEW
Component: Add-Ons: Extensions API → Address Book
Ever confirmed: true
Product: Thunderbird → MailNews Core
Assignee

Comment 2

5 months ago

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.

Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #9039002 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 3

5 months ago

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

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

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

Great, thx! r=mkmelin
Attachment #9039002 - Flags: review?(mkmelin+mozilla) → review+

Comment 5

5 months ago

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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

5 months ago
Target Milestone: --- → Thunderbird 67.0
Reporter

Comment 6

5 months ago

Can this be backported to TB 60, please?

Comment 7

5 months ago

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

Flags: needinfo?(geoff)
Reporter

Comment 9

4 months ago

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?

Reporter

Comment 11

4 months ago

I added two screenshots.

Comment 12

4 months ago

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.

Reporter

Comment 13

4 months ago

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

Comment 14

4 months ago

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

Assignee

Updated

4 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 15

4 months ago

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

Attachment #9042861 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 16

4 months ago

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.

Attachment #9042862 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 17

4 months ago
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.
Attachment #9039002 - Flags: approval-comm-beta?

Comment 18

4 months ago
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.
Attachment #9039002 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #9042861 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9042862 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Updated

4 months ago
Keywords: checkin-needed

Comment 19

4 months ago
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.

Updated

4 months ago
Keywords: checkin-needed
Assignee

Comment 20

4 months ago

Yes, yes I do.

Attachment #9042861 - Attachment is obsolete: true
Attachment #9044520 - Flags: review+
Assignee

Updated

4 months ago
Keywords: checkin-needed

Comment 21

4 months ago

Pushed by mozilla@jorgk.com:
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

Status: REOPENED → RESOLVED
Closed: 5 months ago4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Reporter

Comment 22

4 months ago

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.

Assignee

Comment 23

4 months ago

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…

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.

Reporter

Comment 25

4 months ago

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?

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

Reporter

Comment 27

4 months ago

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 29

4 months ago

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.

Updated

3 months ago
Attachment #9042002 - Flags: approval-comm-esr60? → approval-comm-esr60+

Comment 31

3 months ago

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 :-)

Flags: needinfo?(john.bieling)
Reporter

Comment 32

3 months ago

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.

Flags: needinfo?(john.bieling)
Reporter

Comment 33

3 months ago

I will check bug 1520737 on next Tuesday.

Comment 34

3 months ago

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?

Assignee

Comment 35

3 months ago

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.

Reporter

Comment 36

3 months ago

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.

Assignee

Updated

3 months ago
Blocks: 1534175
Reporter

Comment 37

3 months ago

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
Reporter

Comment 38

3 months ago

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

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

3 months ago

(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 :-(

Reporter

Updated

3 months ago
Blocks: 1534596
Reporter

Updated

3 months ago
Blocks: 1534598
You need to log in before you can comment on or make changes to this bug.