User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

Received this email in Thunderbird with only an attachment (no message body):

Content-Type: text/plain; name=test.txt
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename=test.txt
From: from@localhost.com
To: me@mydomain.com
Subject: Test
Message-ID: <c7a2664e-6803-c144-1c70-96dc0941e3a1@localhost.com>
Date: Mon, 04 Feb 2019 19:35:59 +0000
MIME-Version: 1.0

VGhpcyBpcyB0aGUgdGV4dApmaWxlIDMu

Actual results:

The inline-display of the attachment (when enabled) displayed the text file correctly. However, once opened from a temp file, or downloaded elsewhere, the file "test.txt" contained the base64 encoded value.

Expected results:

The file "test.txt" should have contained the base64 decoded as plain-text.

Related to Bug 1361422.

Alfred, could you please take a look.

I just added a obj->options->is_child to

https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/mime/src/mimeleaf.cpp#100

https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/mime/src/mimeleaf.cpp#96

      // If we need the object as "raw" for saving or forwarding,
      // don't decode text parts of message types. Other output formats,
      // like "display" (nsMimeMessageBodyDisplay), need decoding.
      (obj->options->format_out == nsMimeOutput::nsMimeMessageRaw &&
       obj->parent &&
       (!PL_strcasecmp(obj->parent->content_type, MESSAGE_NEWS) ||
        !PL_strcasecmp(obj->parent->content_type, MESSAGE_RFC822)) &&
       !PL_strncasecmp(obj->content_type, "text/", 5)))

That begs me the question, what is the purpose of this code?
When do we need the coded form of an attachment?
An example for testing would be helpful?

Hmm, all good questions. Doesn't the comment suggest that we need the un-decoded for for forwarding, for example. I'll try this with the patch. Yes, we're lacking test coverage in this area.

I tried the patch. Yes, with the message in comment #0 it works, but it doesn't work, when it's forwarded. You get the enclosed message and the attachment doesn't open correctly.

