630011 - Make sure messages with MimeUnknown parts don't break recursive MimeMessage size computations

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Jonathan Protzenko [:protz]]

Attachment: A one-liner that fixes the issue, yay!

MimeUnknown parts break their parent container's size computations because their size property is undefined, which means the parent computation returns NaN, which means we're screwed. This patch just fixes this, and enhances the pretty printing of MimeMessage instances while we're at it.

Please note that we still don't really know how to record the size of MimeUnknown parts, so in the case of the message gNeandr sent me, the standard message reader count all bytes, regardless of the underlying Mime nature of the parts, which means it includes the size of the MimeUnknown part for the total figure.

Thunderbird Conversations, on the other hand, uses the MimeMessage abstraction, and fails to take the size of the MimeUnknown part into account for the total size. I think we can live with that.

Comment 1

[Jonathan Protzenko [:protz]]

Attachment: A one-liner that fixes the issue, yay!

Forgot to tell bugzilla this is a patch, looks like the only solution is to re-upload the attachment.

Comment 2

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

Comment on attachment 508222 [details] [diff] [review]
A one-liner that fixes the issue, yay!

This feels like the kind of thing that wants a unit test... (more so because it sounds like we need MimeUnknown coverage than this specific change is of dire importance.)

nit: Those pluses should really have spaces around them; if you are fighting 80 columns, just spill to a new line.

quasi-nit: You have a hunk in jsmimeemitter thad adds squigglies for no clear reason; presumably you tried adding code there earlier in the patch and the removed it again?  You can leave the squiggles if you want, I don't care.

Comment 3

[Jonathan Protzenko [:protz]]

Attachment: Snapshot of the patch

So this patch adds tests for the two important cases:
- when the JS mime emitter is able to attach a size to the MimeUnknown instance, and
- the case where it's not able to attach a size because libmime won't emit the individual notifications (means the corresponding part is actually not an attachment).

There's a big chunk of comments where I discuss why the second case happens, how we end up with a MimeUnknown instance whose size we don't know, and how we could possibly workaround it, and why I think it's not worth it to just take into account buggy messages (hasn't happened frequently to me...).

As I said on IRC, I'm interested in broad comments on the patch. I just finished it, which means I'll probably polish it after a good night's sleep, so don't take that as final :-).

Comment 4

[Jonathan Protzenko [:protz]]

Attachment: A better patch

This patch makes the test better by not injecting a raw mbox string into the folder, but rather by recreating the weird structure through SyntheticParts and streaming it through the usual process. This allowed me to make the test more understandable, and hopefully to exercise the MimeUnknown stuff a little bit more.

I'd be glad to address any review comments! :-)

Comment 5

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

Comment on attachment 512788 [details] [diff] [review]
A better patch

Sorry, no review comments!  Thanks for the pretty test!

Comment 6

[Jonathan Protzenko [:protz]]

Cool. Checked-in.

http://hg.mozilla.org/comm-central/rev/cb713ede0c5c