1251783 - Forwarding of multipart message doesn't detect correct charset if multipart/related comes before multipart/alternative

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Jorg K (CEST = GMT+2)]

After fixing bug 1239658 where an erroneous charset override was removed, another bug is now more prominent.

When forwarding a multipart message which contains this structure

multipart/related
  R1 multipart/alternative
    A1 plaintext part
    A2 HTML part
  R2 embedded image
  R3 embedded image

instead of what TB produces

multipart/alternative
  A1 plaintext part
  A2 multipart/related
    R1 HTML part
    R2 embedded image
    R3 embedded image

the charset is not properly detected. This was first observed on Linux with attachment 438477 [details] from bug 494912 and is now also reproducible with attachment 520140 [details] from bug 532395.

A workaround is obviously setting the charset manually, thus causing an override, before forwarding the message.

My default outgoing charset is ISO-8859-1 and when I forward attachment 520140 [details] from bug 532395 it's all garbled.

Comment 1

[Jorg K (CEST = GMT+2)]

Magnus, this has come back to haunt us. I'd better look at this real quick before this causes problems in TB 45.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: Very simple test message showing the problem since the related part comes first.

I've hand-crafted a simple test message for debugging. The messages from the wild are far too complicated for debugging.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: Very simple test message showing NO problem since alternative part comes first.

Weird, right ;-(

Comment 4

[Jorg K (CEST = GMT+2)]

Joshua, any comment on this? This is totally weird. I'm not familiar with nsMailboxProtocol.cpp and nsMailboxService.cpp where the message is shuffled around. Let's add Kent and Neil as well. Sadly this is going outside the area of my expertise. As far as composition is concerned, the two messages are treated exactly the same, but something deep in the bowels goes wrong with one of them. Note that replying works for both cases.

This is an ancient old problem where an invalid fix was attempted in bug 494912 which caused regressions and was hence removed in bug 1239658 exposing the old problem again.

Comment 5

[Jorg K (CEST = GMT+2)]

BTW, replying works since it's a very different code path. Also, for replies we set the charset here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1988

Forwards seem to rely on the charset derived from the message content. Processing is here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1922 and exits here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1977

Setting the charset in this block makes no difference to the inlined content since the preceding MIME processing already got it wrong.

So this is not a composition bug but a MIME bug.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: WIP: debug patch

I've done a little debugging, here is what I see:
===== Got UTF-8 from text/html; charset=UTF-8
===== MimeMultipart_notify_emitter charset |UTF-8|
===== Getting charset from text/html
===== mimeCharset is null, using ISO-8859-1

So the charset was determined to be UTF-8, yet somewhere else it's not considered set, to we default to ISO-8859-1.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: WIP: debug patch (v2).

Slightly different:
===== Got UTF-8 from text/html; charset=UTF-8
===== MimeMultipart_notify_emitter charset |UTF-8|
===== MimeMultipart_notify_emitter default is charset |ISO-8859-1|
===== Getting charset from text/html
===== mdd->mailcharset: ISO-8859-1
===== mdd->options null
===== mimeCharset is null, using ISO-8859-1

Does anyone have a suggestion of how to get the charset information from point A to point B?

Sorry about the NI-SPAM ;-)

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: Possible solution (v1).

This works. Who can review this? Let's see what it breaks:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=da0e39fcaebb

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8724514 [details] [diff] [review]
Possible solution (v1).

Damn, this does NOT work. I pressed "Reply" instead of "Forward" and "Reply" always works. Sorry. So back to comment #7: How can the information be transferred.

This is funny, isn't it:
===== Got UTF-8 from text/html; charset=UTF-8 
...
===== Getting charset from text/html

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: WIP: debug patch (v3).

More comprehensive debug:

Setting:
===== Got UTF-8 from text/html; charset=UTF-8 <====
===== MimeMultipart_notify_emitter charset |UTF-8|
===== MimeMultipart_notify_emitter default is charset |ISO-8859-1|
===== Setting obj->options->default_charset on obj = 07624660, obj->options = 0BBF8570

Retrieving:
===== mdd = 08EF4470, mdd->obj = 08EF4510
===== mdd->options->default_charset = ISO-8859-1
===== mdd->obj->options->default_charset = ISO-8859-1
===== Getting charset from text/html <==== bad ????
===== mdd->mailcharset: ISO-8859-1
===== mdd->options is null
===== mimeCharset is null, using ISO-8859-1

