561851 - Hack libmime so that it notifies attachment sizes to GlodaMimeAttachments

Status: RESOLVED FIXED

(MailNews Core :: Database, enhancement)

Description

[Jonathan Protzenko [:protz]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.4) Gecko/20100413 Firefox/3.6.4
Build Identifier: 

This a bug to track the progress of this feature. Basically, I'd need that for my extension

(see http://jonathan.protzenko.free.fr/gcv2.png)

There IS a way to do that already (see what this addon does for instance https://addons.mozilla.org/en-US/thunderbird/addon/878). This basically involves opening the nsIIOService and streaming the whole attachment to a listener that does nothing but count bytes. A little bit gory.

As per discussion on IRC, this should be feasible in a cleaner way.

<asuth> protz: it definitely seems useful
<asuth> and the information should be available-ish
<asuth> certainly libmime could be made to count the bytes without writing them over the emitter

Andrew, do you think you could take this?

Reproducible: Always

Comment 1

[Jonathan Protzenko [:protz]]

And I forgot to mention that bug 195702 is about this too, but in a more general context, so I don't think it's a duplicate strictly speaking.

Comment 2

[David :Bienvenu]

Do you want to display the size of the attachment in the mail message, or the size it will be on disk when you save it? I think the user will expect the latter, which means you need to either decode the attachment, or make a pretty good guess what the decoded size will be. For base64, you can do a reasonable calculation, but quoted printable will just be a guess...

Comment 3

[Jonathan Protzenko [:protz]]

The size in the message makes sense too: that represents the amount of data the user will have to download in order to save the attachment. I wouldn't be shocked if it were the first size you mention.

Comment 4

[David :Bienvenu]

Most likely, the message will already be downloaded (pop3, and imap messages already in the offline store).

Comment 5

[David :Bienvenu]

An in fact, if libmime has streamed the message, we've definitely downloaded it.

Comment 6

[Jonathan Protzenko [:protz]]

I have absolutely zero knowledge of libmime, but as it seems to stream the whole message, can't we make it count the base64 bytes as we go and compute the size from it?

Comment 7

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

(In reply to comment #0)
> Andrew, do you think you could take this?

It's a possibility after 3.1 is shipped; I'm going to be focusing on performance and correctness issues until then.

Comment 8

[Jim Porter (:squib)]

I could use this too, for bug 195702 (and bug 559559).

Comment 9

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

protz pinged me on IRC about the state of this but is gone from there now...

This is not on my radar; it might end up sorta happening if I write a JS-based mime parser, but otherwise no.  Patches of course likely accepted if they come with delightful unit tests.

Comment 10

[Jonathan Protzenko [:protz]]

Attachment: WIP

Ok, taking. I've spent two days digging through the code of libmime, and that's what I've come up with.
i) It only works for images. I guess I must set some flag so that the libmime parser actually decodes all kinds of attachments when it's streaming to the js mime emitter, but I don't know (yet) how to do so. Advice is welcome.
ii) This means it doesn't work for attached .eml messages which are also containers. Probably harder to fix than i)
iii) I didn't change any return value from any of the functions from libmime, for fear of breaking something. Instead, I chose to pass pointers to integers.
iv) I did change the nsMsgAttachment struct.

This might benefit bug 559559 and bug 195702 although I'm not interested in taking these.

Please note that because of mqueue management failure on my side, this also includes the patch (soon-to-be-checked-in) from bug 576282.

I'll keep working on it, but any feedback would be appreciated :).

Comment 11

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

Comment on attachment 464650 [details] [diff] [review]
WIP

you can use interdiff (from patchutils) to reverse your queue snafu.

From a skim, I don't get why you are returning size deltas of 1 when nothing is written to a buffer.  If this is intentional, you really need to comment on it.  And in the cases where you use the ternary operator to specifically return 0, you could just be returning the subtraction to get the same result.  (Unless people are setting the pointers to dangerous values before the start of the buffer?)

In terms of understanding what libmime is actually getting up to, you may want to check our my blog post where I was in similar horrible straits:
http://www.visophyte.org/blog/2009/03/15/understanding-libmime-using-chroniquery-and-unit-tests/

It has example logs of using chroniquery to figure out just what libmime was up to including example traces like so:
http://clicky.visophyte.org/examples/chroniquery/20090315-03/trace-621889723.html

chronicle-recorder and accordingly chroniquery should work on OS X if you need additional runs.  Of course gdb now has working tracepoints so you could probably also get quasi-usable traces that way too.  (and/or just breakpoints where the associated command continues execution...)

