Last Comment Bug 705431 - make sure a MIME part with content-disposition: attachment is displayed as an attachment nonetheless even though it doesn't have a filename
: make sure a MIME part with content-disposition: attachment is displayed as an...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Reader UI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 13.0
Assigned To: Jonathan Protzenko [:protz]
:
Mentors:
Depends on:
Blocks: 609667 702938
  Show dependency treegraph
 
Reported: 2011-11-26 06:27 PST by Julian Reschke
Modified: 2012-03-12 04:02 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
11+
fixed


Attachments
sample mail file (2.24 KB, text/plain)
2011-11-26 06:27 PST, Julian Reschke
no flags Details
Patch, with tests (7.32 KB, patch)
2012-01-20 14:22 PST, Jonathan Protzenko [:protz]
squibblyflabbetydoo: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Review

Description Julian Reschke 2011-11-26 06:27:31 PST
Created attachment 577060 [details]
sample mail file

Thunderbird tries to extract filename information from Content-Dispositon.

When this fails (either because the filename parameter is missing, or because parsing the header failed), it should still display the attachment.
Comment 1 rsx11m 2011-11-26 07:58:39 PST
Jonathan, is your recent work in bug 564423 on getting rid of pseudo-attachments contributing to what's seen here? (xref discussion in bug 702938)
Comment 2 Jonathan Protzenko [:protz] 2011-11-26 09:08:11 PST
Julian: the whole point of bug 564423 was to not show attachments which didn't have a filename. If an attachment doesn't have a filename, it means it's not really an attachment: it's a part of the message, like a mailing list signature in html format, or some similarly innocuous thing. We then added a View > Show as > All body parts to make sure people could access these "attachments" if they wanted to.

The big libmime rewrite we've been dreaming of includes, as one of its goal, a better criterion for deciding whether to show these parts, namely "if no other displayed part of the message references that part, we should give the user a way to access it".

Reverting bug 564423 would basically make all html mailing-list messages have a fake attachment named "Part 1.2". I don't think that's a good thing to have in a modern email client.

If we really have to do this, the right thing to do is change the current criterion to: don't treat this part as an attachment if it doesn't have a filename AND its content-disposition is not attachment.

