Last Comment Bug 745919 - TB 11.0.forwarding plain-text (non-MIME) emails creates empty "Attached Message Part" even when forward messages is set Inline, if no Content-Type header, no parameter in Content-Type:.
: TB 11.0.forwarding plain-text (non-MIME) emails creates empty "Attached Messa...
Status: RESOLVED FIXED
: regression
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: 11
: All All
: -- normal with 1 vote (vote)
: Thunderbird 23.0
Assigned To: Magnus Melin
:
Mentors:
: 752177 829317 846343 (view as bug list)
Depends on:
Blocks: 856456
  Show dependency treegraph
 
Reported: 2012-04-16 13:27 PDT by Jim Lawson
Modified: 2013-04-30 02:49 PDT (History)
18 users (show)
mkmelin+mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Mail folder file (8 mails for test) (3.88 KB, text/plain)
2012-04-17 22:07 PDT, WADA
no flags Details
[checked in] Fix (1.38 KB, patch)
2012-06-04 16:06 PDT, Hiroyuki Ikezoe (:hiro)
mozilla: review+
Details | Diff | Splinter Review
email that cause the problem (4.71 KB, text/plain)
2013-02-09 06:52 PST, Serg Iv
no flags Details
proposed fix (6.52 KB, patch)
2013-04-13 12:05 PDT, Magnus Melin
mconley: review+
Details | Diff | Splinter Review

Description Jim Lawson 2012-04-16 13:27:24 PDT
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120310194926

Steps to reproduce:

I received a plain-text (non-MIME) message and forwarded it, while using "forward inline" mode.


Actual results:

Thunderbird forwarded the message inline as I requested, but also added a zero-length attachment of Content-Type: text/plain name="Attached Message Part"


Expected results:

