Closed Bug 1362130 Opened 4 years ago Closed 4 years ago

Cannot send message to mailing list when mailing list is added via contacts sidebar and contains certain special characters causing quoting

Categories

(Thunderbird :: Message Compose Window, defect)

52 Branch
defect
Not set
normal

Tracking

(thunderbird_esr5254+ fixed, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr52 54+ fixed
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: celarsen, Assigned: aceman)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

2.40 KB, patch
jcranmer
: review+
jorgk-bmo
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.96 Safari/537.36

Steps to reproduce:

This problem which was resolved in version 51 has now come back with version 52.0 and 52.1.  From incoming message I can press "Forward" button.  The send button will not function when using a distribution list.


Actual results:

If I add an additional email address to the forwarded email list, then the SEND button will become active.  When sending the email will be forwarded to the single email address as well as the distribution list.  Same thing happens when creating a new message.


Expected results:

I should not have to add a single non distribution list address in order to forward to a distribution list.  Same thing happens if I click "Reply" and replace the reply to address with a distribution list.
Does this happen when you have all add-ons disabled? - See Help menu.
Yes, I disabled all add-ons and there was no change.  I remember a previous version of the program had this same problem but I don't remember which version that was.  It was fixed but now the problem is back in version 52.1.  I tried rolling back to 52.0 but that didn't change the problem.
Does it also happen when you compose a clean new email?
Yes, if I compose new email, forward or even if I hit 'reply' then replace the recipient with a distribution list, the 'send' button is not highlighted and does not work.  Only if I have at least one non-distribution list email in the addresses to send to will the 'send' button be enabled.
Can you please attach a screenshot of the mailinglist showing in the TB addressbook? Of course you can censor all the other contacts in the image. But we'd like to see the addressbook type and icon.
Can you try this on a new profile. That should be very easy, since you have a Gmail account. You can just configure the same account after starting TB with -p and creating a new profile.
I tried it in a new profile and while doing that I discovered a quirk in this bug.  I normally have the address book displayed in the "Contact Sidebar".  If I select the distribution list from the contact sidebar (right click and select "TO" or "BCC", the situation I have reported is true.  However, If I simply begin typing the name of the distribution list in the "to" field in the email itself and select it from the dropdown list that appears, the SEND button highlights and I can send the message.  So, the bug appears to be only when the distribution list is selected from the contact sidebar.  It worked the same way when using a new profile.
Sorry, I can't reproduce this, neither in TB 55 Daily nor TB 52 ESR. That's on Windows 7. Whenever I drag a mailing list to the To field or use the "Add to To field" the Send button becomes activated. Richard, is there a problem on Windows 10? Aceman, do you see this on Linux?
Flags: needinfo?(richard.marti)
No problem here with TB 52 and 55. Tried macOS too with both versions. The system AB groups works with tb 55 too the same way.
Flags: needinfo?(richard.marti)
I think I have found what is going on.  If I manually type the list into the "TO" field I get this:

.FWD from Bob <.FWD from Bob>

However, if I right click from the contacts sidebar I get this:

".FWD from Bob" <".FWD from Bob">

If I manually remove the quote symbols, it works.  So, by right clicking on the distribution list from the contact sidebar the quote symbols are being automatically added which causes the send button to not recognize a valid distribution list.
I can't reproduce either with the contacts sidebar.
However, we had this in bug 863231 long ago.
A dot and spaces in the list name?
So what exactly is the list name, nickname and description?
Name = |.FWD from Bob| ?
Summary: Cannot send message to distribution list → Cannot send message to mailing list when mailing list is added via contacts sidebar and contains certain special characters causing quoting
OK, a list name of |.test| causes the failure, it gets placed as ".test" <.test>. So there you go.
So apparently we need to remove the quotes :)

