606699 - The new "count the size of attachments" feature does not count bytes for .eml attachments

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Jonathan Protzenko [:protz]]

This is basically due to the fact that inline .eml parts are treated just like regular MIME parts, so they don't go through the codepath that decodes attachments and counts the bytes (I suspect we don't go through mimeeobj.cpp in that case).

Gloda exposes a correct representation (see bug 576282) because the JS MIME representation only trusts the sizes provided by libmime for "hard" attachments (i.e. images, pdfs, etc.). Attached .eml are actually no MimeAttachment instances, but rather MimeBody instances (see mailnews/db/gloda/modules/mimemsg.js), the size method of which just returns the length of its text part + the sum of its subparts sizes.

This hits us in bug 559559, because attached eml files have a size of -1 (as expected from the comments in libmime). So the UI code is clever enough not to display a 4GB size, but we should definitely try to count the sizes of attached .eml files.

Comment 1

[Jonathan Protzenko [:protz]]

This is going to be especially painful since attached .eml are actually containers so their size is the cumulated size of their subparts + their body(ies).

Comment 2

[Jonathan Protzenko [:protz]]

Attachment: First take

This is a WIP. Jim, I have trouble adapting your test file to the last case I added. The thing is, we have an email with an attached email. The attached email also contains an attachment (an image). We have the following notifications sent to gMessageHeaderSink through addAttachmentField:
- addAttachmentField with size = the size of the attached message, including its body and its attached image
- addAttachmentField with size = the size of the image in the attached message

The second call overrides the previous (correct) size value, and the onStopRequest is called, with the wrong size stored in gMessageHeaderSink.

I don't really know if this is ok. If this is not the case, then the logic is wrong, and we should either
- fix libmime to not emit notifications for the inner attachment (probably annoying) or
- fix the logic in the test and in msgHdrViewOverlay.js to use deal with these situations. This probably implies not overriding the size if it's non-null.

Thoughts?

Comment 3

[Jim Porter (:squib)]

(In reply to comment #2)
> I don't really know if this is ok. If this is not the case, then the logic is
> wrong, and we should either
> - fix libmime to not emit notifications for the inner attachment (probably
> annoying) or

Instinctively, I'd prefer this method, but I can see some possible reasons why you'd care about nested attachment info. I'm not really sure if that matters in practice, though. It's probably reasonable to update msgHdrViewOverlay.js and leave a comment saying that attached .emls with attachments can emit multiple size fields and that you just have to be careful.

Comment 4

[Jonathan Protzenko [:protz]]

I just realized that the js mime emitter relies on these nested attachment infos, because it only cares about these nested attachments when indexing messages. I guess I'll probably implement a solution similar to that of the js mime emitter, which consists in keeping a mapping from mime parts to attachments in msgHdrViewOverlay.js.

Comment 5

[Jim Porter (:squib)]

Let's do the fix in msgHdrViewOverlay.js, then. I don't know if we need to go all out and actually map parts to attachments just yet (though it may be useful one day). I think just ignoring all but the first "size" field will be sufficient for now, unless you have something specific in mind.

Also, regarding the attached patch, what's this comment in reference to: "I don't think it will work for external objects". Is that for detached attachments? Other than that, things seem ok, though I haven't actually compiled with the patch or anything... :)

Comment 6

[Jonathan Protzenko [:protz]]

Attachment: Fixes the test file, and makes sure the size is recursively computed properly.

I believe the patch is right, and I managed to make the test right too, so it should all be good. Some caveats, though:
- there seems to be a bug in my implementation of size for the MimeMessage representation, so there will be some inconsistencies between what's shown for the conversation view and what's shown for the classic message reader
- if message A has message B attached (.eml) and message B contains an image attachment, viewing message A will flatten attachments: message A's attachments will be message B and the image as well.

The image will have size N, and the message will have size N + message B's body size. That might seem kind of weird, but that's the way it's done currently. Now I just need to investigate to see whether I can find out why MimeMessages have a weird size...

Comment 7

[Jonathan Protzenko [:protz]]

There's no bug, the inconsistency is just due to the fact that Gloda enforces MAX_SANE_BODY_SIZE and stop reading bodies after 20k bytes, and my test message had an insanely big body, so everything's fine =).

Comment 8

[Jim Porter (:squib)]

Comment on attachment 501290 [details] [diff] [review]
Fixes the test file, and makes sure the size is recursively computed properly.

Shouldn't this include the changes in msgHdrViewOverlay.js that you mentioned earlier?

Comment 9

[Jonathan Protzenko [:protz]]

Er, no, the only real issue here was that we were not recursively computing the length of .eml attachments. Notifications come in order, i.e. HandleAttachment is always followed by AddAttachmentField, the calls are not nested.

Comment 10

[Jim Porter (:squib)]

Does this mean that nested attachments will all be displayed in the attachment pane in the message reader? Do we actually want that, or should we prevent it from happening?

Comment 11

[Jonathan Protzenko [:protz]]

It looks this is what is happening if I double-click on the attached .eml, and that .eml contains another nested .eml with inner attachments. That's very weird, looks like a specific case to me, and I don't the patch changes this behavior. Probably some libmime-specific quirk, I don't think we want to tackle this issue... (but that's the lazy me talking).

Comment 12

[Jim Porter (:squib)]

I've thought about this a bit, and I think it would be good to get an outside opinion (e.g. from a Thunderbird driver), since I'm really not 100% sure what we should be doing here.

:asuth, what are your opinions here?

Comment 13

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

In the big picture, there are three possible sizes we might care about: size on server, size occupied locally in the mail store, size of the payload file when we extract it and drop it on the disk.  From a user expectation perspective, I think they would expect the extracted size on disk.  I believe this is what we try to give them.

I have come to grips of the limitations of the current message reader.  I think in the complex hierarchical case it would be neat if we had a tree-view or something which could express to the user what the size of the message with its attachments is as well as the break-out of those specific attachments.  Given that we don't build a tree representation (and I don't think we should bother with that right now either), I think things are going to be confusing no matter what.

Accordingly, my opinion is that tallying the sizes is a large improvement over -1.  If we could add in the tallied body size, that would be even better, but the patch that currently exists is a solid improvement and looks fine to me.

r=asuth assuming it doesn't break the gloda tests.

PS: I don't think I'm actually a driver, but I am a peer for this sub-module.

Comment 14

[Jonathan Protzenko [:protz]]

Comment on attachment 501290 [details] [diff] [review]
Fixes the test file, and makes sure the size is recursively computed properly.

http://hg.mozilla.org/comm-central/rev/9093f7af1615

r=asuth