790852 - Make nsIMimeHeaders use the new MIME parser

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Joshua Cranmer [:jcranmer]]

nsIMimeHeaders is a relatively self-contained interface. All it does is just parse the header portion of an RFC 822 message (or MIME subpart) and let you get the value(s) for a particular header. Why not make this interface use the new MIME parser?

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Clean up the interface

This part doesn't actually depend on the new MIME interfaces; it's just a cleanup of the interface that is badly needed.

ACString is more correct than AUTF8String here since the charset of a mail message is unknown and not necessarily UTF-8. Since all of the output data from the interface comes from the input, we preserve all charset data. RFC 2047/RFC 2231 decoding happens via other interfaces. There was a change several months ago which stopped xpconnect from complaining about the presence of non-7-bit chars in an ACString (and string, IIRC) conversion going to and from JS, which means \x00-\xff in C++-land convert to \u0000-\u00ff in JS land and vice versa (JS chars above \u00ff still get complaints IIRC).

I *think* I have the string ownership models of XPCOM correct here, but given that part 2 rips this out for a JS model, I didn't look at everything too hard.

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2: Use the new MIME parser for this interface instead

No review request yet, I want to wait for bug 746052 to finish first, and I have some other cleanup I want to do.

This patch comes with a slight semantic modification which unfortunately does impact tests. See bug 746052 comment 34 for more details.

Comment 3

[[e-mail]]

Comment on attachment 660676 [details] [diff] [review]
Part 1: Clean up the interface

>-  void initialize([const] in string allHeaders, in long allHeadersSize);
>+  void initialize(in ACString allHeaders);
[Extra arguments are ignored, so assuming JS consumers pass the length in anyway, it will make no difference.]

>+    return static_cast<nsresult>(MimeHeaders_parse_line(flatString.get(), flatString.Length(), mHeaders));
Please use aAllHeaders.BeginReading()/.Length() instead.

>+  retval.Adopt(MimeHeaders_get(mHeaders, headerName, false, allOfThem));
Unfortunately the external API doesn't allow an nsACString& to Adopt(). Please either assign and free or Adopt into a temporary string. sr=me with that fixed.

>+            mimeHeaders->Initialize(nsDependentCString(
>+              (*workHeaders)->all_headers, (*workHeaders)->all_headers_fp));
nsDependentCString requires a null-terminated string; I'm not sure that you can guarantee that here, so please use Substring() instead.

Comment 4

[Kent James (:rkent)]

Comment on attachment 660676 [details] [diff] [review]
Part 1: Clean up the interface

Looks good to me. Neil answered all of my questions in his review.

Comment 5

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

Is there a simple torture test you can run to see what the memory usage impact is from moving a reasonably low-level API that did not involve GC to something that will involve GC?  Although the good news is that the cycle collector needn't get involved because this is native code calling into C++, I think it's prudent to have an idea of the impact.  Also, comparing the CPU time accumulated for the process between running with this and without this would be informative, as the GC's will not be without cost.

Comment 6

[Joshua Cranmer [:jcranmer]]

(In reply to Andrew Sutherland (:asuth) from comment #5)
> Is there a simple torture test you can run to see what the memory usage
> impact is from moving a reasonably low-level API that did not involve GC to
> something that will involve GC?  Although the good news is that the cycle
> collector needn't get involved because this is native code calling into C++,
> I think it's prudent to have an idea of the impact.  Also, comparing the CPU
> time accumulated for the process between running with this and without this
> would be informative, as the GC's will not be without cost.

Message headers are small (most messages only have 1-5KB [1] of data, depending on how much gunk MTAs add to the message, and the typical header extraction is less than 70 characters). Looking at where we have users, there are the following:
1. An ad-hoc MIME parser I want to eliminate anyways (nsMsgDBFolder)
2. Compose message extraction
3. Message header display
4. MDN notification generation
5. Windows search indexing

Two of these (3 and 5) are already in JS, so the added GC structures would probably be negligible. The others (AIUI) are going to be attached to processes that require I/O first, so extra GC costs should be lost in the noise of I/O scheduling and the memory costs are probably on the order of one or two pages.

Given these constraints, probably the fairest test is timing how long mozmill tests that exercise message header UI, but it doesn't look like we have any, and mozmill tests are rather noisy in terms of how long they take.

[1] Note to self: don't extrapolate header size from newsgroup messages. NNTP servers are much nicer about not munging headers than MTAs...

Comment 7

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Clean up the interface

Uploading an updated version of the patch, since the original one bitrotted a bit.

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2: Use the new MIME parser for this interface instead

Updated version of this patch for review.

Comment 9

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

Comment on attachment 706424 [details] [diff] [review]
Part 2: Use the new MIME parser for this interface instead

I'm not going to be able to get to this in a timely fashion and provide the appropriate level of review and testing this requires.  Removing myself from review for now, especially since you have another reviewer.

Comment 10

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 706424 [details] [diff] [review]
Part 2: Use the new MIME parser for this interface instead

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

Clean up the patch comment, everything else is nice.

Comment 11

[Joshua Cranmer [:jcranmer]]

Patches pushed: <https://hg.mozilla.org/comm-central/pushloghtml?changeset=b0ee9c28b03a>.