Although I'm not going to compel you to try and use my tool, please reference specific MIME body structures (preferably already existing in the unit tests or new ones you add) that are failing to generate the size output as it simplifies my own libmime spelunking.

Generally speaking, the strategy seems fine to me.  I do think it is important that we only generate additional functional call traffic to count size when it is so requested and definitely never generate any XPConnect traffic.  Which is to say, writes of the attachments we are only processing for sizing information must never hit the emitter.  This can probably be best accomplished by modifying the JS mime emitter in the general write case that is not supposed to happen set a flag on the content object like "illegalWriteHappened" and have the unit tests check for that (being careful that the attachment object swap-out does not discard the flag.)

Comment 12

[Jonathan Protzenko [:protz]]

Attachment: WIP (2)

I think I have it working (phew!). I've written a unit test as well for it, testing different sizes of attachments. They all return the right size, which is comforting. I added an extra option called "stream_all_attachments" that makes sure that when we're talking to the js mime emitter, we actually decode all kinds of attachments to make sure we are able to compute their size.

I plan on also modifying Gloda attributes to include attachments urls and sizes (most wanted for thunderbird air), but that's something I need to discuss with you before going for it.

Comment 13

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

Comment on attachment 465010 [details] [diff] [review]
WIP (2)

Reviewing/sanity checking patches is a lot easier when I can use my reviewboard tool against the patch.  Since this patch appears to be on top of another patch in your queue (thanks for factoring that previous patch back out if that's what happened!), the assumption that it is not against trunk doesn't work out.  So when possible (and not just for complainy people like myself), it's preferable to have the patch be against trunk.  Frequently this can be accomplished with little effort by popping all the other patches on your queue, "qup"-ing this patch, and then doing your diff/export/what not.  Sometimes (when changes happen within 3 lines of context) your patch may require minor changes.

Another alternative that makes my reviewboard happy is if you include a link to a changeset you pushed to the try server; that works even when the specific patch is built on top of a number of other patches.  It also has the nice side effect of letting reviewers more easily check that your patch passes tests (or will, very shortly, with our new try server.)

My main helpful comment is that you don't need to actually store the data for a test inside the javascript file, so the 1000+ lines of encoded PDF could actually live in an external file that we load/import on demand.  Of course, we could probably also synthetically generate an encoded file consisting of multiple lines quite easily.  And since libmime has 4 encoded cases, you are going to want test cases covering all 4 encodings which is perhaps an additional vote in favor of just dynamically creating those encodings.

Comment 14

[Jonathan Protzenko [:protz]]

Attachment: WIP (3)

Third take. This patch works fine as far as I can tell. I wrote a test that checks the correctness of byte-counting for:
- text attachments
- text attachments with content-disposition: inline
- uuencode attachments
- quoted-printable attachments
- base64 attachments
- non-encoded attachments
- image attachments (different behavior).

This test does not cover:
- yencode attachments (took me 3 hours to figure out how to create a properly crafted message, long story short, I had to prevent the message injection framework from adding a content-type header): they do not end up in the gloda attachments; plus, they're only used for newsgroup messages and gloda doesn't support that case. However, the attachment size is properly computed. It's just that I have no easy way of testing it since they don't appear in the list of gloda attachments (I think that's because they don't have a content-type).

- inline images -- their size is not even computed properly (always returns 0), but I don't feel like hacking even deeper into libmime just to make that work. The interest is very limited imho and they're not even considered attachments neither by the message reader or gloda, so I can't possibly figure out why we'd like to know their size.

Andrew, I also short-circuited all calls to MimeObject_write for images and attachments, to make sure we don't fall into the "aaaarrgh this should not be happening" case where we do a roundtrip through XPConnect. I'm fairly confident my changes are OK. However, I've been unable to reproduce, so I'd happily accept some hints if you know how to reproduce the situation.

All printfs will be removed before the final commit, it's just that I'd like to keep them in case I need to adjust a little bit.
I think this patch has reached quite a good state. I don't think it's worth spending 72 more hours on this just to get the 10% remaining cases working. IMHO, you can consider this as a tentative final version of the patch.

Comment 15

[Jonathan Protzenko [:protz]]

Just to give more context: MimeObject_write is located at http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimei.cpp#1723 and it calls MimeOptions_write... which contains this line http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimei.cpp#1684 that actually makes the roundtrip call to XPConnect (and matches Andrew's comment about libmime not being happy if that calls doesn't return a value > 0).

opt->output_fn is defined to be mime_output_fn in mimemoz2.cp. mime_output_fn is the function that actually does the call to JS.

http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#878

So I basically disabled the calls in the two subclasses of MimeLeaf that might call WriteBody: MimeInlineImage and MimeExternalAttachment.

Please correct me if I'm wrong :).

Comment 16

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

Comment on attachment 465492 [details] [diff] [review]
WIP (3)

Can't give r with all the debug still in.  Generally looks good and you deserve some form of prize for making it out the other side of libmime without going crazy.  Some specific notes below.

If you don't have 'try' commit access (level 1), please request it and cite me as the voucher, and then make sure you push to comm-central try before the next review step.  See here:
http://www.mozilla.org/hacking/commit-access-policy/


on file: mailnews/db/gloda/components/jsmimeemitter.js line 350
>     this._curPart = partMatch && partMatch[1];

this._curPart is an object everywhere else; having it be a string for this
phase is confusing.


on file: mailnews/db/gloda/components/jsmimeemitter.js line 434
>     dump("JSMIMEEMITTERWRITEOMGOMGOMG\n");

hehe :)

