583419 - Plaintext attachments are always displayed inline

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Joe Sabash [:JoeS1]]

Attachment: testcase eml

Noted in:
Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b3pre) Gecko/20100728 Shredder/3.2a1pre ID:20100728032601
Not seen in 20100722 build.

STR:
View the attached eml

The content-disposition is attachment, yet the text displays regardless of pref settings.

Looks like fallout from bug 551698

Comment 1

[rsx11m]

Also reproduced in Mozilla/5.0 (Windows; Windows NT 5.1; rv:2.0b3pre) Gecko/20100730 SeaMonkey/2.1a3pre, TB 3.1 Lanikai builds are ok.

Comment 2

[Phil Lacy]

in view orig html I get the message and background
a divider line and 'this body part will be downloaded on demand' and attached.txt in attachments

in view plain I get
This body part will be downloaded on demand.
same divider line--attached.txt
This body part will be downloaded on demand.
and part 1.1.1.2 and attached.txt in attachments

to get the attachment to display I have to set display attachments inline

Comment 3

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #2)
> in view plain I get
> This body part will be downloaded on demand.
> same divider line--attached.txt
> This body part will be downloaded on demand.
> and part 1.1.1.2 and attached.txt in attachments
> to get the attachment to display I have to set display attachments inline

Phil Lacy, how did you view the test mail?
(A) Import .eml to local mail folder (Drag&Drop .eml to thread pane)
(B) File/Open Saved Message... of local .eml file by Tb
(C) Click attachment(message/rfc822) of this bug by Browser => Opened by Tb
(D) Import .eml to IMAP offline-use=on  folder(Drag&Drop .eml to thread pane)
(E) Import .eml to IMAP offline-use=off folder(Drag&Drop .eml to thread pane)

I could observe "This body part will be downloaded on demand" only by (E) with next trunk build.
> Mozilla/5.0 (Windows NT 5.1; rv:2.0b4pre) Gecko/20100806 Shredder/3.2a1pre
I think it's a variant of known issue(you probably already know) around mismatch among multipart-on-demand, MIME, Mail Display when complex structure.
 
Phil Lacy, as (B),(C),(D),(E) may produce other problem than this bug's main problem, I recommend you to check by (A) first.
Note: Even (D), problem like bug 572974 comment #7 can happen, although such problem is very hard to reproduce with attached test mail to this bug.

Comment 4

[Joe Sabash [:JoeS1]]

(In reply to comment #2)
> in view orig html I get the message and background
> a divider line and 'this body part will be downloaded on demand' and
> attached.txt in attachments
> 
I probably should have added that the original symptoms were seen in pop3 mail.
But if you open the testcase directly by TB or as a local copy (as WADA suggested) you should see the attachment over-writing the background (always) and not under control of the "display attachments inline pref"

Comment 5

[Phil Lacy]

I see it now in local folders. I was in imap with mime parts on demand set to true.

Comment 6

[David :Bienvenu]

the issue here is the ignoring of the pref, not the content disposition. In other words, the pref trumps the content disposition.

Comment 8

[Mark Banner (:standard8)]

David seems to have an idea about what this is about, so passing to him.

Comment 9

[David :Bienvenu]

the fix for bug 551698 did indeed cause this regression, in particular, I think the first chunk of the patch. I'll see if there's some way of fixing this without breaking bug 551698, but failing that, I'd back out that patch.

Comment 10

[David :Bienvenu]

Attachment: proposed fix

I think displaying the cached part inline is always the right thing to do.

Comment 11

[David :Bienvenu]

Comment on attachment 528447 [details] [diff] [review]
proposed fix

Phil, does this seem like the right thing to do? It fixes this bug, without regressing the earlier bug.

Comment 12

[David :Bienvenu]

Attachment: better fix

this is a bit less hacky fix. It backs out the regression causing patch, and fixes that bug a different way by explicitly telling mime_create to force the part to be inline, in this case. I don't know quite how to write a unit test for this.

Comment 13

[David :Bienvenu]

Attachment: fix with unit test

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 530395 [details] [diff] [review]
fix with unit test

(I think Jim is the king of attachments these days :)

Comment 15

[Jim Porter (:squib)]

Comment on attachment 530395 [details] [diff] [review]
fix with unit test

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

Aside from the two nits I have below, this looks good. r=me with those addressed.

::: mailnews/mime/test/unit/test_text_attachment.js
@@ +34,5 @@
+
+/**
+ * This test creates some messages with attachments of different types and
+ * checks that libmime reports the expected size for each of them.
+ */

This should be changed to describe the contents of this test.

@@ +51,5 @@
+// Create a message generator
+const msgGen = gMessageGenerator = new MessageGenerator();
+// Create a message scenario generator using that message generator
+const scenarios = gMessageScenarioFactory = new MessageScenarioFactory(msgGen);
+

Do we need anything here other than just the "gMessageGenerator = new MessageGenerator();" part?

Comment 16

[David :Bienvenu]

fix checked in with comments addressed, thx for the review!
http://hg.mozilla.org/comm-central/rev/d538b947880c

Comment 17

[Phil Lacy]

thanks for fix, David, I been out of country traveling on and off for some time with only my iPad so can't do much to help or test without TB.

Comment 18

[David :Bienvenu]

thx for getting back to me, Phil. I think you/bugzilla accidentally re-opened, so re-marking fixed.

Comment 19

[Serge Gautherie (:sgautherie)]

http://hg.mozilla.org/releases/comm-2.0/rev/9db274a5beb4