The default bug view has changed. See this FAQ.

Since jsmime.js, no longer possible to use Disposition-Notification-To in mail.compose.other.header

RESOLVED FIXED in Thunderbird 50.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: Jonathan Kamens, Assigned: Jorg K (GMT+1))

Tracking

(Blocks: 1 bug, {regression})

Trunk
Thunderbird 50.0
regression

Thunderbird Tracking Flags

(thunderbird47 wontfix, thunderbird48 wontfix, thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr4548+ fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8722312 [details] [diff] [review]
allow-strings-in-address-fields.patch

A long, long time ago, in a galaxy before jsmime.js was integrated into the mail composition code path, I provided these instructions to the users of my Send Later add-on for how to add a Disposition-Notification-To header manually to outgoing messages:

1. Open the Thunderbird options dialog with Tools > Options… or Edit > Preferences…
2. Click “Advanced” and then “Config Editor…” and click the “I’ll be careful, I promise!” button if it asks you to.
3. Enter “mail.compose.other.header” in the filter box.
4. Double-click on the mail.compose.other.header setting and set it to “Disposition-Notification-To”. If it already has a non-empty value, add a comma and then “Disposition-Notification-To” to the end of it.
5. Quit from and restart Thunderbird.
6. When you are composing a message for which you want a return receipt and which you want to schedule to be sent later, then click on an empty line in the message header in the compose window, select “Disposition-Notification-To” in the drop-down, and then enter your email address as the value of the header.

This worked just fine. Now, it does not. When you follow step 6 above and then try to save the resulting draft, you get this:

JavaScript error: resource:///modules/jsmime.jsm -> resource:///modules/jsmime/jsmime.js, line 3074: TypeError: invalid 'in' operand addr

I don't know if the attached patch is the _correct_ fix to this problem (in fact, I'd wager good money it isn't), but it works, and at least points to the source of the problem.

I suspect bug 1180209 may be related to this.
(Assignee)

Updated

a year ago
Blocks: 1180209
(Assignee)

Comment 1

a year ago
I'm not a jsmime expert but I have certainly hacked on it on various occasions.

Your patch looks inoffensive, but you don't request review from Joshua Cranmer or Kent James, nothing will happen to it.

Please note the format of the commit message:
Bug 1250376 - Allow strings in address fields in draft messages. r=rkent (or r=jcranmer)

Jsmime is new code and there are many tests for it in mailnews/mime/jsmime/test. Perhaps you can add a test to the appropriate file. To run the (modified) test, do this:
mozilla/mach xpcshell-test mailnews/mime
or
mozilla/mach xpcshell-test mailnews/mime/<name of test>
for example:
mozilla/mach xpcshell-test mailnews/mime/test_header_emitter.js
(Reporter)

Comment 2

a year ago
I did not submit my patch for review because I think it is unlikely that it is the best solution to the problem; I included it merely to document the extent to which I was able to localize the problem when I researched it.

Similarly, I did not submit my patch for review because I knew that if I did, somebody would respond with, "We can't accept this patch without a test," and I don't have time to write a test.

As for, "if you don't request review... nothing will happen to it," well I do not have time or space in my life to cope with the brokenness of the Thunderbird development ecosystem right now. I contributed what I was able to this bug; if that's not good enough, c'est la vie.
(Assignee)

Comment 3

8 months ago
Comment on attachment 8722312 [details] [diff] [review]
allow-strings-in-address-fields.patch

Kent, should we take this?

I'd do it the other way around, so the "normal" case comes first:

-   if ("email" in addr) {
+   if ([extra test] && "email" in addr) {
      this.addAddress(addr);
+   } else if (typeof addr == "string") {
+     this.addAddress({email: addr});

[extra test]: How can I check that "in" can be applied to "addr"?

If we take it, I can write a test.
Attachment #8722312 - Flags: feedback?(rkent)
(Assignee)

Comment 4

8 months ago
(In reply to Jonathan Kamens from comment #0)
> I don't know if the attached patch is the _correct_ fix to this problem (in
> fact, I'd wager good money it isn't), but it works, and at least points to
> the source of the problem.
Or maybe that should be fixed higher up the call chain ;-)
I'll do some debugging to get the JS stack and see.
(Assignee)

Updated

8 months ago
Attachment #8722312 - Flags: feedback?(rkent)
(Assignee)

Comment 5

8 months ago
Created attachment 8774109 [details] [diff] [review]
WIP: Debug patch

Here is what I see with the debug patch, I've added the function name to the jsmime line number:

=== buildMimeText: value = |content-language|
=== buildMimeText: header = |en-US|
=== buildMimeText: value = |to|
=== buildMimeText: header = |[object Object]|
=== buildMimeText: value = |disposition-notification-to|
=== buildMimeText: header = |huhu@huhu.com|
== JS stack> resource:///modules/jsmime.jsm -> resource:///modules/jsmime/jsmime.js (3074) - addAddresses()
== JS stack> resource:///modules/jsmime.jsm -> resource:///modules/jsmime/jsmime.js (146) - writeAddress()
== JS stack> resource:///modules/jsmime.jsm -> resource:///modules/jsmime/jsmime.js (2995) - addStructuredHeader()
== JS stack> file:///C:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/components/mimeJSComponents.js (123)
JavaScript error: resource:///modules/jsmime.jsm -> resource:///modules/jsmime/jsmime.js, line 3079: TypeError: invalid 'in' operand addr

So, as we can see, and as Jonathan already diagnosed, for "to" addAddresses() is called with an object, as expected, but for "disposition-notification-to" it's called with a string. Good question where that should be tweaked.

BuildMimeText() is called here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#796

So perhaps there is a problem in how the headers are prepared in mime_generate_headers():
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompUtils.cpp#245
(Assignee)

Comment 6

8 months ago
IMHO, this bug should be seen in context with bug 1288932. There someone added a custom "References" header and populated it with a value that was too long and should have contained space separated references instead.

So somehow we need to process the content of custom headers before passing it to JSMime.
See Also: → bug 1288932

Comment 7

8 months ago
(In reply to Jorg K (GMT+2, PTO during summer) from comment #5)
> So, as we can see, and as Jonathan already diagnosed, for "to"
> addAddresses() is called with an object, as expected, but for
> "disposition-notification-to" it's called with a string. Good question where
> that should be tweaked.

Without knowing this code at all, just on basic grounds since addAddresses expects an iterable of objects (see https://dxr.mozilla.org/comm-central/source/mailnews/mime/jsmime/jsmime.js#3058) the place to fix this seems to be the caller that is incorrectly passing a string.
(Assignee)

Comment 8

8 months ago
I've been trying to find this caller (see comment #5). If you have any hints, let me know ;-)

The problem is that these headers are collected and finally turned into text. That's when the error happens. But the wrong type is added during the collection phase. Clearly, I have the caller (see comment #5), but it just passes in "wrong" information (wrong type) assembled elsewhere earlier. That "elsewhere" needs to be found and fixed.
(Assignee)

Comment 9

8 months ago
Created attachment 8774287 [details] [diff] [review]
Preposed solution (v1a).

Sorry Kent, another JSMime regression. I think the patch is self-explanatory.
Assignee: nobody → mozilla
Attachment #8722312 - Attachment is obsolete: true
Attachment #8774109 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8774287 - Flags: review?(rkent)

Comment 10

8 months ago
Comment on attachment 8774287 [details] [diff] [review]
Preposed solution (v1a).

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

::: mailnews/mime/jsmime/jsmime.js
@@ +143,5 @@
>    // Make sure the input is an array (accept a single entry)
> +  if (!Array.isArray(value)) {
> +    // Cater for custom headers where addresses are supplied as strings.
> +    if (typeof value == "string")
> +      value = {email: value};

Looks like you need to set the name property too:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/jsmime/jsmime.js#3054
(Assignee)

Comment 11

8 months ago
Works without it, but can easily be added. Of course the value of a bug like this in not writing a less-than-ten-line patch, but in the research that comes before.

Comment 12

8 months ago
(In reply to Jorg K (GMT+2, PTO during summer) from comment #11)
> Works without it, but can easily be added. Of course the value of a bug like
> this in not writing a less-than-ten-line patch, but in the research that
> comes before.

My concern was that if the name property is required and the code is not careful about its presence, you might eventually end up with a name somewhere set to "undefined" instead of a name "" (which the comment says is allowed).
(Assignee)

Comment 13

8 months ago
Created attachment 8774300 [details] [diff] [review]
Preposed solution (v1b).

OK, added the name property.
Attachment #8774287 - Attachment is obsolete: true
Attachment #8774287 - Flags: review?(rkent)
Attachment #8774300 - Flags: review?(rkent)
(Assignee)

Comment 14

8 months ago
Created attachment 8774334 [details] [diff] [review]
Proposed solution (v2a).

Cater for custom headers which include name and email.
Attachment #8774300 - Attachment is obsolete: true
Attachment #8774300 - Flags: review?(rkent)
Attachment #8774334 - Flags: review?(rkent)
(Assignee)

Comment 15

8 months ago
Created attachment 8774343 [details] [diff] [review]
Proposed solution (v2b).

More "manual" parsing, no idea how far we should take this.
Attachment #8774334 - Attachment is obsolete: true
Attachment #8774334 - Flags: review?(rkent)
Attachment #8774343 - Flags: review?(rkent)

Updated

8 months ago
Attachment #8774343 - Flags: feedback?(Pidgeot18)
(Assignee)

Comment 16

8 months ago
(In reply to Jorg K (GMT+2, PTO during summer) from comment #15)
> More "manual" parsing, no idea how far we should take this.
Surely instead of "manual" parsing we should just use the mime parser itself, something like:

function writeAddress(value) {
  // Make sure the input is an array (accept a single entry)
  if (!Array.isArray(value)) {
    // Cater for custom headers where addresses are supplied as strings.
    if (typeof value == "string") {
      value = headerparser.parseAddressingHeader(value, false); // false: No RFC 2047 required here.
    } else {
      value = [value];
    }
  }
  this.addAddresses(value);
}

but somehow due to my poor JS skills and the complexity of jsmime.js I couldn't get this to work yet.
(Assignee)

Comment 17

8 months ago
Created attachment 8774697 [details] [diff] [review]
Proposed solution (v3a).

Now using JSMime itself to parse custom addressing headers passed in as strings.

As I said, my JS skills are not the best, so just tell me how to improve this working solution ;-)
Attachment #8774343 - Attachment is obsolete: true
Attachment #8774343 - Flags: review?(rkent)
Attachment #8774343 - Flags: feedback?(Pidgeot18)
Attachment #8774697 - Flags: review?(rkent)

Comment 18

8 months ago
Every time I read jsmime, I get a little peeved that all of that non-standard (at least to Mozilla) boilerplate and strange method of function definition was added with no comments at all. Very obscure javascript that assumes possible existence of stuff (like amd modules) that do not exist at all in the Mozilla code.

Jcranmer, is there any way that you could add some comments that explains what this stuff is supposed to do? jsmime already has more levels of abstraction than us mere mortals can comprehend, and the extra multi-define module boilerplate really puts this over the top.
Comment on attachment 8774697 [details] [diff] [review]
Proposed solution (v3a).

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

I'm not sure this is the best approach.

Ideally, the code management for setting the custom header should be to parse the header and then re-emit it as a structured header. I'm pretty sure that any string that is setting the value as a string parses it and then re-emits as a structured header (that's what msgIWritableHeaders::setRawHeader does).

::: mailnews/mime/jsmime/jsmime.js
@@ +143,5 @@
>    // Make sure the input is an array (accept a single entry)
> +  if (!Array.isArray(value)) {
> +    // Cater for custom headers where addresses are supplied as strings.
> +    if (typeof value == "string") {
> +      let headerparser = require('./headerparser');

The easier way to do this is something akin to <https://github.com/jcranmer/jsmime/blob/mime-emit/structuredHeaders.js#L9> (for future reference). Recursive dependencies in JS is really hard to express in simplistic module systems.
(Assignee)

Comment 20

8 months ago
(In reply to Joshua Cranmer [:jcranmer] from comment #19)
> Ideally, the code management for setting the custom header should be to
> parse the header and then re-emit it as a structured header.
But then the "code management for setting the custom header" would have to know with which type of header it's dealing, right? So it needs to know that "Disposition-Notification-To" is an addressing header so a string needs to be parsed as address.

BTW, I've never found the "code management for setting the custom header". I would assume that the system knows about "From" and "To" already.
(Assignee)

Comment 21

8 months ago
Created attachment 8775553 [details] [diff] [review]
Proposed solution (v4).

(In reply to Joshua Cranmer [:jcranmer] from comment #19)
> Ideally, the code management for setting the custom header should be to
> parse the header and then re-emit it as a structured header. I'm pretty sure
> that any string that is setting the value as a string parses it and then
> re-emits as a structured header (that's what
> msgIWritableHeaders::setRawHeader does).

This is the solution Joshua asked for. Instead of using setHeader() which passes the string on, we now use setRawHeader() to benefit from parsing.

One line change, works beautifully ;-)
[So much work went into finding this one line, as Kent likes to point out]

You wouldn't believe it, but this one line change fixes bug 1288932 as well. Yes, the custom header input gets parsed properly and then passed on.
Attachment #8774697 - Attachment is obsolete: true
Attachment #8774697 - Flags: review?(rkent)
Attachment #8775553 - Flags: review?(rkent)
(Assignee)

Updated

8 months ago
Duplicate of this bug: 1288932
(In reply to Jorg K (GMT+2, PTO during summer) from comment #21)
> One line change, works beautifully ;-)
> [So much work went into finding this one line, as Kent likes to point out]

Unfortunately, it's hard to write documentation that other people can read, since you know the architecture you're developing intimately but others don't, and so it's not clear that what you think is an obvious principle of detail is not in fact obvious to others.

But I'm glad that I was able to clear something up. :-)

Comment 24

8 months ago
Comment on attachment 8775553 [details] [diff] [review]
Proposed solution (v4).

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

Thanks jcranmer, yes this makes sense. I also search and could not find any other obvious places where setHeader was used when setRawHeader made more sense.
Attachment #8775553 - Flags: review?(rkent) → review+
(Assignee)

Comment 25

8 months ago
https://hg.mozilla.org/comm-central/rev/bee84619fc28
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-thunderbird47: --- → wontfix
status-thunderbird48: --- → wontfix
status-thunderbird49: --- → affected
status-thunderbird50: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
(Assignee)

Comment 26

8 months ago
Comment on attachment 8775553 [details] [diff] [review]
Proposed solution (v4).

Long-standing issue: Custom headers don't work as expected. This was caused by the transition to JSMime. Low risk patch (one line) to correct this.
Attachment #8775553 - Flags: approval-comm-aurora+
(Assignee)

Comment 27

8 months ago
Aurora (TB 49):
https://hg.mozilla.org/releases/comm-aurora/rev/14361c5826f9
status-thunderbird49: affected → fixed
(Assignee)

Comment 28

8 months ago
Comment on attachment 8775553 [details] [diff] [review]
Proposed solution (v4).

[Approval Request Comment]
Regression caused by (bug #): JS Mime
User impact if declined: Custom headers don't work and e-mail is sent with no headers at all.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
Low risk, just swapping one function call for another.
Attachment #8775553 - Flags: approval-comm-esr45?
(Assignee)

Comment 29

8 months ago
Hi, this needs porting to SM if SM has custom headers.
Flags: needinfo?(rsx11m.pub)

Comment 30

8 months ago
Thanks, reproducible in SeaMonkey - I've filed bug 1292364.
Flags: needinfo?(rsx11m.pub)
See Also: → bug 1292364

Updated

7 months ago
Attachment #8775553 - Flags: approval-comm-esr45? → approval-comm-esr45+

Comment 31

7 months ago
https://hg.mozilla.org/releases/comm-esr45/rev/2f3c74c01deacebb6a00ce3618051d7c5d5cd4c0
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → +

Updated

7 months ago
tracking-thunderbird_esr45: + → 48+
(Reporter)

Comment 32

7 months ago
Hi, can somebody please clarify what it means to set "tracking-thunderbird_esr45" to "48+", in terms of exactly when the fix for this issue will be released in the ESR version? I'm asking because I need to let my add-on's users know when they can expect a fix for this issue. Thanks.
(Assignee)

Comment 33

7 months ago
46+ = 45.1
47+ = 45.2
48+ = 45.3
Basically ERS xx.y should come out at the same time FF xx+y.0 comes out, but we're always late ;-(
(Assignee)

Comment 34

7 months ago
s/ERS/ESR/
You need to log in before you can comment on or make changes to this bug.