So as we see, the part that knows the information operates on a different object from the part that retrieves the information.

Let's compare with the "good" message where the parts are in the correct order:
===== Got UTF-8 from text/plain; charset=UTF-8; format=flowed
===== MimeMultipart_notify_emitter charset |UTF-8|
===== MimeMultipart_notify_emitter default is charset |ISO-8859-1|
===== Setting obj->options->default_charset on obj = 07810E00, obj->options = 09C85420

===== Got UTF-8 from text/html; charset=UTF-8 <====
===== MimeMultipart_notify_emitter charset |UTF-8|
===== MimeMultipart_notify_emitter default is charset |ISO-8859-1|
===== Setting obj->options->default_charset on obj = 09C29940, obj->options = 09C850E0

===== mdd = 06F57B50, mdd->obj = 06F57F10
===== mdd->options->default_charset = UTF-8 <=== how did that change to UTF-8??
===== mdd->obj->options->default_charset = UTF-8
===== Getting charset from text/html; charset=UTF-8 <==== good 
===== mdd->mailcharset: UTF-8
===== mdd->options is null

As we can see, it works as designed, charset is retrieved from text/html; charset=UTF-8.

Comment 11

[Jorg K (CEST = GMT+2)]

I had a chat with Joshua on IRC. He gave me some tips for debugging. He also said that no one really understands the mime code, so I'll clear the NIs.

The problem appears to be that the inner alternative parts don't communicate the charset back to the outer related parts.

Displaying and replying works since the charset is directly communicated to the window. Reply also works based on the window's charset.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: Possible solution (v1).

OK, this is a bit hacky and adds more charset magic to the mime soup.

Once detected, we store the charset in the mime object and move it up to the parent.

Comment 13

[Jorg K (CEST = GMT+2)]

I should mention: I'm setting the default, so it can be retrieved here:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1152
and then used here:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1362

As we seen from the debugging,
mimeCharset = MimeHeaders_get_parameter (mdd->messageBody->m_type.get(), "charset", nullptr, nullptr);
comes back null (since mdd->messageBody->m_type.get() == "text/html") and we use mdd->mailcharset which was retrieved at line 1152 from the default.

Comment 14

[Jorg K (CEST = GMT+2)]

Another remark:
If mdd->messageBody->m_type.get() didn't come back with "text/html" but with "text/html; charset=UTF-8", of course I wouldn't need the default fiddle. Sadly I wasn't able to work out where this is set. I was searching for "messageBody->m_type" and I can't see it being set.

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #14)
> If mdd->messageBody->m_type.get() didn't come back with "text/html" but with
> "text/html; charset=UTF-8", of course I wouldn't need the default fiddle.
> Sadly I wasn't able to work out where this is set. I was searching for
> "messageBody->m_type" and I can't see it being set.
I've been debugging this for several hours now. I hope I can present a more elegant solution based on mdd->messageBody->m_type.get() returning the correct value in the next 24 hours.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v2).

OK, in hindsight, I could have found this faster ;-)

This is the proposed solution, a 1/4 line change.

Sorry about the r?-SPAM, this is urgent since it needs to ship in TB 45. Otherwise forwarding of (some) UTF-8 messages won't work ... and that won't be well received. To understand the background, read comment #0.

Comment 17

[Jorg K (CEST = GMT+2)]

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

Comment 18

[Jorg K (CEST = GMT+2)]

Try 100% green. Please r+ this patch!!

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8724890 [details] [diff] [review]
Proposed solution (v2).

