Closed Bug 1358565 Opened 3 years ago Closed 3 years ago

Embedded image from HTML part (multipart/alternative + multipart/related) cannot be opened, saved or detached when viewing plain text part

Categories

(MailNews Core :: MIME, defect)

defect
Not set
normal

Tracking

(thunderbird_esr45 fixed, thunderbird_esr5254+ fixed, thunderbird53 wontfix, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr45 --- fixed
thunderbird_esr52 54+ fixed
thunderbird53 --- wontfix
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: johnkw, Assigned: jorgk-bmo)

References

Details

(Keywords: dataloss, regression)

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170413192749

Steps to reproduce:

Click: View > Message Body As > Plain text
Click the dropdown arrow below the bottom-right of the message.
Click "Detach...", then "Save".


Actual results:

The file is created with the correct name, in the requested location, but as a zero-byte useless file.  The attachment is then deleted from the email, resulting in what appears to be total data loss of the attachment.

It's worth noting that if this happens to you, and you are a fairly tech-savvy user, you can go in and edit the mbox "X-Mozilla-Status: 0009" to say "X-Mozilla-Status: 0001" for the old message, and then do "Properties > Repair Folder" and get your data back.


Expected results:

Correct file saved.
I can't reproduce this, but I haven't tried all possible variations:
Local folder, IMAP folder, with/without local storage, etc.

Does this happen with all add-ons disabled (see Help menu)?

In TB 52.0.1 we have trouble with large attachments in IMAP folders which are not sync-ed/selected for offline use, bug 1355350.

Can you attach a message whose attachment can't be detached?

Are you sure your anti-virus program doesn't eat the saved file?
The bug appears to be in the part of Thunderbird responsible for extracting the bytes, not the part writing the file.

If you take an HTML-embed email, and do "View > Message Body As > Plain text" and then expand the bottom-left ">" and then double-click the image name, Thunderbird pops up an error "This attachment appears to be empty."  If you return to "View > Message Body As > Original HTML" then it renders as normal.
Attached file mbox to reproduce bug
It looks like it is possible to create an HTML email that doesn't reproduce the bug, if you omit the "plain text" mime section that would normally be present from any email client.  Other than that, I don't see any special requirements.  See attached a small mbox file that reproduces the bug.
Hmm, I never use "Detach" and the "natural" way to save the image would be viewing it in HTML mode and saving it with a right click.

But I can confirm that double-clicking throws up the message "This attachment appears to be empty", and didn't in TB 45.x

We fixed a few bugs between 45.x and 52, for example, viewing the plain text part now really shows the plain text part and not a "stripped down" HTML part.

Alice, looks I need your help again.

STR:
- Save the bug attachment as whatever.eml and store it into a folder.
- View > Message Body As > Plain Text
- Double-click the attachment.

Regressing bug maybe bug 253830 comment #133 (landed 2016-05-30) or bug 574989 comment #31 (landed  2016-06-04). Or something else.

Many thanks in advance.
Flags: needinfo?(alice0775)
Component: Untriaged → MIME
Product: Thunderbird → MailNews Core
Version: 52 Branch → 49
Thank you, Alice.

I think what's happening here is somewhat clear.

The message has this structure:

multipart/alternative
  text/plain
  multipart/related
    text/html
    image

After bug 253830 the plain text part is now selected in plaintext display and somehow the access to the image in the other part doesn't work any more.
You can generally fix MIME problems with a single line change ;-)

Aceman, I know, you're not a MIME expert, but the fix is pretty obvious. When opening the attachment, nsMimeMessageRaw is passed as the output option.

Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8e54dc1867698ddf2cc2b8aadd181f59b88640ca
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8860757 - Flags: review?(acelists)
Summary: Using "Detach" on a file (image) from HTML email causes data loss → Embedded image from HTLM part (multipart/alternative + multipart/related) cannot be opened, saved or detached when viewing plain text part
Summary: Embedded image from HTLM part (multipart/alternative + multipart/related) cannot be opened, saved or detached when viewing plain text part → Embedded image from HTML part (multipart/alternative + multipart/related) cannot be opened, saved or detached when viewing plain text part
(In reply to Jorg K (GMT+2) from comment #7)
> You can generally fix MIME problems with a single line change ;-)
I think that MIME code was always wrong only that you couldn't prioritise the plain text part properly before bug 253830. So bug 253830 didn't actually "cause" this problem, it just exposed it ;-)

Also, the use case is of course questionable. "Normal" attachments were never affected, only the "fake" attachments you get then showing the plain text part. So if someone decides to detach such images and destroy their HTML part, then they have the additional problem of losing the image. As I said in comment #4, usually you save those images in the HTML view with a right-click onto the image.

