Closed Bug 1584383 Opened 5 months ago Closed 5 months ago

Cannot Open or Save PDF Attachments in Emails Forwarded from MS Outlook 2016

Categories

(MailNews Core :: Attachments, defect)

defect
Not set

Tracking

(thunderbird_esr6870+ fixed, thunderbird70 fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 71.0
Tracking Status
thunderbird_esr68 70+ fixed
thunderbird70 --- fixed
thunderbird71 --- fixed

People

(Reporter: dank, Assigned: alta88)

Details

Attachments

(2 files, 2 obsolete files)

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

Steps to reproduce:

Update To new version: 68.1.1 (32-bit) From: 68.1.0 (32-bit).
Forward email with pdf attachment from MS Outlook 2016.

Actual results:

Receive Alert dialog box with message:
This attachment appears to be empty.
Please check with the person who sent this.
Often company firewalls or antivirus programs will destroy attachments.

Also, attachment does not show when attempting to forward the email.
No problems when receiving the same email sent from Thunderbird.

Expected results:

Should have allowed the user to open or save the attachment.

You need to attach the e-mail in question. Save it as .eml file, edit it in an editor to remove identifying information, then attach it to the bug using "Attach New File". Of course the attachment will be publicly visible. Or forward the message directly to me as an attachment.

Based on your description, there could be 100 issues. If the e-mail is OK when sent from TB, we really need to see what Outlook 2016 has sent.

Attachment #9096975 - Attachment mime type: message/rfc822 → text/plain

The message has a wrong MIME structure. Line 20 should be Content-Type: multipart/mixed; not multipart/related;

If you change it to "mixed", all works.

Status: UNCONFIRMED → RESOLVED
Closed: 5 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1404822

I do understand the problem is caused by MS Office but this hadn't been a problem in 68.1.0. It just started in 68.1.1. I rechecked on multiple installations.

OK. Yes, we made some MIME changes between TB 68.1.0 and TB 68.1.1. Maybe they have this side effect. Kai, can you please take a look.

Flags: needinfo?(kaie)

Aha, I tried in TB 60.9 and the attachment is "size unknown". So that change doesn't come from the MIME changes but some other attachment changes. Alta88?

Flags: needinfo?(kaie) → needinfo?(alta88)
Attached patch fetch.patch (obsolete) — Splinter Review

So this non spec case is actually one of those "libmime never managed to figure out", and because it is in a local folder (non imap url) it will test the fetch() codepath and thus demonstrate the dumbest typo ever.

Mozmill attachment/ tests run fine locally.

Assignee: nobody → alta88
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(alta88)
Attachment #9097137 - Flags: review?(jorgk)
Resolution: DUPLICATE → ---
Comment on attachment 9097137 [details] [diff] [review]
fetch.patch

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

Thanks for looking into this.

::: mail/base/content/msgHdrView.js
@@ -2155,5 @@
>            // will come from libmime.
>            let reader = response.body.getReader();
>            let { value: chunk } = await reader.read();
>            reader.cancel();
> -          size = chunk && chunk.value ? chunk.value.length : -1;

I can just see how error-prone that shorthand it, so how about leaving the line you changed and change two lines above to
let chunk = await reader.read();
?
Attached patch fetch.patch (obsolete) — Splinter Review

Do you mind using the version that a simple mind can understand?

Attachment #9097137 - Attachment is obsolete: true
Attachment #9097137 - Flags: review?(jorgk)
Attachment #9097154 - Flags: review+
Comment on attachment 9097154 [details] [diff] [review]
fetch.patch

BTW, yes, that fixes the issue with the sample e-mail.
Attachment #9097154 - Flags: approval-comm-esr68+
Attachment #9097154 - Flags: approval-comm-beta+

The syntax is {value: |some var like chunk|, done: |some var like readerDone| } and at some point the whole stream can be read and a real size determined, since libmime didn't do it, which needs a small loop until readerDone is true. So I think leaving that syntax makes that obvious, and I would have done it like that except the linter complains if readerDone isn't used. Anyway, the fix is in, you can tweak/land it however you like.

So if you go with your way, I'd use a var like |result| and not chunk.

Attached patch fetch.patchSplinter Review

Thanks for the comment. I'm cool with result like our M-C friends do:
https://searchfox.org/mozilla-central/search?q=await+reader.read()%3B&case=false&regexp=false&path=

Attachment #9097156 - Flags: review+
Attachment #9097156 - Flags: approval-comm-esr68+
Attachment #9097156 - Flags: approval-comm-beta+
Attachment #9097154 - Attachment is obsolete: true
Attachment #9097154 - Flags: review+
Attachment #9097154 - Flags: approval-comm-esr68+
Attachment #9097154 - Flags: approval-comm-beta+
Target Milestone: --- → Thunderbird 71.0

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/57f0f8a7f242
Fix fetch size checking. r=jorgk

Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Resolution: --- → FIXED
Component: FileLink → Attachments
Product: Thunderbird → MailNews Core
You need to log in before you can comment on or make changes to this bug.