1584211 - Modifying a mailing list in the Edit List dialog is buggy

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Alexander Bergmann]

Modifying a mailing list in the "Edit List" dialog is buggy. For example:

1. Create a few new contacts A <[e-mail]>, B <[e-mail]>, C <[e-mail]>
2. Create a new empty mailing list, e.g. "Test", and add the contacts as members

(The mailing list is correctly created with the three contacts as members.)

3. Right click on the mailing list and select "Edit List"
4. In the "Edit List" dialog remove contacts B and C from the mailing list and click "OK"

Sometimes the changes are not applied at all, i.e. the mailing list still has three members A, B and C. Sometimes the changes are applied partially, i.e. the mailing list has two members A and C.

It gets even worse:

Create a new list with five contacts A, B, C, D and E. Then delete B, C, D and E in the "Edit List" dialog. In the address book window, the mailing list is displayed with still having five members A, B, C, D and E - which is obviously wrong.

And if we create a new message or edit the list again in the "Edit List" dialog, then the mailing list appears to have only three contacts A, C and E - which is obviously wrong.

Finally, after restarting Thunderbird the mailing list has once again five members A, B, C, D and E - which is obviously wrong.

From time to time, I have noticed some other variations, too, but I have never seen a correct result, when deleting multiple members of the mailing list from within the "Edit List" dialog.

Deleting the members of the mailing list from within the address book window itself does work reliably though.

This does affect Thunderbird 60.9.0 and Thunderbird 68.1.1.

Comment 1

[Wayne Mery (:wsmwk)]

duplicate?

Comment 2

[Alexander Bergmann]

It may be related to Bug 1505279 ?

Comment 3

[Eckard Berberich]

I cannot reproduce this behaviour in Thunderbird 60.x.x.
Running macOS 10.14.6

Comment 4

[Jorg K (CEST = GMT+2)]

Can you try this in TB 68.1.1. Also see bug 1505279 comment #6.

Comment 5

[Alexander Bergmann]

Yes, as stated in comment #0 I am able to reproduce the problem in version 68.1.1 as well. I should specify: Using Arch Linux and the official Thunderbird version downloaded from www.thunderbird.net.

I have just downloaded version 68.1.1 again and also created a new profile from scratch. The problem as described in comment #0 is still happening in all kinds of flavors for me - always when using the "Edit List" dialog. (There are no errors or warnings in the Error Console either.)

Comment 6

[Jorg K (CEST = GMT+2)]

Sorry, I can't read every line in the 200 bugmails I get ever day.

Paul, interested in mailing lists. Is this a UI issue or more a backend issue?

Comment 7

[Alexander Bergmann]

I've now also run a quick test with Thunderbird 60.9.0 (64-bit) under Windows 10 Version 1903 Build 18362.356 (64-bit) and the problem is also reproducible for me under Windows.

Comment 8

[Paul Morris [:pmorris]]

Looks like a UI issue, since the report indicates that deleting recipients from a mailing list works if you do it directly in the address book window rather than in the "edit mailing list" dialog.

I've tried to reproduce this on both my 68.1.1 and development builds (Ubuntu Linux) and wasn't able to reproduce it on either one. (I appreciate how the steps to reproduce are very clearly described.) That's as far as I've gotten for now.

Comment 9

[Alexander Bergmann]

Further investigation from me shows, that there is another small thing, which has to be taken into account:

Let us start with a mailing list with five members, i.e. A, B, C, D and E (see screenshot 1). When removing a member from the mailing list in the "Edit List" dialog, there are two ways to do so:

1. Leaving the row empty (see screenshot 2 right before clicking the "OK" button) -> works!
2. Removing the row completely (see screenshot 3 right before clicking the "OK" button) -> does not work!

In the first case, the edited mailing list has three members A, C and E - which is the correct result. In the second case, the edited mailing list has four members A, C, E and E - which is obviously not correct!

The corresponding function is GetListValue() in abMailListDialog.js:
https://dxr.mozilla.org/comm-central/source/comm/mailnews/addrbook/content/abMailListDialog.js#49

