Open Bug 1211292 Opened 5 years ago Updated 2 months ago

Rewrite nsMsgSend in JS

Categories

(MailNews Core :: Composition, task, P3)

Tracking

(Not tracked)

People

(Reporter: jcranmer, Assigned: jcranmer)

Details

(Whiteboard: [patchlove])

Attachments

(1 file)

Don't panic, I'm nowhere near done with this yet. I'm just filing early so that relevant parties can influence some of the development direction.

nsMsgSend is the largest part of the compose source code; I use the term loosely here, since the logic implied by nsIMsgSend ends up getting spread out to 10 different files which are really inextricable from the current C++ implementation--the most important being nsMsgSend.cpp, nsMsgCompUtils.cpp (which handles headers), nsURLFetcher.cpp, nsMsgAttachmentHandler.cpp (which mostly handles figuring out how to convert an attachment to a MIME part), and nsMsgSendPart.cpp (which does most of the actual assembly). This is collectively about 10,000 lines of C++ code, which will end up being replaced by perhaps 3000-5000 lines of JS.

So the good news is that we get to migrate 3% of our C++ codebase to JS in one go! And the bad news is that this ends up being effectively one gigantic patch: the backend here is not designed (nor is it easy to retrofit into) to allow for piecemeal conversion of logic. nsMsgSend has some quite frankly broken logic and simultaneously overly and insufficiently expressive interfaces; the rewrite will end up "accidentally" fixing several issues of known broken logic (even the possible duplicates list that pops up when I wrote this bug managed to uncover one such possible issue)--and, naturally, will probably come with severe regressions of its own that can't be uncovered except by real-world tests.

As I see it, nsMsgSend comprises basically three loosely-dependent but atomic steps of a pipeline:
1. From input values, produce the top-level headers, and collect all of the bodies, attachments, and inline images into a uniform representation.
2. Convert said representation into a MIME blob.
3. Send that MIME blob over SMTP/NNTP/to a folder/etc. as desired.

This suggests a potential transition plan that allows for these steps to be converted independently, but reality isn't quite so simple: nsMsgSendPart (in no small part thanks to nsIMsgComposeSecure) isn't really set up to be easily rewritten in JS independently of nsMsgSend, and the burden of tasks like "who chooses the Content-Transfer-Encoding?" is in the wrong place for my tastes. I have an abandoned patch queue that does make it feasible, but it's ≥9 patches that involve almost completely rewriting nsMsgSendPart just to make it possible to use a JS implementation instead. I will point out, though, that the boundary between parts 2 and 3 is much easier to cross: my current work relies on the C++ implementation entirely for part 3 while doing parts 1 and 2 all in JS.

It's possible to land a "MimePartBuilder" (the main class in my implementation to do step #2) independently--but the potential uses in the code base outside of the compose backend are too few and too minor to provide much useful feedback on its implementation.

So this basically leaves us with "land the JS implementation all at once" or "land the JS implementation minus step #3" as the most plausible landing plans, much as I'm sure the reviewers will hate me for that. I rather suspect that it would behoove us to leave the C++ implementation in-tree (and not break it!) for at least 1 or 2 beta cycles and ideally an ESR release so that we can quickly switch back to it if need be. This implies that interface simplification (sorely needed!) would have to wait that long as well.


A rundown of the current architecture of my reimplementation, compared to the original:
1. MimePartBuilder becomes a separate class with a dedicated, exposed interface (although I'm considering not providing XPIDL for it, since the only potential C++ consumer at this point, nsMsgMdnGenerator, could be easily rewritten in JS).
2. The main entry points to nsIMsgSend (createAndSendMessage/createRFC822Message) shove all their parameters into easier-to-handle BodyOptionsBag and SendOptionsBag object and pass those to a promise/Task-based internal operation (with errors being processed in a catch-all handler, although the error-handling code is probably broken at present. Too many listeners!).
3. The C++ methods ::InitCompositionFields and msg_generate_headers have one-to-one correspondence with JS methods such that you could open the C++ code and the JS code and directly confirm their equivalence.
4. Everything else about step 1 was designed from scratch in terms of "how would I go about setting this code up?", since the C++ code is... wrong, squirrely, broken, inane. It does make it much harder to verify that the capabilities are the same, particularly around corner cases like attaching .eml messages.
5. Step 2 is mostly leveraged around jsmime. I don't have any support for anything less than "write the entire body all at once" (internally, the emitters work on a streaming interface, but the API doesn't expose any sort of piecemeal body emission, largely because of concerns about both how to do so (WHATWG streams aren't implemented yet) and complexity of transfer encodings at that point).
6. I've not started any work on the actual send portion of the interface. I have a vague notion of what I want (essentially, expressing the code to send to file/folder/SMTP/NNTP as a "send target" and hooking up send to merely go through each of the send targets in turn).

In the medium term, there is scope for radical interface simplification. nsIMsgSend is mostly "internal" methods being exposed, for example, and nsIMsgSendReport and nsIMsgSendListener are far too overengineered when you look at how they get used in practice. On the other hand, there are situations where the current interfaces are just insufficient: nsIMsgComposeSecure desperately wants to be rewritten, I think, and there's no easy way for new account types to do custom mail delivery (that's partially what I want to rectify with send targets), as I think rkent has too much experience with.
The ExQuilla/SkinkGlue override of nsMsgSend is one of two places where I use js overrides instead of C++ classes. The js part of this is 585 lines, not too bad actually. Although the code is not free licensed, because it is in js it is freely readable, and I have no objection to using it to guide development.

I fully agree that "nsMsgSend comprises basically three loosely-dependent but atomic steps of a pipeline" means we really need to break those out into clearly defined separate steps. I also fully agree that nsMsgSend is probably too convoluted now to be amendable to piece-wise conversion from C++ to js (unlike say nsMsgDBFolder), which hopefully will be possible in other methods if we can extend JsAccount to include that capability.

In the SkinkGlue/JsAccount system, an additional interface is added that enables additional internal features that must be overridden for a js implementation to work. Although ExQuilla does not have a partticularly robust send capability, here is what was needed (see source at https://bitbucket.org/rkentjames/skinkglue/src/43f63f3e35b7ab171125b6d4db287fe4bf6958e9/public/msqISgSend.idl?at=tip)

interface msqISgSend : nsISupports
{
  readonly attribute nsIMsgCompFields sendCompFields;
  AString getSendBody();
  ACString getSendBodyType();
  readonly attribute nsIMsgIdentity identity;

  // The lifetime of the attachment is dependent on the existence
  //  of the underlying send object, so do not hold onto these
  //  attachments.
  msqISgAttachmentHandler getAttachment(in unsigned long index);

  // testing only
  void setDeliveryMode(in long aDeliveryMode);

  // error reporting uses an internal string name which we must set
  attribute AString savedToFolderName;

  // allows us to set m_dont_deliver_p outside of init
  attribute boolean dontDeliver;
  
};

Not a very long list actually.
I forgot to do this last night, but here's roughly the work in progress I have. It's a bit squirrelly since it's being developed sort-of as an add-on, and the test fixes needed are annoying.
Whiteboard: [patchlove]
Status: ASSIGNED → NEW
Type: defect → task
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.