Anyway, that's just to put things into perspective. The regression will be fixed, no discussion about that.
I would point out that this is a common use-case if you have correspondents that are not super tech savvy. I think it's what happens when they drag photos into the text of an email in order to share them, creating a massive email with a photo album embedded in base64 in HTML. The email itself will sometimes include useful text, but you want to detach the attachments in order to save the many megs of disk space. Also right-clicking each photo one after the other after the other is quite painful.
Point taken. But detaching the image will actually destroy the HTML part, the image shows as broken after the operation and I don't intend to fix that.

(In reply to johnkw from comment #9)
> I would point out that this is a common use-case if you have correspondents
> that are not super tech savvy.
I don't have any of those. Even my 80 y/o mother knows how to attach images ;-)
Comment on attachment 8860757 [details] [diff] [review]
1358565-select-for-attachment.patch (v1).

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

Yes, this works for me on the attached test message.

You can land it but I'll see if I can make a test for it.
Attachment #8860757 - Flags: review?(acelists) → review+
https://hg.mozilla.org/comm-central/rev/ce3b48dad7205fe03e852aad11968a01c51d3a6f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Comment on attachment 8860757 [details] [diff] [review]
1358565-select-for-attachment.patch (v1).

[Approval Request Comment]
Regression caused by (bug #): Bug 253830.
Attachment #8860757 - Flags: approval-comm-esr52?
Attachment #8860757 - Flags: approval-comm-beta+
Attached patch test (obsolete) — Splinter Review
The promised test :) It fails without your fix as attachmentElem.attachment.isEmpty will be true.
Attachment #8862976 - Flags: review?(jorgk)
Comment on attachment 8862976 [details] [diff] [review]
test

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

Thanks, I tried hard to find some nits ;-)

::: mail/test/mozmill/attachment/bug1358565.eml
@@ +1,5 @@
> +Date: Thu, 22 Feb 2017 10:00:00 -0300
> +From: <nobody@example.com>
> +MIME-Version: 1.0
> +To: <nobody2@example.com>
> +Content-Type: multipart/alternative; boundary=Apple-Mail-16-000000000

I wouldn't run free ads for Apple in our code. Can you lose the "Apple".

@@ +59,5 @@
> +Ajy28ZrvOT7lPViA+03PlneSNY/VSC0OVppGucuCAtuwStZ7BbZhMeZa4VtgVy4ru44tRhfYRuVN
> +TsQWowtso7KmKFPWeQW2QXlrsSkLCgLbYAj2piVTfvJAYCuJbLV0MxavN95PFKTu4BPYyhMJTA0C
> +IaUEldDrLdYPFwUEdiGw1nsnoEmGho3XmdB8Mh6cszteYCcGW7p8l/uTBwLbKFi8vKQYTmAriYyV
> +8JqyVOf9JF9K9iuwM0OmCKFfrvNgc4zW/7Zi1QgiDHs6NMgEAisJrCSwksBK8foHNXX2LMmMFTgA
> +AAAASUVORK5CYII=

Hmm, this message is invalid. You're missing the closing separators

--Apple-Mail-17-000000000-- and
--Apple-Mail-16-000000000--

They have trailing --.
Attachment #8862976 - Flags: review?(jorgk) → review+
Attached patch test v1.1Splinter Review
Thanks. I used the file uploaded by the user. Does Apple mail produce such invalid messages?
Attachment #8862976 - Attachment is obsolete: true
Attachment #8863039 - Flags: review+
I don't think so, most likely truncated by the submitter.
Oh, do there need to be empty lines before and between the headers at the end?
I aggressively deleted lines that weren't critical to repro the Thunderbird bug. Apple emails that I have do contain the MIME closing separators.
(In reply to Jorg K (GMT+2) from comment #20)
> Oh, do there need to be empty lines before and between the headers at the
> end?

I don't know. I looked at the file content-utf8-alt-rel.eml in mozmill/composition. But haven't preserved the empty lines exactly as it seemed to work. But then, the mail worked in TB also without the ending delimiters :)
Checkin-needed? I can add an extra empty line between the headers at the end.
Flags: in-testsuite+
https://hg.mozilla.org/comm-central/rev/ce3b48dad7205fe03e852aad11968a01c51d3a6f (comment #12, fix)
https://hg.mozilla.org/comm-central/rev/461a08a028a3746bd86e5949abff636f6b6083af (test)

Landed the test with the following modifications:
- Added empty line between the headers at the end.
- Changed the .eml file to CRLF and made the content of the plain text and HTML parts
  a little more descriptive.
- Added // make SOLO_TEST=attachment/test-attachment-in-plain-msg.js mozmill-one to the test,
  nice for copy/paste when running the test manually.
Nice, thanks.
Attachment #8860757 - Flags: approval-comm-esr52? → approval-comm-esr52+
Duplicate of this bug: 1372935
You need to log in before you can comment on or make changes to this bug.