I haven't understand the source code in detail. But it looks like at the moment existing elements are only removed, when the row is empty (lines 94 - 108) - like in the first case. What I find odd: At the moment some existing elements seem to get removed from the end of the list (lines 135 - 141) - probably to handle the second case.

All of this only happens, if multiple elements are removed from the list. Removing a single element from the list seems to work.

Comment 10

[Alexander Bergmann]

Attachment: Screenshot 1

The mailing list before any modification

Comment 11

[Alexander Bergmann]

Attachment: Screenshot 2

The mailing list right before clicking the "OK" button -> works!

Comment 12

[Alexander Bergmann]

Attachment: Screenshot 3

The mailing list right before clicking the "OK" button -> does not work!

Comment 13

[Alexander Bergmann]

There is actually another third case:

If you additionally remove the last empty row in the second case as well - see screenshot 4 - then (finally!) there is an error in the Error Console:
NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMutableArray.removeElementAt] abMailListDialog.js:132

The mailing list again has four members A, C, E and E - which is obviously not correct. (Just like in the second case). But now in the address book window the change is not reflected and the mailing list is still shown as having five members A, B, C, D and E - just like before the modification.

Comment 14

[Alexander Bergmann]

Attachment: Screenshot 4

The mailing list right before clicking the "OK" button -> does not work! Triggers error in the Error Console.

Comment 15

[Jorg K (CEST = GMT+2)]

Thanks for all the effort, I hope we can get to the bottom of it.

Comment 16

[Paul Morris [:pmorris]]

Attachment: mailing-list-dialog-bugs-0.patch

I've untangled the code and fixed things.

Thanks for all the insights on tracking down how to trigger these bugs. I didn't reproduce it before because I was deleting using empty rows rather than removing rows.

I found (and fixed) another issue. If you enter more than one address on a line like d, e, and f here:

a
b, d, e, f
c

Then those new addresses were not being added, even though there was code that was supposed to do it.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9098533 [details] [diff] [review]
mailing-list-dialog-bugs-0.patch

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

