Closed Bug 1251783 Opened 4 years ago Closed 4 years ago

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

Categories

(MailNews Core :: MIME, defect)

defect
Not set

Tracking

(thunderbird45+ fixed, thunderbird46 fixed, thunderbird47 fixed)

RESOLVED FIXED
Thunderbird 47.0
Tracking Status
thunderbird45 + fixed
thunderbird46 --- fixed
thunderbird47 --- fixed

People

(Reporter: jorgk, Assigned: jorgk)

Details

Attachments

(4 files, 6 obsolete files)

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.
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
Attachment #8724365 - Attachment description: Very simple test message showing the problem. → Very simple test message showing the problem since the related part comes first.
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)
Try 100% green. Please r+ this patch!!
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 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
(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.
(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.
Attached patch WIP: test (obsolete) — Splinter Review
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)
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)
(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
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 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+
You land?
https://hg.mozilla.org/comm-central/rev/19112cfa2d6a -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
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+
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+
(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 ;-)
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+
Attachment #8725923 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8725923 [details] [diff] [review]
Proposed test (v1).

[Triage Comment]
Attachment #8725923 - Flags: approval-comm-beta+ → approval-comm-esr45+
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.