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

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jonathan Kamens]

Attachment: 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.

Comment 1

[Jorg K (CEST = GMT+2)]

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

Comment 2

[Jonathan Kamens]

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.

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Jorg K (CEST = GMT+2)]

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

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 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 = |[e-mail]|
== 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

Comment 6

[Jorg K (CEST = GMT+2)]

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.

Comment 7

[aleth [:aleth]]

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

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: Preposed solution (v1a).

Sorry Kent, another JSMime regression. I think the patch is self-explanatory.

Comment 10

[aleth [:aleth]]

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

Comment 11

[Jorg K (CEST = GMT+2)]

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

[aleth [:aleth]]

(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).

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: Preposed solution (v1b).

OK, added the name property.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2a).

Cater for custom headers which include name and email.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2b).

More "manual" parsing, no idea how far we should take this.

Comment 16

[Jorg K (CEST = GMT+2)]

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

Comment 17

[Jorg K (CEST = GMT+2)]

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

Comment 18

[Kent James (:rkent)]

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 19

[Joshua Cranmer [:jcranmer]]

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.

Comment 20

[Jorg K (CEST = GMT+2)]

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

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 23

[Joshua Cranmer [:jcranmer]]

(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

[Kent James (:rkent)]

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.

Comment 25

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/bee84619fc28

Comment 26

[Jorg K (CEST = GMT+2)]

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.

Comment 27

[Jorg K (CEST = GMT+2)]

Aurora (TB 49):
https://hg.mozilla.org/releases/comm-aurora/rev/14361c5826f9

Comment 28

[Jorg K (CEST = GMT+2)]

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.

Comment 29

[Jorg K (CEST = GMT+2)]

Hi, this needs porting to SM if SM has custom headers.

Comment 30

[rsx11m]

Thanks, reproducible in SeaMonkey - I've filed bug 1292364.

Comment 31

[Kent James (:rkent)]

https://hg.mozilla.org/releases/comm-esr45/rev/2f3c74c01deacebb6a00ce3618051d7c5d5cd4c0

Comment 32

[Jonathan Kamens]

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.

Comment 33

[Jorg K (CEST = GMT+2)]

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

Comment 34

[Jorg K (CEST = GMT+2)]

s/ERS/ESR/