564423 - Get rid of "Part 1.2" attachments being displayed unless "Show all message parts" is selected

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Jonathan Protzenko [:protz]]

This is the followup to bug 554294. Basically, we'd like Gloda not to treat as attachments:
 - .eml forwarded messages or quoted messages,
 - vcards,
 - footers,
 - Part $X attachments

According to Patrick, signatures (.asc) should be considered as regular attachments because the user might need to save them.

This is not an easy task because we cannot rely simply on the attachment having a name or not in the MIME message (signatures might not have a name for instance).

A nice side-effet would be to fix the message reader not to display those "fake" attachments in the attachment list. But there's this preference called View > Display Attachments Inline that is to be taken into account too (it would be risky to consider an attached .eml as fake if the only way to see it then is to have this preference enabled). 

According to Andrew, we might want to fix this for Gloda first, then fix the message reader next, if it hasn't been replaced by the time we get there.

Comment 1

[Patrick Brunschwig]

(In reply to comment #0)
> According to Patrick, signatures (.asc) should be considered as regular
> attachments because the user might need to save them.

To be precise here: signatures (.asc) file are real attachments, *unless* their content-type is "application/pgp-signature". In the latter case the signature can be hidden.

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

What kind of files are these signatures (.asc) which aren't content-type "application/pgp-signature"?  I don't think I've seen one before.


Much of the question of real is going to be "should it be a facet for searching", "should it show in the message lists", "should it show in the message reader".  Some, I believe most signatures, should only appear in the message reader and likely inline instead of in the attachment area.

Comment 3

[Patrick Brunschwig]

These are signature files from inline-PGP, usually either plain text files or application/octet-stream encoded. These files can have any name; signature.asc is just a possible file name.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

If we can recognize a signature lets not consider it an attachment.  I'm happy to work on a design such that signature can be saved however I don't think the signature qualifies as a regular attachment according to the questions in comment 2

I would suggest signatures are indicated in the message reader area and have some way of saving/working with them, perhaps similar to vcards or signed messages.

Otherwise it seems like we should start making the list and then do some testing to see if our assumptions are correct.

Comment 5

[WADA:World Anti-bad-Duping Agency]

FYI.
In bug 602718, new "View>>Show all parts as attachments" feature is being discussed, for generic/flexible mailformed/not-well-formed mail display, for user's convenience of saving any special part as local file. It'll allow greater flexibility of message reader design on "what is shown as attachment, what is not shown as attachment" and/or on "what part is mail body, what part is attachment, what part is other" .

Comment 6

[Jim Porter (:squib)]

I think we should probably keep .eml files shown as attachments. They seem no more or less "attachment-y" than any other attachment that can be shown inline. I suppose I can see some arguments for handling them differently via gloda, but for non-gloda applications, I think we should leave them like they are.

That said, I agree with the other cases.

Comment 7

[Jonathan Protzenko [:protz]]

Yeah it was never an option to not show .eml attachments, the original bug was that gloda was not treating these as attachments, which was a problem when generating the conversation metadata from the gloda information.

Comment 9

[Jonathan Protzenko [:protz]]

Changing the title to something more meaningful

Comment 10

[rsx11m]

Modifying again per comment #5 to reflect bug 602718 exception.

Comment 11

[Jonathan Protzenko [:protz]]

Attachment: Patch v1

So the patch does the following:
- each nsMsgAttachmentData now has a field that tells whether there was a real filename for this part or not,
- when encountering an attachment, in mimemoz2.cpp (NotifyEmittersOfAttachmentList), if there was no real name for that part, and unless we want to view all body parts, we don't notify the emitters for the attachment;
- when gloda indexes a message, if it turns out there's no real attachment for that message, then we set the property accordingly on the header, which means no more paperclip in the message list.

This also means newly indexed Gloda messages won't have any information about the Part 1.2-like attachments, but since messages indexed before that still hold the bogus information, I suggest to leave the test in http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/mimemsg.js#679

Patrick, I don't know how the Enigmail internals work, but it turns out you have a custom mime emitter and need to see some of these "Part 1.2"-like things pass by, it might break some stuff. I'm pretty confident this doesn't happen, though, so I suggest to land the patch on trunk, and see how it goes.

I also took the liberty of cleaning up Jonathan Kamen's patch, by making him not query the preference service in a loop, and initialize the pref only once, just like it's done in that file.

Comment 12

[Jonathan Protzenko [:protz]]

Oh and I forgot to explain that the gloda bit allows to remove the paperclip icon early. Without that, the paperclip icon only disappears once you've displayed the message. This is a detail, but improves the experience nicely imho.

Comment 13

[Jim Porter (:squib)]

I took a quick look at this; is there any reason for the changes in nsMimeBaseEmitter? It doesn't look like mDisplayHtmlAs is referenced anywhere.

Comment 14

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

So, to summarize my understanding of what happens with gloda:

- We will no longer be generating startAttachment notifications for things that were just body parts masquerading as attachments.

- This does not affect mimemsg's resulting representation because inside the call to startAttachment we were constructing a MimeMessageAttachment representation and invoking its isRealAttachment check.  If it said it was not a real attachment, we would not replace the part in our hierarchy, thereby acting like the call to startAttachment was never invoked.

(In reply to comment #11)
> This also means newly indexed Gloda messages won't have any information
> about the Part 1.2-like attachments, but since messages indexed before that
> still hold the bogus information, I suggest to leave the test in
> http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/mimemsg.
> js#679

Where do we persist the MimeMsg representation?  Are you thinking of the GlodaAttachment representation?

Assuming we don't persist it, it seems like we could cause the getter to always return true.

btw: you ideally want to be generating your patches with 8 lines of context, especially in this new world of the shredder review mechanism.  While I am sure my .hgrc settings are old and busted, putting the following in there can help assuming you are manually generating the patch:

[defaults]
diff=-U 8 -p
qdiff=-U 8 -p

Comment 15

[Jonathan Protzenko [:protz]]

Jim: right, thanks for spotting this.
Andrew: this would work assuming I change the condition for not emitting attachment information to (there's no real name && (as_html != 4 || opt->metadata_only == true)) (opt->metadata_only indicating we're dealing with the JS mime emitter).

That seems cleaner. I'll submit an updated patch. And yes, I did confuse MimeMsg with GlodaAttachment, my bad :-).

Comment 16

[Patrick Brunschwig]

Comment on attachment 547752 [details] [diff] [review]
Patch v1

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

The patch as such would work fine with Enigmail. The problem I have now is that MsgHdrToMimeMessage() cannot find embedded MIME parts after the message was decrypted. This means that messages following RFC 3156, section 6.1 (that is, messages doing RFC 1847 encapsulation) won't be verified anymore.

As said, this is not a problem of the patch itself. But due to the patch, my workaround to detect such an embedded message based on the presence of a fake attachment (the one for the signature) doesn't work anymore.

Comment 17

[Jonathan Protzenko [:protz]]

Can't you just walk down the MIME structure and use any part that has the right mime type? That seems easily feasible. You can easily test if (part instanceof MimeContainer) and then walk down part.parts...

Comment 18

[Patrick Brunschwig]

That's exactly what doesn't work. I do walk down the mime structure.

(A) The tree before decryption looks like this:
- Part1 (multipart/encrypted)
  - Part1.1 (application/pgp-encrypted)
  - Part1.2 (application/octet-stream)

(B) After the decryption the tree should look similarly to this:
- Part1 (multipart/encrypted)
  - Part1.1 (application/pgp-encrypted)
  - Part1.2 (multipart/signed)
  - Part1.2.1 (embedded original message) 
  - Part1.2.2 (application/pgp-signature)

However, the resulting tree I get using MsgHdrToMimeMessage() after the decryption is still (A).

Comment 19

[Jonathan Protzenko [:protz]]

So just to make things clear, without the patch, (A) has a fake attachment that allows you to figure out stuff, and after the patch, (A) doesn't have the fake attachment anymore? Have you filed a bug for that? I might be able to look into it...

Comment 20

[Patrick Brunschwig]

(In reply to comment #19)
> So just to make things clear, without the patch, (A) has a fake attachment
> that allows you to figure out stuff, and after the patch, (A) doesn't have
> the fake attachment anymore? 

That's correct

> Have you filed a bug for that? I might be able to look into it...

See bug 527927.

Comment 21

[Jonathan Protzenko [:protz]]

So the other option is to have only the HTML emitter skip unnamed attachments (hence the unused mHtmlAs), and still have the jsmimeemitter process them. This is a less-than-satisfactory solution, of course, so I'm going to try to fix bug 527927 if I can...

Comment 22

[Jonathan Protzenko [:protz]]

Since what the patch does is drop the paperclip in the message list when gloda, at indexing-time, figures out there's no real attachment associated with the message, we should make sure Gloda is positive there's no attachment. Currently, Gloda fails to figure out uuencoded attachments, detached attachments, and possibly feed enclosures (although I don't know what that is). Adding dependencies for the right bugs.

Comment 23

[Jonathan Protzenko [:protz]]

Attachment: Patch v2

This patch addresses the remarks found in former comments. I deleted a portion of a test that relied on an unnamed part being promoted to a MimeAttachment, which is not the case anymore.

Comment 24

[Jonathan Protzenko [:protz]]

The commenting out of half the tests from test_mime_attachments_size.js is of course not part of the patch.

Comment 25

[Jonathan Protzenko [:protz]]

Oh and also I'm probably going to have to write tests for that, but I'd like to make sure the approach satisfies everyone first.

Comment 26

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

Comment on attachment 550549 [details] [diff] [review]
Patch v2

yes, this is the right direction (given that you are addressing the encryption issues in bug 527927).  Yes, tests are required or please explicitly cite the existing tests that cover the things you called in comment 0.

Comment 27

[Jonathan Protzenko [:protz]]

Attachment: Patch v3, I can haz tests

Here's a new patch with tests. The bit that has been removed in test_mime_attachments_size is legitimate, because the invalid part is now (rightly) treated as a MimeUnknown part, because it has no name, so we shouldn't be expecting to have the bytes for these.

Two new tests have been added:
- one that makes sure that these very parts that used to be treated as attachments are now MimeUnknown parts,
- one that makes sure that gloda properly clears the Attachment flag on messages after they've been indexed (but only if they've been fully streamed).

Comment 28

[Jonathan Protzenko [:protz]]

Attachment: Patch v3b, I can haz qrefresh

Sorry, forgot to qrefresh...

Comment 29

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

Comment on attachment 550780 [details] [diff] [review]
Patch v3b, I can haz qrefresh

I like the new tests, but:

(In reply to comment #0)
> This is the followup to bug 554294. Basically, we'd like Gloda not to treat
> as attachments:
>  - .eml forwarded messages or quoted messages,
>  - vcards,
>  - footers,
>  - Part $X attachments

is explicitly calling out things that are not directly tested.  Perhaps if we added a flag to each of the attachments in the synthetic mime tree indicating whether they should be treated as a proper attachment or not and then have the tree checker check that?  You could probably remove the new test from that file, then.

Comment 30

[Jonathan Protzenko [:protz]]

I should've been clearer: the first bullet point is definitely wrong (attached or forwarded messages *are* attachments); I still think we should treat vcards as attachments; and footers are those of tb-planning which are called Part 1.2. So in the end, this patch only focuses on the Part 1.2-like attachments, which is exactly what the test is exercising.

Comment 31

[Jonathan Protzenko [:protz]]

Attachment: Update patch

As discussed, I've added an extra piece of test that makes sure that we're treating VCards as attachments.

Comment 32

[Jonathan Protzenko [:protz]]

http://hg.mozilla.org/comm-central/rev/5441c058d714

As part of larger libmime improvements, this removes "Part 1.2"-like attachments which *usually* don't make sense. In case anyone misses these attachments, the "Show all body parts" thing will allow you to go back to the old behavior.

Comment 33

[Tony Mechelynck [:tonymec]]

(In reply to Jonathan Protzenko [:protz] from comment #32)
> http://hg.mozilla.org/comm-central/rev/5441c058d714
> 
> As part of larger libmime improvements, this removes "Part 1.2"-like
> attachments which *usually* don't make sense. In case anyone misses these
> attachments, the "Show all body parts" thing will allow you to go back to
> the old behavior.

Where is that "Show all body parts" menuitem? From the above, I understood that it was a menuitem somewhere (probably under View) but I can find it neither in Shredder nor in SeaMonkey.

Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110810 Thunderbird/8.0a1

Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110810 Firefox/8.0a1 SeaMonkey/2.5a1 ID:20110810003133

Comment 34

[Tony Mechelynck [:tonymec]]

P.S. Shredder build ID: 20110810030009

Comment 35

[rsx11m]

Tony, please see bug 602718. It's in View > Message Body As, but you have to toggle mailnews.display.show_all_body_parts_menu first before it's available.

Comment 36

[Tony Mechelynck [:tonymec]]

(In reply to rsx11m from comment #35)
> Tony, please see bug 602718. It's in View > Message Body As, but you have to
> toggle mailnews.display.show_all_body_parts_menu first before it's available.

Hm, thanks for telling me. Not very «discoverable» IMHO.

Comment 37

[Tony Mechelynck [:tonymec]]

P.S. After checking, I have

mailnews.display.show_all_body_parts_menu   user set   boolean   true

in about:config (don't know whree it came from, AFAICT I didn't set it myself)

but the "View → Message Body As" menu has nothing else than
    * Original HTML
      Simple HTML
      Plain Text

That was in SeaMonkey. In Shredder it was false, and toggling it makes the menuitem appear immediately.

Comment 38

[rsx11m]

Yes, that's because the respective mail/ changes need to be ported to suite/ (which is SeaMonkey bug 677905 that was opened this morning).

Comment 39

[Tony Mechelynck [:tonymec]]

(In reply to rsx11m from comment #38)
> Yes, that's because the respective mail/ changes need to be ported to suite/
> (which is SeaMonkey bug 677905 that was opened this morning).

...by me (just before I noticed that I didn't see the menuitem in Shredder either). :-) Now let's just see if someone is willing to assign bug 677905 to himself. Later today I'll have a look at the diffs, and see if I think that I can do it, but until then I'm taking no bets: in the only bug I tried to fix until now I flopped lamentably and finally had to reassign it to Nobody. If anyone wants to take it, go ahead with my blessing.