The original message should be forwarded inline, with no empty attachment.  The empty attachment is confusing to the recipient.
Comment 1 Jim Lawson 2012-04-16 13:30:07 PDT
This was seen on Thunderbird 11.0.1 on Win7 and OSX 10.7.3.
Comment 2 Jim Lawson 2012-04-16 13:42:34 PDT
This problem also exists in 12.0.b4
Comment 3 Hashem Masoud 2012-04-17 00:36:12 PDT
(In reply to Jim Lawson from comment #2)
Works for me:
User Agent: Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20120327 Thunderbird/11.0.1
Application Build ID: 20120327115527

Try to start Thunderbird without add-ons and check.
Comment 4 Jim Lawson 2012-04-17 04:49:07 PDT
Thanks for trying to recreate.  Starting without add-ons does not make the problem go away.

IMAP server is Dovecot 2.1.4.  Although copying the original plain text message to a local folder does not change the behavior.

I will try a Linux install of 11.0.1 when I get to that setup...
Comment 5 Jim Lawson 2012-04-17 07:51:37 PDT
Was able to recreate problem in 11.0.1 on Linux (Ubuntu 10.04.4 LTS) on x86_64

Name=Thunderbird
Version=11.0.1
BuildID=20120327115527

Try an RFC822 complaint message, one WITHOUT MIME-Version:, Content-Type:, and Content-Transfer-Encoding: headers.  The problem is not triggered if these headers are present.
Comment 6 WADA 2012-04-17 20:36:59 PDT
Problem is observed in next cases(With MIME-Version: 1.0 existent).
(1) No Content-Type: header
(2) Content-Type: (no parameter)
(3) Content-Type: unknown/unknown
(4) Content-Type: ???### (syntactically invalid)
Problem doesn't occur if major type is text.
(5) Content-Type: text/??? (syntactically invalid)
(6) Content-Type: text/unknown
Perhaps a regression by "quirks for malformed mail of non-displayable content-type like audio/wav, application/pdf". By it, such malformed mail is internally treated as if multipart/mixed in order to show the non-displayable message body of RFC822 mail as if attachment.

http://tools.ietf.org/html/rfc2045#section-5.2 says default is following.
  Content-type: text/plain; charset=us-ascii

What mailer or mail system did send mail without Content-Type: header?
Comment 7 Jim Lawson 2012-04-17 20:51:53 PDT
Sendmail which receives a "bare" message without headers will not add Content-Type headers, as it is not required for compliance with RFC2822 (or, it appears, RFC2042 if text/plain is the default.)
Comment 8 WADA 2012-04-17 22:07:26 PDT
Created attachment 616015 [details]
Mail folder file (8 mails for test)

Problem is observed also on multipart/mixed mail with no attachment part.
(7) multipart/mixed, single part only in multipart/mixed,
    no Content-Type: header of text data part(message body part)
(8) multipart/mixed, single part only in multipart/mixed,
    Content-Type: unknown/unknown of text data part(message body part)
In (3) Content-Type: unknown/unknown and (4) Content-Type: ???### cases, phenomenon of "message body is not shown by view" is also observed in Tb 11.0.1.

Quick check result with a Tb 7.0a2 build. 
> Mozilla/5.0 (Windows NT 5.1; rv:7.0a2) Gecko/20110816 Thunderbird/7.0a2
Note: "This bug occurs" in Tb 7.0a2 = Attachment part is created by Forward,
                                      data is message body text instead of null.
(1) No Content-Type: header         This bug doesn't occur
(2) No parameter in Content-Type:   This bug doesn't occur
(3) Content-Type: unknown/unknown   Attachment is shown by view.
                                    This bug occurs. 
(4) Content-Type: ???###            Attachment is shown by view.
                                    This bug occurs. Bug 738284 occurs.
(5) Content-Type: text/???          This bug occurs.
(6) Content-Type: text/unknown      This bug occurs.
(7) multipart/mixed, single part only in multipart/mixed,
    no Content-Type: header of text data part(message body part)
                                    This bug doesn't occur.
(8) multipart/mixed, single part only in multipart/mixed,
    Content-Type: unknown/unknown of text data part(message body part)
                                    Attachment is shown by view.
                                    This bug occurs.

No Content-Type: case and No parameter of Content-Type: case looks regression.
Other cases seems old problem.
Comment 9 Hiroyuki Ikezoe (:hiro) 2012-06-04 16:06:53 PDT
Created attachment 629984 [details] [diff] [review]
[checked in] Fix

This bug is a regression by bug 679476.
Comment 10 Hiroyuki Ikezoe (:hiro) 2012-06-04 16:07:10 PDT
*** Bug 752177 has been marked as a duplicate of this bug. ***
Comment 11 David :Bienvenu 2012-06-04 16:24:31 PDT
Comment on attachment 629984 [details] [diff] [review]
[checked in] Fix

thx, Hiro, yes, that gets the code back to what it did before the regression.
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-06-09 07:26:26 PDT
https://hg.mozilla.org/comm-central/rev/d5a2b2d481d5
Comment 13 lightsolphoenix 2012-09-05 07:14:34 PDT
A few employees at my company just upgraded to Thunderbird 15, and we're seeing this bug again; phantom empty "nsmail.tmp" being created when someone hits Forward.
Comment 14 Hiroyuki Ikezoe (:hiro) 2012-09-05 16:50:03 PDT
Please read comment 12. This issue will be solved in Thunderbird 16. Sorry for the patience.
Comment 15 Thiago 2012-10-15 11:37:00 PDT
This bug also exists in TB 16.0.1(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> Please read comment 12. This issue will be solved in Thunderbird 16. Sorry
> for the patience.

This bug also exists in TB 16.0.1
Comment 16 Steve L 2012-10-29 11:22:14 PDT
We are seeing this problem but only with a few machines here. We are running TB 16.0.2 on Windows XP pro. Anyone have the solution?
Comment 17 robert1 2012-12-30 07:14:21 PST
Still seeing this at TB 17.0 running on Windows 7 64 bit.
Comment 18 seanspratt 2013-01-15 18:03:52 PST
Confirmed and seeing this problem on Thunderbird Windows 7 x64 in 17.0.2 even with add-ons disabled
Comment 19 Bassam 2013-01-31 14:53:12 PST
I'm having this issue as well, it doesn't do it on all emails, but the ones it does do this on if you reply it does no attachment. This only happens when forwarding.
Comment 20 Serg Iv 2013-02-09 06:45:28 PST
The problem is still here in 17.0.2. Can someone reopen this bug?
Comment 21 Serg Iv 2013-02-09 06:52:11 PST
Created attachment 712133 [details]
email that cause the problem

email that cause the problem
Comment 22 WADA 2013-02-28 21:48:22 PST
> http://hg.mozilla.org/comm-central/annotate/65b8e2fa4eba/mailnews/mime/src/mimedrft.cpp#l489
>           hg@0 489 //It's possible we must treat the message body as attachment!
>       mwu@8587 490 bool bodyAsAttachment = false;
> hiikezoe@10396 491 if (mdd->messageBody &&
> hiikezoe@10396 492 mdd->messageBody->m_type.Find("text/html", CaseInsensitiveCompare) == -1 &&
> hiikezoe@10396 493 mdd->messageBody->m_type.Find("text/plain", CaseInsensitiveCompare) == -1 &&
> hiikezoe@10396 494 !mdd->messageBody->m_type.LowerCaseEqualsLiteral("text"))
>  bugzilla@9082 495 bodyAsAttachment = true;
Patch by this bug is still active.

I also see problem in Tb 17.0.3 on Win-XP as Serg Iv says, and as seen in Bug 846343. With any test mail of my attachment 616015 [details] , I could still see problem of this bug.

Re-opening.

Ikezoe-san, did you actually test the patch?

Is default string of "text/plain" set in mdd->messageBody when no Content-Type: header?
If no, it looks for me the patch resolved problem with "Content-Type: Text" like case...
Comment 23 WADA 2013-02-28 21:51:51 PST
mistake in my pasting... sorry for spam.
  mdd->messageBody  ==>  mdd->messageBody->m_type
Comment 24 Ed L 2013-03-01 07:02:59 PST
Also seeing this issue in Tbird 17.0.3 on Win 7 64bit, x-ref bug report 846343
Comment 25 Bassam 2013-03-01 08:43:40 PST
Hello  WADA ,

Is there anything I can do to help you guys solve this? Where on Thunderbird can I see the default string for content type?

This issue is still on 17.03.
Comment 26 WADA 2013-03-01 20:03:33 PST
Following is compose format determination logic.
> http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1336
> 1336     if (mdd->messageBody)
> 1337     {
> 1338       MSG_ComposeFormat composeFormat = nsIMsgCompFormat::Default;
> 1339       if (!mdd->messageBody->m_type.IsEmpty())
> 1340       {
> 1341         if(mdd->messageBody->m_type.Find("text/html", CaseInsensitiveCompare) != -1)
> 1342           composeFormat = nsIMsgCompFormat::HTML;
> 1343         else if (mdd->messageBody->m_type.Find("text/plain", CaseInsensitiveCompare) != -1 ||
> 1344                  mdd->messageBody->m_type.LowerCaseEqualsLiteral("text"))
> 1345           composeFormat = nsIMsgCompFormat::PlainText;
> 1346         else
> 1347           //We cannot use this kind of data for the message body! Therefore, move it as attachment
> 1348           bodyAsAttachment = true;
> 1349       }
> 1350       else
> 1351         composeFormat = nsIMsgCompFormat::PlainText;

Ikezoe-san, I think following is better. Is it right?
> 489   //It's possible we must treat the message body as attachment!
> 490   bool bodyAsAttachment = false;
> 491   if (mdd->messageBody &&    ===>    if (mdd->messageBody && !mdd->messageBody->m_type.IsEmpty() &&
> 492       mdd->messageBody->m_type.Find("text/html", CaseInsensitiveCompare) == -1 &&
> 493       mdd->messageBody->m_type.Find("text/plain", CaseInsensitiveCompare) == -1 &&
> 494       !mdd->messageBody->m_type.LowerCaseEqualsLiteral("text"))
> 495      bodyAsAttachment = true;
Comment 27 WADA 2013-03-01 20:10:37 PST
And, if TEXT, Text, texT(no / nor subtype) is considered text, I think TEXT/, Text/, texT/ (with / and no subtype) is better considered text.
Comment 28 WADA 2013-03-02 20:28:34 PST
Some other thoughts on code.
1. If texT/ like one is cared, comparison like CaseInsensiveStartWith("text/") is better.
2. If logic like 1336 to 1351 is used at multiple places, Objects's method like mdd->messageBody->m_type.Is_text_family() may be useful, because duplicate code is eliminated.
Comment 29 WADA 2013-03-03 20:34:17 PST
*** Bug 846343 has been marked as a duplicate of this bug. ***
Comment 30 Jonathan Kamens 2013-03-31 13:00:30 PDT
*** Bug 829317 has been marked as a duplicate of this bug. ***
Comment 31 Jonathan Kamens 2013-03-31 13:07:48 PDT
I have regressed bug 745919 and confirmed that the first nightly build in which it appears is 2011-09-23. That means that it was almost certainly caused by the commit in bug 679476, which landed on 2011-09-22.

That means that the bug has been in the code base for over 18 months, and for over 9 months since it was allegedly fixed by the commit in bug 745919 which didn't actually fix it.

It would be nice if one of the people responsible for writing or reviewing the commit in bug 679476, or one of the people responsible for writing or reviewing the patch in bug 745919, would step up and fix this bug properly.

It's pretty appalling that it's still in the code base after over 18 months.
Comment 32 Magnus Melin 2013-04-11 13:23:29 PDT
I have a fix for this, thx for the detective work WADA!
Comment 33 Magnus Melin 2013-04-13 12:05:22 PDT
Created attachment 737167 [details] [diff] [review]
proposed fix

Fix the case of non-mime message (with no content type), and add test. (Well, testing bug 856456 which this also fixes, and tagging on the test there's no attachment either.)
Comment 34 Bassam 2013-04-16 15:42:15 PDT
So what is the fix for this problem now, is there even one?
Comment 35 Magnus Melin 2013-04-16 23:14:50 PDT
If you do your own build, apply the patch. Otherwise wait until the patch is reviewed and checked in, then it is fixed in nightly builds.
Comment 36 Mike Conley (:mconley) - (Needinfo me!) 2013-04-28 17:50:51 PDT
Comment on attachment 737167 [details] [diff] [review]
proposed fix

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

Just some nits here, but I think this looks alright.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2558,5 @@
>  {
>    var send_default_charset = gMsgCompose.compFields.defaultCharacterSet;
>  //  dump("send_default_charset is " + send_default_charset + "\n");
>  
> +  var compFieldsCharset = gMsgCompose.compFields.characterSet ||

While you're here, let not var.

::: mail/test/mozmill/composition/test-save-changes-on-quit.js
@@ +34,5 @@
>    collector.getModule("prompt-helpers").installInto(module);
>    collector.getModule("window-helpers").installInto(module);
>  
>    folder = create_folder("PromptToSaveTest");
> +  

Trailing whitespace.

@@ +44,5 @@
>  
> +function msgSource(aSubject, aContentType) {
> +  let msgId = Components.classes["@mozilla.org/uuid-generator;1"]
> +                        .getService(Components.interfaces.nsIUUIDGenerator)
> +                        .generateUUID() +"@invalid";

Space after +

@@ +247,5 @@
> +/**
> + * Test there's no prompt on close when no changes was made in reply/forward
> + * windows - for the case the original msg had content type "text".
> + */
> +function test_no_prompt_on_close_for_unmodified_content_type_text() {

Nice tests.

::: mailnews/mime/src/mimedrft.cpp
@@ +478,5 @@
>    nsMsgAttachedFile           *tmpFile = NULL;
>  
>    //It's possible we must treat the message body as attachment!
>    bool bodyAsAttachment = false;
> +  if (mdd->messageBody && !mdd->messageBody->m_type.IsEmpty() &&

Let's put this new term on its own line.
Comment 37 Bassam 2013-04-29 10:06:29 PDT
I don't have my own build, do you know how long it will be before Thunderbird adds this fix to their newer builds?
Comment 38 Magnus Melin 2013-04-29 11:22:41 PDT
It will be in nigthly builds in a guess a day or a few, depending on when i have the chance to update the patch and check it in. (When i mark this bug FIXED, it's in the next nigthly build.)
Comment 39 Magnus Melin 2013-04-30 02:48:07 PDT
http://hg.mozilla.org/comm-central/rev/eabed5c2dfe1 -> FIXED

Removing the part about corrupted mime-type from summary. The patch didn't change those, and i don't know how/if those shoud be handled - in any case they should be rare.

Note You need to log in before you can comment on or make changes to this bug.