I was thinking we could set a flag on the root message produced and the unit
test could check for that flag and explode.  Much better at detecting
regressions than putting this back in periodically.


on file: mailnews/db/gloda/test/unit/test_mime_attachments_size.js line 16
>  * The Initial Developer of the Original Code is
>  * Mozilla Messaging, Inc.

I realize you're just copying but this should be "is the Mozilla Foundation".



on file: mailnews/db/gloda/test/unit/test_mime_attachments_size.js line 22
>  *   Andrew Sutherland <[e-mail]>

and this should probably be you.

Comment 17

[Jonathan Protzenko [:protz]]

Attachment: WIP (4)

New version of the patch. Comments have been addressed, and a "size" getter has been added to MimeMessage, MimeContainer and MimeBody in mimemsg.js so that the interface is unified. That way, one can get the total size of a message.

While waiting for my credentials to be approved, could someone please push this to try-comm-central? Then, I'll request r and sr.

Comment 18

[Jonathan Protzenko [:protz]]

Andrew, concerning the "aaaargh this should not be happening", I have at least one reproducible case (spoiler: any news message), so I plan on creating a new bug to specifically write a test for that (since it's not directly related to the work I'm doing here).

Comment 19

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

Newsgroup messages aren't expected to work.  It's a bug if any in-tree code tries to use the JS mime emitter to stream them at this point.  I realize the documentation fails to disclose that fact so out-of-tree code might trigger such things; feel free to add a guard and documentation to the code in a new bug if you like.

You can just ask for review now; since the try server won't run the xpcshell tests we'll need to locally build and test too anyways.

Comment 20

[Jonathan Protzenko [:protz]]

Comment on attachment 466148 [details] [diff] [review]
WIP (4)

Requesting s and sr, since this touches a critical part of the code. And yes, you're right, I'd forgotten about the tryserver not running xpcshell tests.

By the way, unrelated question: how do I make sure that bugzilla shows the nice diff UI with colors and all instead of plaintext when I click on a patch's name?

Comment 21

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

Comment on attachment 466148 [details] [diff] [review]
WIP (4)

Looking very good.  Please make sure you re-run your xpcshell tests once you
correct the bit about some of the cases not apparently being tested...  As
long as that passes, and the try server mozmill tests don't explode (please keep an eye on TBPL at http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry ), r=asuth.


on file: mailnews/db/gloda/components/jsmimeemitter.js line 387
>     if (aField == "X-Mozilla-PartSize" && this._curPart)
>         this._partMap[this._curPart.partName].size = parseInt(aValue);

Urgh, reviewboard ate my epic prose on this one.

Bottom line, attachments are notified in a second phase of the streaming
process after all the body parts are notified (as they are streamed) and
this._curPart is not something you can use.  My previous complain about your
use of this._curPart was that you were reusing the same attribute name in a
different way than it was originally used, not that you were persisting info
about what attachment you were hearing about.  Feel free to use
_curAttachmentName or something to propagate the state.

Also, this probably only worked because your unit test has commented out the
multiple case situation.


on file: mailnews/db/gloda/modules/mimemsg.js line 463
>   get size () {
>     return [child.size for each ([, child] in Iterator(this.parts))]
>       .reduce(function (a, b) a + b, 0);
>   },

If you are going to return -1 sometimes, your summing code needs to treat -1's
as 0's... please correct here and for the other duplicates...


on file: mailnews/db/gloda/test/unit/test_mime_attachments_size.js line 172
>     attachments: [tachImage/*, tachPdf, tachUU,
>       tachApplication, tachText, tachInlineText,*/],

this seems concerning; please make the other test cases happen...


on file: mailnews/mime/src/mimeeobj.cpp line 260
>     return 1;

same deal on the return value; 0 or explain.