::: mailnews/addrbook/content/abMailListDialog.js
@@ +75,5 @@
>    mailList.listNickName = document.getElementById("ListNickName").value;
>    mailList.description = document.getElementById("ListDescription").value;
>  
> +  // Gather email inputs, removing any empty lines.
> +  const addressInputs = Array.from(

Please prefer let, here and elsewhere.
But, I don't think you need this temp, you can just tag on the .reduce

@@ +78,5 @@
> +  // Gather email inputs, removing any empty lines.
> +  const addressInputs = Array.from(
> +    document.querySelectorAll(".textbox-addressingWidget"),
> +    element => element.value
> +  ).filter(value => value && !value.match(/^\s*$/));

better to use .test() when not using the results. But it could be easier to just do value => value.trim(). It wouldn't ever be null

@@ +84,5 @@
> +  // Expand input lines into address objects. There can be more than one email
> +  // address on a given input line.
> +  const addressObjects = addressInputs.reduce(
> +    (addressObjects, addressInput) => {
> +      const objects = MailServices.headerParser.makeFromDisplayAddress(

let mailboxes =

Please just pass addressInput as the only param, the rest is not needed. Seems there is old junk around for a few other callers of this too. Can you take care of those too?

@@ +104,2 @@
>  
> +    const cardProperty = addNewEntry

can we fix the naming while here. isn't this just a card?
then please just reuse it, as card = card && card.QueryInterface(Ci.nsIAbCard);

Comment 18

[Alexander Bergmann]

Thank you, Paul, for taking the time to look into this issue! :-)

I am very relieved, that you were finally able to reproduce and - more important - fix(!) the bug(s). As soon as there is a test version available, I would be happy to give it a try and confirm whether or not it works for me as well.

While you are working at mailing lists, I wonder, whether you can also improve the current behavior of Thunderbird for handling mailing list names containing illegal characters. At the moment there is a check for an empty list name and leading and trailing spaces get trimmed. However, there seem to be more restrictions within Thunderbird, that are not handled gracefully, yet.

At the moment, i.e. Thunderbird 68.1.1, I am able to create new mailing lists, that are not usable in the message compose window containing the following characters:

• Bug 170823: Multiple Spaces
• Bug 1175900: Comma(s)
• Semicolon(s)
• Bug 1156245: Multiple Double Quotes
• < and >

I think there should be an error message shown like "list name contains one or more illegal characters", if the user enters a character in the list name, that is not supported by Thunderbird internally. (I wonder, whether there are actually more cases like this?)

Comment 19

[Jorg K (CEST = GMT+2)]

Please paste bugs just as text since BMO will show the appropriate tooltips:
Bug 170823: Multiple Spaces
Bug 1175900: Comma(s)
Bug ???: Semicolon(s)
Bug 1156245: Multiple Double Quotes

Comment 20

[Paul Morris [:pmorris]]

Attachment: mailing-lists-dialog-bugs-1.patch

Thanks for the review.

Please prefer let, here and elsewhere.

As you wish. I'd be curious to hear your rationale for not using const though. If a programmer doesn't intend for a variable to be re-assigned then why wouldn't they use const to express that?

Preferring const over let makes code easier to read and understand. (See https://eslint.org/docs/rules/prefer-const ) It's one reason languages like Rust default to immutable variables. When do you think I should use const instead of let?

But, I don't think you need this temp, you can just tag on the .reduce

Yes, but I wanted to have the temp because it makes for a clearer separation between the two steps of getting the input from the form and then processing it.

Anyway, on second look I found a way to simplify things and avoid the reduce.

better to use .test() when not using the results. But it could be easier to
just do value => value.trim(). It wouldn't ever be null

Ah, makes sense, good call. (The new patch avoids the need to filter.)

Please just pass addressInput as the only param, the rest is not needed.

Done.

Seems there is old junk around for a few other callers of this too. Can you
take care of those too?

I'll do a separate patch for that.

Comment 21

[Paul Morris [:pmorris]]

Try server run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=08d1cd4836df205325acf37c6cffc9471d0cda68

Comment 22

[Paul Morris [:pmorris]]

Alexander, you're welcome. I'm glad I could fix it. Hopefully you'll be able to test it soon. I'll put it on my list to have a look at those other issues with mailing lists that you mentioned.

Comment 23

[Paul Morris [:pmorris]]

Attachment: remove-unneeded-arguments-0.patch

I only found two places that had the extra argument to MailServices.headerParser.makeFromDisplayAddress. This removes them.

Comment 24

[Alexander Bergmann]

I've tried this version on Arch Linux (64-bit):
https://taskcluster-artifacts.net/afvqcBuDR56X4o9qajVb6Q/0/public/build/target.tar.bz2

In my test profile I had switched on the hidden pref mail.debug.test_addresses_sequence while debugging the problem initially. When this hidden pref is enabled, then I get this new error in the Error Console, which I didn't get before:

TypeError: item.querySelector(...) is null addressingWidgetOverlay.js:493:26
    awTestRowSequence chrome://messenger/content/messengercompose/addressingWidgetOverlay.js:493
    awDeleteRow chrome://messenger/content/addressbook/abMailListDialog.js:379
    awDeleteHit chrome://messenger/content/messengercompose/addressingWidgetOverlay.js:641
    awRecipientKeyDown chrome://messenger/content/messengercompose/addressingWidgetOverlay.js:1035
    onkeydown chrome://messenger/content/addressbook/abEditListDialog.xul:1
    goEditListDialog chrome://messenger/content/addressbook/abCommon.js:782
    AbEditSelectedDirectory chrome://messenger/content/addressbook/abCommon.js:245
    doCommand chrome://messenger/content/addressbook/abCommon.js:190
    goDoCommand chrome://global/content/globalOverlay.js:101
    oncommand chrome://messenger/content/addressbook/addressbook.xul:1

I don't know, how serious this is, but the comment in the function awTestRowSequence() describing the use of the hidden pref mail.debug.test_addresses_sequence is as follows:

  /*
    This function is for debug and testing purpose only, normal user should not run it!

    Every time we insert or delete a row, we must be sure we didn't break the ID sequence of
    the addressing widget rows. This function will run a quick test to see if the sequence still ok

    You need to define the pref mail.debug.test_addresses_sequence to true in order to activate it
  */

Other than that, modifying the mailing list works flawlessly now! :-)

Comment 25

[Jorg K (CEST = GMT+2)]

Maybe comment #24 is something we want to address before landing.

Comment 26

[Alexander Bergmann]

Oh no... I found another new problem:

Create a new list with contacts A, B, C, D and E. Deleting all contacts A-E by removing the rows works; deleting all contacts A-E by emptying the rows does not work! A new contact with ",,,,," as display name and primary email address is created instead.

Comment 27

[Paul Morris [:pmorris]]

Attachment: mailing-lists-dialog-bugs-2.patch

Good catch Alexander. This fixes the issue you found when clearing all the rows (resulting in ",,,,"). I just had to reinstate the step of filtering out the empty rows when processing the inputs.

More on that console error in a sec...

Comment 28

[Paul Morris [:pmorris]]

Attachment: prevent-false-positive-errors-0.patch

This fixes the error you were seeing in the console. Turns out it is a false positive. I was able to reproduce it without the other patch applied. It occurs when you delete a row, and so is not directly affected by the code changes in the other patch. To quote my commit message for the patch:

Bug 1584211 - Avoid false positive errors from awTestRowSequence
This debugging function is written to test UIs like the compose
dialog where each row has a drop down menu (to,cc,bcc). Those menus
have an ID attached that has to be kept in sequence. But in the
mailing list dialog the rows have no drop down menus, so this
function was generating false positive errors there. Adjust it
so it works for cases where there is no drop down menu.

With this patch applied I didn't get the error in the console, but instead the "---SEQUENCE OK---" message.

Thanks again Alexander for catching all these buggy edge cases and the clear reports! If I have a chance I'd like to write some tests for this bit of UI to help keep it bug free.

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 9098908 [details] [diff] [review]
mailing-lists-dialog-bugs-2.patch

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

LGTM, r=mkmelin

Comment 30

[Magnus Melin [:mkmelin]]

Re let/const: const isn't really used that much (that way) in Mozilla code. Personally, I can't think of situation where reassigning was causing a problem, so I'd just not want to think about whether its immutable or not. Usually, there is little need to care - it either is or it isn't. If it turns out I did need to change it, then I need to change the declaration but that's extra work.

Comment 31

[Paul Morris [:pmorris]]

Jorg, I think we'll want to backport the main patch but I haven't had a chance to try it on the other branches yet. So feel free to land on trunk and I can work on the others next week. Although, that said, backporting might just work as-is, since it's basically just one function that's changed.

Magnus, thanks for your thoughts on let/const. There's something to be said for not having to think about things that don't make much difference, especially in this case on the writing side of things. Especially on the reading side, I like having the distinction, but I can default to let since that's the convention.

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 9098855 [details] [diff] [review]
remove-unneeded-arguments-0.patch

You missed one:
https://searchfox.org/comm-central/rev/d4ab6480c22e079ed0d38e329591de9d0e64ff94/mail/components/compose/content/addressingWidgetOverlay.js#136

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 9098855 [details] [diff] [review]
remove-unneeded-arguments-0.patch

Let's not use this since the clean-up is already happening here: Attachment 9098729 [details] [diff].

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5462c8d4451a
Fix mailing list edit dialog bugs. r=mkmelin
https://hg.mozilla.org/comm-central/rev/32aa8b8df4f3
Avoid false positive errors from awTestRowSequence. r=mkmelin

Comment 35

[Jorg K (CEST = GMT+2)]

TB 70 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/1f623ce59de43a442e87cd9ea2e2757d40be6e93

The first patch already needed rebasing and the second one was even worse, so I left that.

Comment 36

[Jorg K (CEST = GMT+2)]

TB 68.2.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/5b12fd88dbd91e2874433d9a3076faba850bbd24