The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 47.0

Status

MailNews Core
MIME
RESOLVED FIXED
a year ago
10 months ago

People

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

Tracking

Trunk
Thunderbird 47.0

Thunderbird Tracking Flags

(thunderbird45+ fixed, thunderbird46 fixed, thunderbird47 fixed)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

a year ago
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 hidden (obsolete)
(Assignee)

Comment 2

a year ago
Created attachment 8724365 [details]
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.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a year ago
Created attachment 8724367 [details]
Very simple test message showing NO problem since alternative part comes first.

Weird, right ;-(
(Assignee)

Updated

a year ago
Attachment #8724365 - Attachment description: Very simple test message showing the problem. → Very simple test message showing the problem since the related part comes first.
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Comment 16

a year ago
Created attachment 8724890 [details] [diff] [review]
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.
Attachment #8724590 - Attachment is obsolete: true
Attachment #8724590 - Flags: review?(Pidgeot18)
Attachment #8724890 - Flags: review?(rkent)
Attachment #8724890 - Flags: review?(mkmelin+mozilla)
Attachment #8724890 - Flags: review?(Pidgeot18)
(Assignee)

Comment 17

a year ago
Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cc4120580a69
(Assignee)

Comment 18

a year ago
Try 100% green. Please r+ this patch!!
(Assignee)

Comment 19

a year ago
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.
Attachment #8724890 - Flags: approval-comm-beta?
Attachment #8724890 - Flags: approval-comm-aurora+

Comment 20

a year ago
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

a year ago
(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.
(Assignee)

Comment 22

a year ago
(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.
(Assignee)

Comment 23

a year ago
Created attachment 8725582 [details] [diff] [review]
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.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 24

a year ago
Aceman pointed me to test-commands.js. This opens a message and copies it to a folder. That's what I need.
Flags: needinfo?(mkmelin+mozilla)

Comment 25

a year ago
(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
(Assignee)

Comment 26

a year ago
Created attachment 8725923 [details] [diff] [review]
Proposed test (v1).

Here is your test. Can I please get approval for the 1/4 line change.
Attachment #8725582 - Attachment is obsolete: true
Attachment #8725923 - Flags: review?(mkmelin+mozilla)

Comment 27

a year ago
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
Attachment #8724890 - Flags: review?(rkent)
Attachment #8724890 - Flags: review?(mkmelin+mozilla)
Attachment #8724890 - Flags: review?(Pidgeot18)
Attachment #8724890 - Flags: review+
(Assignee)

Comment 28

a year ago
You land?

Comment 29

a year ago
https://hg.mozilla.org/comm-central/rev/19112cfa2d6a -> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-thunderbird47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0

Comment 30

a year ago
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)
Attachment #8725923 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 31

a year ago
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!
Attachment #8725923 - Flags: approval-comm-beta?
Attachment #8725923 - Flags: approval-comm-aurora+
(Assignee)

Comment 32

a year ago
(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 ;-)
(Assignee)

Comment 33

a year ago
Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/e38c9c1defb9
status-thunderbird46: affected → fixed

Comment 34

a year ago
Ok, thx for investigating.
Comment on attachment 8724890 [details] [diff] [review]
Proposed solution (v2).

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

(combined landing with test)
Attachment #8724890 - Flags: approval-comm-beta? → approval-comm-beta+

Updated

a year ago
Attachment #8725923 - Flags: approval-comm-beta? → approval-comm-beta+

Updated

a year ago
status-thunderbird45: affected → fixed
(Assignee)

Comment 36

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

[Triage Comment]
Attachment #8725923 - Flags: approval-comm-beta+ → approval-comm-esr45+
(Assignee)

Comment 37

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

[Triage Comment]
Attachment #8724890 - Flags: approval-comm-beta+ → approval-comm-esr45+
You need to log in before you can comment on or make changes to this bug.