on file: mailnews/mime/src/mimeiimg.cpp line 227
>     return 1;

as discussed on IRC, please return 0 unless there's a good reason for
returning 1, and if there is, please add a brief comment to indicate why since
there is no enum to make it clear.  (ex: "tell caller to not call this part
again").

Comment 22

[Jonathan Protzenko [:protz]]

Attachment: WIP (5)

This patch addresses all previous comments. Still needs sr=bienvenu.

Comment 23

[David :Bienvenu]

very cool. In general, looks OK, modulo asuth's prev comments, and these nits:

+    // the url should contain a part= piece that tells us the part name, which
+    //  we then use to figure out where.

where what?

+ * Portions created by the Initial Developer are Copyright (C) 2008

2010

+  if (outSize != nsnull)
+    *outSize = out - buffer;

just if (outSize) (several places)

+  /* Don't do a roundtrip through XPConnect when we're only interested in
+   * metadata and size. 0 means ok, the caller just checks for negative return
+   * value */

should use
/**
 *
 */

form for this kind of comment.

Comment 24

[David :Bienvenu]

Comment on attachment 466396 [details] [diff] [review]
WIP (5)

sr=me, modulo outstanding comments.

Comment 25

[Jonathan Protzenko [:protz]]

Attachment: Final take, good for commit

Comments addressed, this is good for checkin.

Comment 26

[Mark Banner (:standard8)]

Checked in:

http://hg.mozilla.org/comm-central/rev/00b28751b20d

I also realised (just after pushing) that you'd hadn't updated the uuid in the idl file - this needs changing even for additions so that binary extensions can detect changes correctly - I pushed an additional fix so we don't forget it:

http://hg.mozilla.org/comm-central/rev/56e03772b272

Comment 27

[Mark Banner (:standard8)]

I had to back this out because I couldn't figure out the unit test failures on Windows:

TEST-PASS | e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/test_mime_attachments_size.js | [check_attachments : 187] 174 == 174
*** Attachment now is funky.funk
NEXT ERROR TEST-UNEXPECTED-FAIL | e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/test_mime_attachments_size.js | 175 == 174 - See following stack:
JS frame :: e:\buildbot\win32-comm-central-check\build\mozilla\testing\xpcshell\head.js :: do_throw :: line 317
JS frame :: e:\buildbot\win32-comm-central-check\build\mozilla\testing\xpcshell\head.js :: do_check_eq :: line 347
JS frame :: e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/test_mime_attachments_size.js :: check_attachments :: line 187
JS frame :: e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/test_mime_attachments_size.js :: anonymous :: line 206
JS frame :: resource:///modules/gloda/mimemsg.js :: anonymous :: line 134
TEST-INFO | (xpcshell/head.js) | exiting test
TEST-UNEXPECTED-FAIL | e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/test_mime_attachments_size.js | 2147500036 - See following stack:
JS frame :: e:\buildbot\win32-comm-central-check\build\mozilla\testing\xpcshell\head.js :: do_throw :: line 317
JS frame :: e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/test_mime_attachments_size.js :: anonymous :: line 208
JS frame :: resource:///modules/gloda/mimemsg.js :: anonymous :: line 134
TEST-INFO | (xpcshell/head.js) | exiting test
before 360448, after 352256, break 00000000

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1282031113.1282035187.23996.gz#err1

http://hg.mozilla.org/comm-central/rev/1fc0ca093d0e
http://hg.mozilla.org/comm-central/rev/f444a2daf464

Comment 28

[Jonathan Protzenko [:protz]]

NEXT ERROR TEST-UNEXPECTED-FAIL |
e:/buildbot/win32-comm-central-check/build/objdir/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/test_mime_attachments_size.js
| 175 == 174

Looks like libmime decodes an extra byte on Windows. How I hate you libmime!

I suggest replacing the equality check

    do_check_eq(att.size, originalText.length + 1);

with

    do_check_true(Math.abs(att.size - originalText.length - 1) <= 1)

thoughts?

(Too bad the tryservers don't run xpcshell tests, they probably would've spotted that).

Comment 29

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

I figure it's a newline thing; might be better to count the newlines or something like that and have an explicit comment.

Comment 30

[Jonathan Protzenko [:protz]]

Attachment: Libmime strikes back (new patch)

I don't feel like fixing the core issue inside of libmime.
i) I'm probably going to break everything because that will require me to fiddle with the decoding and somehow make it understand that it's got to skip the count for the last newline.
ii) This probably implies a lot of interactions inside libmime because the decoders are not aware of the whole attachment mime part, there's only line parsing methods
iii) I'm not interested in a very accurate count, and having an offset of at most two bytes is ok for me