(In reply to Alfred Peters from comment #4)

      // If we need the object as "raw" for saving or forwarding,

[...]

That begs me the question, what is the purpose of this code?

I do remember: Bug 1350080

When do we need the coded form of an attachment?
An example for testing would be helpful?

attachment 8850706 [details]

(In reply to Jorg K (GMT+1) from comment #6)

Works in the case presented here, but now when the message is forwarded.

Now obj->options->is_child is also TRUE and we are back at the beginning.

Would you like to investigate a little further?

Sure, of course. In fact we need a better solution for Bug 1350080.

The example form comment 0 and attachment 8850706 [details] are really hard to keep apart.

The real point is: when should we not decode the body?
Answer: if we save it together to a Content-type: header.

The solution is to recognize this case as correctly as possible.

obj->parent->output_p should indicate something has been written from the parent object. This could only have been the headers.

(In reply to Alfred Peters from comment #8)

The real point is: when should we not decode the body?
Answer: if we save it together to a Content-type: header.

Content-Transfer-Encoding: header of cause.

(In reply to Jorg K (GMT+1) from comment #9)

Comment on attachment 9044508 [details] [diff] [review]
Do not decode attachments (base64/qp), if they are saved with headers.

Review of attachment 9044508 [details] [diff] [review]:

This works for the cases connected to the bug.

I tested it with the test cases from Bug 1350080 Bug 1361422 and this bug of cause.
I tested view, save and forward the attachments/mails.

::: mailnews/mime/src/mimeleaf.cpp
@@ -101,3 @@

    (!PL_strcasecmp(obj->parent->content_type, MESSAGE_NEWS) ||

•    !PL_strcasecmp(obj->parent->content_type, MESSAGE_RFC822)) &&
  

•   !PL_strncasecmp(obj->content_type, "text/", 5)))
  

What was this doing and why is it OK to remove? Can you give an example?

That's from the fix to the first regression (Bug 1361422) from Bug 1350080.

This doesn't work because a TXT file and an email both have content-type 'text/*'

The Content-type: header also does not matter. No matter what we save, if we do that with Content-Transfer-Encoding: base64 or quoted-printable, then we need the encoded content.

I'll look at it again tonight.

You're aware of our new preference implemented here, right?
https://hg.mozilla.org/comm-central/rev/43c9c600cf84#l1.17
I needed to switch this of get the inline display.

(In reply to Jorg K (GMT+1) from comment #11)

You're aware of our new preference implemented here, right?
https://hg.mozilla.org/comm-central/rev/43c9c600cf84#l1.17

Already since last week.
At first I wondered about the new option in the debugger 🤔.
Then I wondered why the view did not work anymore 😳.
And then came the aha effect 🙄.

OK, tested with ...

• the e-mail from comment #0
• attachment 9042972 [details]
• attachment 8850706 [details] from bug 1350080 (forward, save attachment, open)
• attachment 8851783 [details] from bug 1350080 (forward, save attachment, open)
• attachment 8864261 [details] from bug 1361422 (save attachment, open).

All still working, try run is green, so let's go with this.

Doing the testing I noticed, painfully, that we still haven't fixed the issue that the mailbox name will be used as file name in come cases (mentioned in bug 1350080 comment #1) and bug 1352740 for some tests. I've filed bug 1528932 for the former.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/59f55da59da5
Fix condition for decoding attachments (base64/qp) when saving or forwarding. r=jorgk

I've been agonising over the commit message. The suggestion "Do not decode attachments (base64/qp) if they are saved with headers" didn't sound right since attachments are typically not saved with headers, that's the whole point. They may be processed with headers for forwarding.

Can you enlighten me where output_p is set and what it means. Yes, I should have asked earlier. Maybe we should risk a follow-up and add some comments.

TB 66 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/e243207d0509362dc760c28ebfc3204a6732f832

Alfred, please answer comment #16.

(In reply to Jorg K (GMT+1) from comment #16)

Can you enlighten me where output_p is set and what it means. Yes, I should have asked earlier. Maybe we should risk a follow-up and add some comments.

It is expensive to determine that. Man has to keep manual book, about the object addresses.

In which case exactly do you want to know that?

For saving a TXT attachment, this is done here:

https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/mime/src/mimeobj.cpp#220

OK, let's drop the "where output_p is set" and concentrate on "what it means". What does it mean?

(In reply to Jorg K (GMT+1) from comment #20)

OK, let's drop the "where output_p is set" and concentrate on "what it means". What does it mean?

Hard to say.

1. See Comment 8

2. In which case exactly do you want to know that?
   For saving a TXT attachment:
   https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/mime/src/mimeobj.cpp#208

3. http://geek-and-poke.com/geekandpoke/2009/7/25/the-art-of-programming-part-2.html

Sorry because i could not be helpful.

I read comment #8. I'd just like to enhance the comment here
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/mime/src/mimeobj.h#161
and where we use is now.

Since our decoding logic depends on it now, it would be good to have a better understanding.

(In reply to Jorg K (GMT+1) from comment #22)

I read comment #8. I'd just like to enhance the comment here
https://searchfox.org/comm-central/rev/afd4157b12e26ea99fca1492bcf06196eb817003/mailnews/mime/src/mimeobj.h#161
and where we use is now.

I see.

• bool output_p; /* Whether it should be written. */

• bool output_p; /* Whether it should be written.

•                     If it is the parent of an encoded (base64/qp)
  

•                     attachment, this flag shows, if the headers of that
  

•                     attachment are already written. The flag therefore
  

•                     decides whether the attachment must be written
  

•                     encoded. */
  

I tried to add a comment here and it occurred to me that we can drop the special casing for message attachments, no? I've done some indentation fixes as well.

TB 66 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/9d13f6b3d44693bf5516e1db776b3fad4d9fe702

TB 60.6.1 or later:
https://hg.mozilla.org/releases/comm-esr60/rev/fe503daad8bab81f8e0788cad27c1573d5efbf2c
https://hg.mozilla.org/releases/comm-esr60/rev/d6609c0131946eaa1f243b49bc63b34a232293e6