Plaintext attachments are always displayed inline

RESOLVED FIXED in Thunderbird 5.0b1

Status

MailNews Core
MIME
--
major
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: JoeS1, Assigned: Bienvenu)

Tracking

(Depends on: 1 bug, {regression})

Trunk
Thunderbird 5.0b1
x86
Windows XP
regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-thunderbird5.0 needed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 461719 [details]
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

8 years ago
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.
Component: Mail Window Front End → MIME
Product: Thunderbird → MailNews Core
QA Contact: front-end → mime
Severity: normal → major

Comment 2

8 years ago
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
(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.
(Reporter)

Comment 4

8 years ago
(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

8 years ago
I see it now in local folders. I was in imap with mime parts on demand set to true.
Whiteboard: [tbtrunkneeds]
(Assignee)

Comment 6

8 years ago
the issue here is the ignoring of the pref, not the content disposition. In other words, the pref trumps the content disposition.
Flags: blocking-thunderbird-next+
Whiteboard: [tbtrunkneeds]
blocking-thunderbird5.0: --- → needed
Flags: blocking-thunderbird-next+
Duplicate of this bug: 617874
David seems to have an idea about what this is about, so passing to him.
Assignee: nobody → dbienvenu
(Assignee)

Updated

7 years ago
Attachment #461719 - Attachment mime type: application/x-mimearchive → text/plain
(Assignee)

Comment 9

7 years ago
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.
(Assignee)

Comment 10

7 years ago
Created attachment 528447 [details] [diff] [review]
proposed fix

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

Comment 11

7 years ago
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.
Attachment #528447 - Flags: feedback?(philbaseless-firefox)
(Assignee)

Updated

7 years ago
Whiteboard: [has possible fix]
(Assignee)

Comment 12

7 years ago
Created attachment 530136 [details] [diff] [review]
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.
Attachment #528447 - Attachment is obsolete: true
Attachment #528447 - Flags: feedback?(philbaseless-firefox)
(Assignee)

Comment 13

7 years ago
Created attachment 530395 [details] [diff] [review]
fix with unit test
Attachment #530136 - Attachment is obsolete: true
Attachment #530395 - Flags: review?(bugmail)
(Assignee)

Updated

7 years ago
Whiteboard: [has possible fix] → [has fix for review]
Comment on attachment 530395 [details] [diff] [review]
fix with unit test

(I think Jim is the king of attachments these days :)
Attachment #530395 - Flags: review?(bugmail) → review?(squibblyflabbetydoo)

Comment 15

7 years ago
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?
Attachment #530395 - Flags: review?(squibblyflabbetydoo) → review+
(Assignee)

Comment 16

7 years ago
fix checked in with comments addressed, thx for the review!
http://hg.mozilla.org/comm-central/rev/d538b947880c
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4

Comment 17

7 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 3.3a4 → ---
(Assignee)

Comment 18

7 years ago
thx for getting back to me, Phil. I think you/bugzilla accidentally re-opened, so re-marking fixed.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Whiteboard: [has fix for review]
Depends on: 1018606
You need to log in before you can comment on or make changes to this bug.