So I've just made a workaround and I've documented accurately the issue. I'm still waiting for asuth to run the xpcshell tests for this on his Windows box, and then I'll re-request commit.

PS: interface change is included in that patch

Comment 31

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

TEST-UNEXPECTED-FAIL | c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglob
aldb/unit/test_mime_attachments_size.js | 174 == 175 - See following stack:
JS frame :: c:\rev_control\hg\comm-central\mozilla\testing\xpcshell\head.js :: do_throw :: line 317
JS frame :: c:\rev_control\hg\comm-central\mozilla\testing\xpcshell\head.js :: do_check_eq :: line 347
JS frame :: c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/t
est_mime_attachments_size.js :: check_attachments :: line 194
JS frame :: c:/rev_control/hg/comm-central/objdir-thunderbird-debug/mozilla/_tests/xpcshell/test_mailnewsglobaldb/unit/t
est_mime_attachments_size.js :: anonymous :: line 213
JS frame :: resource:///modules/gloda/mimemsg.js :: anonymous :: line 134

Comment 32

[Jonathan Protzenko [:protz]]

Attachment: With a test that actually works

r=asuth
sr=bienvenu

Just changed the test to take into account the discrepancy between Windows and other OSes. This can be checked in.

Comment 33

[Jonathan Protzenko [:protz]]

Attachment: Fix a typo

This patch fixes a minor typo from the previous patch.

Comment 34

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/80a99f095369

Comment 35

[Jonathan Protzenko [:protz]]

Attachment: Fix random errors

The patch actually works fine but in some tricky cases (i.e. running it on a real inbox) it throws exceptions, which this patch fixes.

Comment 36

[Jonathan Protzenko [:protz]]

Reopening since we can't really consider this FIXED because of the errors.

Comment 37

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

Comment on attachment 468759 [details] [diff] [review]
Fix random errors

This has a bad smell but works.  Thank you for including the descriptive comment!

Comment 38

[Jonathan Protzenko [:protz]]

I guess the clean way would just be putting a setter on MimeMessage that just ignores the value it's passed. Would that make you feel more comfortable?

Comment 39

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

That seems less ugly, yes.  And migrate/change your comment to be on the setter then.  Sounds good!  r=asuth for the changed patch too.

Comment 40

[Jonathan Protzenko [:protz]]

Attachment: Better fix

r=asuth

Interestingly enough, this doesn't show up on comm-central, which is why I hadn't spotted the error before. Probably some quirk in the JS engine, I'm not interested in knowing the details. But since this is going to end up in 3.2, the error probably would have showed up there, so fixing now.

Comment 41

[Jonathan Protzenko [:protz]]

Attachment: Companion patch

Forgot to qrefresh

Comment 42

[Mark Banner (:standard8)]

(In reply to comment #41)
> Created attachment 468835 [details] [diff] [review]
> Companion patch

Checked in: http://hg.mozilla.org/comm-central/rev/685069b46631

Comment 43

[Jonathan Protzenko [:protz]]

Adding dependency information so that when the time comes to check these into 3.2, we get the dependency chain right.

Comment 44

[Ludovic Hirlimann [:Usul]]

I'm seeing 

  Error: this._partMap[this._curAttachment] is undefined
  Source File: file:///Applications/Shredder.app/Contents/MacOS/components
  /jsmimeemitter.js
  Line: 393 

with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b7pre) Gecko/20100922 Thunderbird/3.3a1pre

Comment 45

[Jonathan Protzenko [:protz]]

I think this is a case of "we have a part that the jsmimeemitter doesn't think is an attachment, really (like, say, Part 1.2)" but "I am libmime and I think this is an attachment so I'm sending PartSize in your face".

if (aField == "X-Mozilla-PartSize" && this._curPart)

The test for this._curPart is obsolete and is a leftover of a previous version of the patch, so I suggest changing this into

if (aField == "X-Mozilla-PartSize" && this._curAttachment in this._partMap)

I don't see any other rational explanation... Andrew, thoughts?

Comment 46

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

works for me, rs=asuth

Comment 47

[Jonathan Protzenko [:protz]]

Attachment: Followup fix to some wicked cases -- be paranoid

This fixes the bug and has review from asuth as per the previous comment.

Comment 48

[Mark Banner (:standard8)]

(In reply to comment #47)
> Created attachment 478738 [details] [diff] [review]
> Followup fix to some wicked cases -- be paranoid
> 
> This fixes the bug and has review from asuth as per the previous comment.

Checked in: http://hg.mozilla.org/comm-central/rev/d9d42538b019