Last Comment Bug 790852 - Make nsIMimeHeaders use the new MIME parser
: Make nsIMimeHeaders use the new MIME parser
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 21.0
Assigned To: Joshua Cranmer [:jcranmer]
:
Mentors:
Depends on: 746052
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-12 19:58 PDT by Joshua Cranmer [:jcranmer]
Modified: 2013-02-04 03:21 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Part 1: Clean up the interface (25.45 KB, patch)
2012-09-12 20:14 PDT, Joshua Cranmer [:jcranmer]
rkent: review+
neil: superreview+
Details | Diff | Review
Part 2: Use the new MIME parser for this interface instead (13.12 KB, patch)
2012-09-12 20:16 PDT, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Review
Part 1: Clean up the interface (24.34 KB, patch)
2013-01-25 09:08 PST, Joshua Cranmer [:jcranmer]
Pidgeot18: review+
Pidgeot18: superreview+
Details | Diff | Review
Part 2: Use the new MIME parser for this interface instead (14.18 KB, patch)
2013-01-25 09:13 PST, Joshua Cranmer [:jcranmer]
irving: review+
Details | Diff | Review

Description Joshua Cranmer [:jcranmer] 2012-09-12 19:58:49 PDT
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] 2012-09-12 20:14:20 PDT
Created attachment 660676 [details] [diff] [review]
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] 2012-09-12 20:16:54 PDT
Created attachment 660679 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2012-09-13 12:37:54 PDT
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) 2012-09-13 17:32:09 PDT
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] 2012-09-15 13:12:16 PDT
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] 2012-09-22 15:26:03 PDT
(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] 2013-01-25 09:08:45 PST
Created attachment 706422 [details] [diff] [review]
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] 2013-01-25 09:13:20 PST
Created attachment 706424 [details] [diff] [review]
Part 2: Use the new MIME parser for this interface instead

Updated version of this patch for review.
Comment 9 Andrew Sutherland [:asuth] 2013-01-25 10:55:51 PST
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) 2013-01-30 08:09:15 PST
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] 2013-01-31 05:47:15 PST
Patches pushed: <https://hg.mozilla.org/comm-central/pushloghtml?changeset=b0ee9c28b03a>.

Note You need to log in before you can comment on or make changes to this bug.