Does that sound right for everyone?
Comment 3 Jonathan Protzenko [:protz] 2011-11-26 09:09:34 PST
To make things clear: libmime has a very, very fuzzy notion of "attachment" and the current criterion for determining whether we *erroneously* understood a mime body part to be an attachment is: "it doesn't have a filename". What happens then is that this part is discarded from the attachment list. What I'm suggesting is: change this to "it doesn't have a filename and its content disposition is not attachment".
Comment 4 Julian Reschke 2011-11-26 09:39:25 PST
(In reply to Jonathan Protzenko [:protz] from comment #3)
> ...
> attachment list. What I'm suggesting is: change this to "it doesn't have a
> filename and its content disposition is not attachment".

Sounds right to me. It matches the definition of Content-Disposition, and also how browsers behave.
Comment 5 Jonathan Protzenko [:protz] 2011-11-26 10:12:54 PST
Cool. I'll try to tackle this tomorrow, then :).
Comment 6 Joe Sabash [:JoeS1] 2011-12-11 12:37:23 PST
(In reply to Jonathan Protzenko [:protz] from comment #5)
> Cool. I'll try to tackle this tomorrow, then :).

[ANNIE]
The sun'll come out
Tomorrow
Bet your bottom dollar
That tomorrow
There'll be sun!

Just thinkin' about
Tomorrow
Clears away the cobwebs,
And the sorrow
'Til there's none!
Comment 7 WADA 2011-12-20 00:57:29 PST
Test case to see this bug's problem, which was originally attached to bug 701261 comment #44 : attachment 578180 [details]

Two mails of multipart/mixed, with non-displayable message body part.
 part-1: audio/wav, no name parameter in Content-Type: (Voice00 in string)
           mail-1 : no filename
           mail-2 : with filename
 part-2: text/plain,      no filename,   no name (Voice01 in string)
 part-3: audio/wav,       no filename,   no name (Voice02 in string)
 part-4: application/pdf, no filename,   no name (Voice03 in string)
 part-5: text/plain,      with filename, no name (Voice11 in string)
 part-6: audio/wav,       with filename, no name (Voice12 in string)
 part-7: application/pdf, with filename, no name (Voice13 in string)

(A) non-displayable message body part(part-1) is;
    mail-1 (no filename)   : not shown at attachment pane
    mail-2 (with filename) : shown at attachment pane
(B) sub parts without filename(part-2 to part-4) is not shown at attachment pane
    sub parts with    filename(part-5 to part-7) is shown at attachment pane
Comment 8 WADA 2011-12-20 01:33:39 PST
As written in comment #2, currently available way to see hidden sub parts as attachment at attachment pane is;
(1) Config Editor, mailnews.display.show_all_body_parts_menu = true
(2) View/Message Body As/All Body Parts

Currently known problems in daily use of "All Body Parts" are;
(A) If problem like bug 701261, non-displayable message body of non-multipart mail, user can always be aware of hidden message body by blank message didplay with no attachment, so user can use "All Body Parts" as workaround. However, if non-displayable attachment part with no name/no file name in multipart/mixed, user can't be aware of hidden attachment, so it's hard to use workaround of "All Body Parts", unless user already knows that required attachment is not shown at attachment pane.
(B) If message body part and/or attachment part is multipart/related and contents in the multipart/related is "text/html with cid: url + embed image parts", these image parts are shown as independent attachment and embed images are not rendered in html mail display.
And, if very many embed image parts are used by html mail, it's hard for user to reach at required hidden/important sub parts.  

Because there is requirement to save embed image parts in multipart/related via attachment pane display, above (B) is mandatory behaviour of "All Body Parts" mode.
To improve (B), new "View/All Body Parts with showing multipart/related as multipart/related" like one will be probably required for daily use of "All Body Parts" as workaround of problem caused by malformed or not-so-well-formed or not-formed-as-Tb-expects mail.
Comment 9 WADA 2011-12-20 01:45:04 PST
(In reply to Jonathan Protzenko [:protz] from comment #3)
> What I'm suggesting is: change this to "it doesn't have a filename and its content disposition is not attachment".

I think "not shown part" needs to be added to "parts shown at attachment pane" for problem like bug 674473.
Further, "Content-Type:application/pkcs7-mime if Content-Disposion: header is also specified" may also be needed for problem like bug 705059.
Comment 10 WADA 2012-01-09 23:21:54 PST
FYI. bug 229075 and bug 381544 is report of this bug's problem in 2003 and 2007.
Comment 11 Jonathan Protzenko [:protz] 2012-01-19 02:07:10 PST
Note to self (because I looked into the issue but I don't have time to implement it right now):
- add a m_disposition field to nsMsgAttachmentData (in nsIMsgSend.idl)
- fill in that field from BuildAttachmentList in mimemoz2.cpp
- change the test near the beginning of NotifyEmittersOfAttachmentList in mimemoz2.cpp to not skip the attachment if its content-disposition is indeed attachment

(Should be easy. Famous last words.)
Comment 12 Jonathan Protzenko [:protz] 2012-01-20 14:22:48 PST
Created attachment 590351 [details] [diff] [review]
Patch, with tests

This was way easier than I expected. This basically makes sure that if the part has content-disposition: attachment, then we always, whatever happens, display it. That sounds like a good property we want to have :).
Comment 13 Jonathan Protzenko [:protz] 2012-01-20 14:25:02 PST
s/if the part/if the nsMsgAttachmentData/

Mailing-list footers (the infamous Part 1.2 attachments) have content-disposition: inline, and still they end up being nsMsgAttachmentData. We still don't display these.
Comment 14 Jonathan Protzenko [:protz] 2012-01-20 14:25:55 PST
(In reply to Joe Sabash from comment #6)
> (In reply to Jonathan Protzenko [:protz] from comment #5)
> > Cool. I'll try to tackle this tomorrow, then :).
> 
> [ANNIE]
> The sun'll come out
> Tomorrow
> Bet your bottom dollar
> That tomorrow
> There'll be sun!
> 
> Just thinkin' about
> Tomorrow
> Clears away the cobwebs,
> And the sorrow
> 'Til there's none!

Yes, tomorrow is a little bit late. Having a PhD ongoing and also TA-ing and doing other stuff tends to yield less time available for fixing bugs in libmime :)
Comment 15 Jim Porter (:squib) 2012-02-08 17:48:51 PST
Comment on attachment 590351 [details] [diff] [review]
Patch, with tests

Looks good to me. The XPCShell tests pass, as do my manual tests. Sorry about taking so long on the review!
Comment 16 Jonathan Protzenko [:protz] 2012-02-11 04:19:36 PST
No worries :). It'll actually also fix a bug in Conversations which is good for me too. I'll just wait for the tree to reopen then I'll land this. I'm wondering if we should backport the fix in GlodaAttachment onto aurora and beta...

(https://github.com/protz/GMail-Conversation-View/issues/566 for the record)
Comment 17 Jonathan Protzenko [:protz] 2012-02-14 00:05:35 PST
http://hg.mozilla.org/comm-central/rev/06c52cc3c7f9
Comment 18 Jonathan Protzenko [:protz] 2012-02-14 00:06:30 PST
Comment on attachment 590351 [details] [diff] [review]
Patch, with tests

Requesting approval *only* for the one-line fix in mailnews/db/gloda/modules/datamodel.js
Comment 20 Jonathan Protzenko [:protz] 2012-02-14 12:19:24 PST
Two things:
- I completely forgot to rev the UUID of nsIMsgSend.idl in my original patch,
- I request approval only for the one-line fix in datamodel.js, not the rest of the patch that touched nsIMsgSend.idl

Sorry for the confusion.
Comment 21 Mark Banner (:standard8) 2012-02-14 12:51:56 PST
(In reply to Jonathan Protzenko [:protz] from comment #20)
> Two things:
> - I completely forgot to rev the UUID of nsIMsgSend.idl in my original patch,
> - I request approval only for the one-line fix in datamodel.js, not the rest
> of the patch that touched nsIMsgSend.idl
> 
> Sorry for the confusion.

As discussed on irc, the patch didn't actually touch the interface nsIMsgSend, it just touched a class definition (I do wonder why that's in the idl), aiui this shouldn't affect extensions.
Comment 22 Mark Banner (:standard8) 2012-03-06 00:59:38 PST
Checked in for ESR 10.0.3: http://hg.mozilla.org/releases/comm-esr10/rev/f0e5d995d302

Note You need to log in before you can comment on or make changes to this bug.