[Approval Request Comment]
Regression caused by (bug #): bug 1239658 (undid bug 494912)
User impact if declined: *Severe* as you can't forward some UTF-8 messages.
Testing completed (on c-c, etc.): Manually, try run clear.
Risk to taking this patch (and alternatives if risky):
No risk. 1/4 line change getting the full header incl. charset instead of text/html only.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 8724890 [details] [diff] [review]
Proposed solution (v2).

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

Gotta love the mime code. It's almost like you entered an alternative universe :)

Could you add a test, maybe take model from test-multipart-related.js and/or test-eml-actions.js

Comment 21

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #19)
> No risk. 1/4 line change getting the full header incl. charset instead of
> text/html only.

I think while small this is still fairly risky as there's a fair bit of unknown behaviour here.

Comment 22

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #20)
> Could you add a test, maybe take model from test-multipart-related.js and/or
> test-eml-actions.js
As I said, right now I can't write any tests. TB doesn't create messages like this, to the test would have to import a message and forward it. All that test will prove it that the fix works. Nothing else. I can prove that manually for now. I'm happy to do a bunch of missing test in the next cycle (once you approved the Eudora kill ;-)).

Again, if it doesn't land now, we will ship TB 45 with a big problem which was fixed wrongly by David Bienvenu in bug 494912. As for the risk: All we do is use a full header rather than a stripped one. That's what the two parameters are about. The try run is clear, so the the risk of landing it is less that the certain damage not landing it will do.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: WIP: test

Just to show my super-good will I've started on a test (although I should really be doing something else). In fact this test works and passes. I could just be mean and ask for review and wait until someone finds out that the test is 100% useless ;-)

Opening a saved message and forwarding always works. The case that doesn't work is forwarding from a folder.

So, how do I import a message into a folder? Examples please.

Comment 24

[Jorg K (CEST = GMT+2)]

Aceman pointed me to test-commands.js. This opens a message and copies it to a folder. That's what I need.

Comment 25

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #23)
> Opening a saved message and forwarding always works. The case that doesn't
> work is forwarding from a folder.

Hmm, how come? Working samples in tests are also useful as then you known a certain behaviour is desired when you thinker with the code later.

> So, how do I import a message into a folder? Examples please.

Probably something like http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/message-window/test-commands.js#29

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: Proposed test (v1).

Here is your test. Can I please get approval for the 1/4 line change.

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 8724890 [details] [diff] [review]
Proposed solution (v2).

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

Yeah seems to work. r=mkmelin
I still wonder why it works without the patch for .emls

Comment 28

[Jorg K (CEST = GMT+2)]

You land?

Comment 29

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/19112cfa2d6a -> FIXED

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 8725923 [details] [diff] [review]
Proposed test (v1).

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

r=mkmelin
(already landed this with the main patch)

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 8725923 [details] [diff] [review]
Proposed test (v1).

[Approval Request Comment]
Regression caused by (bug #): bug 1239658 (undid bug 494912)
User impact if declined: *Severe* as you can't forward some UTF-8 messages.
Testing completed (on c-c, etc.): Manually, and with new test. Also clear try run.
Risk to taking this patch (and alternatives if risky):
No risk. 1/4 line change getting the full header incl. charset instead of text/html only.

Note: C-C landing is *one* combined changeset instead of two. So Aurora and Beta only need to transplant one changeset.

Thanks Magnus!

Comment 32

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #27)
> I still wonder why it works without the patch for .emls
So we can both sleep better: It worked by accident. No charset could be determined at
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1362
so the body was left as it is on line after.

Debug says without the patch:
===== mime_parse_stream_complete: line 1362 - mimeCharset = (null), mdd->mailcharset = (null)
With the patch we now get:
===== mime_parse_stream_complete: line 1362 - mimeCharset = UTF-8, mdd->mailcharset = (null)

For messages from a folder we now get:
===== mime_parse_stream_complete: line 1152 - set mdd->mailcharset to ISO-8859-1
===== mime_parse_stream_complete: line 1362 - mimeCharset = UTF-8, mdd->mailcharset = ISO-8859-1

Before, mimeCharset was equally null, so we got ISO-8859-1 from the default and consequently displayed UTF-8 as ISO-8859-1 resulting in mojibake.

Happy? I am because we fixed something Bienvenu couldn't fix back in bug 494912 in 2009 ;-)

Comment 33

[Jorg K (CEST = GMT+2)]

Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/e38c9c1defb9

Comment 34

[Magnus Melin [:mkmelin]]

Ok, thx for investigating.

Comment 35

[Kent James (:rkent)]

Comment on attachment 8724890 [details] [diff] [review]
Proposed solution (v2).

http://hg.mozilla.org/releases/comm-esr45/rev/6f75b0bb8538

(combined landing with test)

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 8725923 [details] [diff] [review]
Proposed test (v1).

[Triage Comment]

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8724890 [details] [diff] [review]
Proposed solution (v2).

[Triage Comment]