Then I think this didn't work since TB 23, but you didn't have a ML with a dot or didn't use the exact steps. Typing the dot name directly does not produce the error as no quotes are added. I'm not sure why we have 2 different ways to encode the ML name, but those are probably the mime library games ;)
Assignee: nobody → acelists
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Apparently it doesn't like the period in front of the list name ".FWD".  I have always had that there so that the list will appear at the top of the addresses.  If I remove the period and make it "FWD" then it works.  It used to work ok but some change in the program doesn't like the '.'
Attached patch patch (obsolete) — Splinter Review
This should do it.
celarsen, thanks for finding out what is the key difference in your use case.
Attachment #8865150 - Flags: review?(jorgk)
Comment on attachment 8865150 [details] [diff] [review]
patch

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

Have you tested the case where the mailing list name already contains quotes?
Heh no, that doesn't work now :(
Comment on attachment 8865150 [details] [diff] [review]
patch

Clearing my review queue ;-)
Attachment #8865150 - Flags: review?(jorgk)
Thanks to all that helped me with this.  I find that if I replace the period with an underline character I get what I originally wanted (distribution lists sorted together at the top) and they work.
(In reply to Jorg K (GMT+2) from comment #18)
> Have you tested the case where the mailing list name already contains quotes?

If you mean a list with name like "name" (surrounded by quotes that are part of the name), such a list can be created but in my tests it can't be sent to. It may be some of the sending code also thinks to remove the quotes and then just name isn't found as a valid list name. So maybe we don't need to bother with that.

My patch also has a problem with names containing quotes inside the name, like na"me. That gets encoded as "na\"me" in the compose display and I do not remove the slash so the string does not match a ML name.
So just stripping the quotes isn't enough, I'll check some decoding function.
Attached patch patch v2Splinter Review
I've discussed this with jcranmer and it seems MimeParser.parseHeaderField() is a function that decodes the list name properly. Handles the names with dots and quotes inside name. Quotes outside name are removed so the name will not match and Send button will not get enabled. But as I said, such list name will also get rejected if user pressed Send. So we now decode similarly to the way the send code will.
Attachment #8865150 - Attachment is obsolete: true
Attachment #8867495 - Flags: review?(jorgk)
Attachment #8867495 - Flags: review?(Pidgeot18)
Comment on attachment 8867495 [details] [diff] [review]
patch v2

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3206,3 @@
>          (isValidAddress(inputValue) ||
> +         // a valid mailing list name in some or our addressbooks
> +         ((listNames = MimeParser.parseHeaderField(inputValue, MimeParser.HEADER_ADDRESS)) &&

I think the right function to use is
MailServices.headerParser.parseEncodedHeader() as can be seen at line 2633 of the same file.

Or would that be a performance issue since it runs on every keystroke?
nsIMsgHeaderParser::parseDecodedHeader() was one of jcranmer's suggestions, but I understood MimeParser.parseHeaderField was the better option.
Hmm, sure Joshua would have suggested the "official" interface parseDecodedHeader(). What do you mean by "one of [his] suggestions"? Did he suggest more? So how did you come to the conclusion that the "internal" function was the better option? It's faster, surely, but you can see that only tests include mimeParser.jsm.
Comment on attachment 8867495 [details] [diff] [review]
patch v2

OK, apparently this was said on IRC:
17:35	jcranmer	nsIMsgHeaderParser::parseDecodedHeader() should work
17:35	jcranmer	or you can use MimeParser.parseHeaderField directly

So I'm fine with the latter ;-)
Attachment #8867495 - Flags: review?(jorgk) → review+
Comment on attachment 8867495 [details] [diff] [review]
patch v2

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

I'm reminded how much I hate the way the compose window handles header fields. Still, when I last touched this, Josiah was planning on rewriting it to not be so sucky... :-(

I haven't tested this, but I will comment that Jorg's assertion of using parseEncodedHeader is not the correct equivalent--we don't want to be doing RFC 2047 decoding here (which is the main distinction between parseEncodedHeader and parseDecodedHeader).
Attachment #8867495 - Flags: review?(Pidgeot18) → review+
Thanks.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8867495 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): bug 1290733, bug 1296535
Attachment #8867495 - Flags: approval-comm-esr52?
Attachment #8867495 - Flags: approval-comm-beta+
Attachment #8867495 - Flags: approval-comm-esr52? → approval-comm-esr52+
You need to log in before you can comment on or make changes to this bug.