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.

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)

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?

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".

(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.

Cool. I'll try to tackle this tomorrow, then :).

(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!

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

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.

(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.

FYI. bug 229075 and bug 381544 is report of this bug's problem in 2003 and 2007.

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.)

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 :).

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.

(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 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!

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 on attachment 590351 [details] [diff] [review] Patch, with tests Requesting approval *only* for the one-line fix in mailnews/db/gloda/modules/datamodel.js

Checked into branches: http://hg.mozilla.org/releases/comm-aurora/rev/4b4f1883cd21 http://hg.mozilla.org/releases/comm-beta/rev/66d01c6ae244

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.

(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.

Checked in for ESR 10.0.3: http://hg.mozilla.org/releases/comm-esr10/rev/f0e5d995d302