(In reply to Joshua Cranmer [:jcranmer] from comment #0)

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.

As Joshua mentions further down in his comment #0, there is also nsIMsgComposeSecure, so at some stage the data needs to be run through encryption. Maybe that happens after step 2 and encryption replaces one "MIME blob" with another.

This patch adds a MessageSend.jsm and pipes to nsMsgSend, the goals of this patch are:

• Everything should work as before
• When using MOZ_NEW_MSG_SEND=1 ./mach run, the js implementation of createAndSendMessage will be used, and should be able to send plain and/or html text mail

Next steps will be:

• Handle attachments
• Handle encryption
• Details and things I'm not aware of yet

A Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8b475b827fc1cb88ca01498ecdae6289727fd03a&selectedTaskRun=BV_QGx_ITICXD4lGRR5-wg.0

Everything seems good, except test_temporaryFilesRemoved.js is failing. The reason is after wrapping nsMsgSend inside MessageSend.jsm, the destructor of nsMsgSend is not run as fast as when using nsMsgSend directly. Haven't found a workaround yet.

This patch has quite a few issues, some major and some minor, so let me first focus on those higher-level issues before delving into minutiae.

The biggest issue I have here is that I don't see any sense of a design for the rewrite. Looking at this patch, I have a hard time seeing what you envision the code will look like once we kill off the old code; how will the various stages be organized in their constituent classes/functions; what kind of API that's not the monstrosity of nsIMsgSend will look like. That's the design part that's lacking here, and I believe it would behoove us to nail down the design before going off and doing much implementation.

A related note is the plan for this implementation. When I proposed essentially one big code drop, I was too focused on making changesets be atomic "feature-complete" changesets rather than actually finding ways to make small changes that make sense. I want to push you to do a large-set-of-small-change approach here, and a sketch of what that would start with is:

1. Add a JS implementation that handles the fallback mechanism, and does nothing else.
2. Gather the features implied by all of the prefs that are hidden somewhere in the process--and fallback if the implementation doesn't support them yet (which it obviously won't at this stage!).
3. Change the compose tests to be able to test both the old and new implementation.
4. Now add support for plain text emission. No format=flowed support, no del=sp, no non-UTF-8 charset support, just the most basic plain text output. For now, we'll send via the old implementation.
5. Add format=flowed support.
6. Add HTML support (but don't support embedded images!)
7. Add support for regular attachments
8. And now cloud attachments
9. And embedded images
10. How about some multipart/alternative support?
11. Encryption might happen now.
12. Wouldn't it be nice to attach an email?
13. Ooh, let's start handling send stuff in JS now. And this probably requires multiple patches of course!

I'm not going to insist on that particular set of changes, but that should give you an idea of the kind of plan I'd like to see for these changes.

I'm not a fan of how the fallback works in this patch. An approach that may work better is to try to use the JS implementation, and fallback to the old C++ implementation if it requires an unsupported feature (this is why I had patch 2 in my above listed sequence). Combine this with a mechanism to force the old or new implementation as well (which works especially well for testing--the tests can run twice, once with forcing old and once with forcing new).

Hi, thanks for the review.

The biggest issue I have here is that I don't see any sense of a design for the rewrite

The overall roadmap is in comment 4. Let me try to rephrase it a bit

1. the current patch: introducing MessageSend.jsm, and fallback to nsMsgSend.cpp, support html and/or plain text
2. support attachments
3. support encryption
4. other things I'm not aware yet

I'm learning by doing right now, I'm not exactly clear what will be involved in the next steps.

3. Change the compose tests to be able to test both the old and new implementation.

Do you mean test the old and new separately? From a user's perspective, there is just one version. In the current patch, the tests are testing nsMsgSend.cpp wrapped inside MessageSend.jsm. When it's ready in a few months, tests will be testing MessageSend.jsm only.

I explained above one way to handle the fallback, which is not the way you've chosen to do it here. Were you to do the fallback method you're doing here
this._send = Components.classesByID["{935284E0-C5D8-11d2-8297-000000000000}"]
.createInstance(Ci.nsIMsgSend);

Yes, I saw Components.classesByID in your prototype patch, but it doesn't seem to exist anymore. https://searchfox.org/mozilla-central/search?path=&q=classesByID returns anything. That's why I think two contractids are needed. I can change the old one to `define NS_MSGSEND_CONTRACTID0 "@mozilla.org/messengercompose/send;0", is that better?

I am not a fan of using environment variables for this sort of thing; it's actually amazingly user-hostile to pass things around via this out-of-band channel.
The general trend at Mozilla, I believe, has been to use hidden prefs to trigger this sort of thing.

Compile time env vars seems common in our code base. Like this https://searchfox.org/comm-central/source/mail/base/modules/MailConstants.jsm#13-18. But it takes a long time to rebuild after toggling a compile time env. That's why I'm using a runtime env here.

This code is incorrect for handling the feature set it's apparently claiming to support. But it's also not going to generalize well to supporting the features that need to be supported.

Is it only the content-type incorrect, or are there other things incorrect? Yes, I'm aware this needs to be changed when handling attachments. I'm going to introduce something similar to the MimePart in your prototype patch and nsMsgSendPart in the next patch.

So if the user specifically requests additional headers, she doesn't get them because you only grab a precious few headers from the compose fields?

Do you mean I should extract all the headers from comp fields? Will update.

Aside from the testing and fallback mechanism, I guess my roadmap is not too different from yours. Do you think changing the old contract id to define NS_MSGSEND_CONTRACTID0 "@mozilla.org/messengercompose/send;0" is good? Or maybe there are other approaches?

For testing "both version", see what we do for ical.js: https://searchfox.org/comm-central/search?q=calendar.icaljs&path=&case=false&regexp=false
(Also does dynamic registration of one or the other calendar library)

(In reply to Magnus Melin [:mkmelin] from comment #8)

For testing "both version", see what we do for ical.js: https://searchfox.org/comm-central/search?q=calendar.icaljs&path=&case=false&regexp=false
(Also does dynamic registration of one or the other calendar library)

Thanks, didn't know xpcom can be dynamically registered.

So this patch adds a MsgSendModuleLoader.jsm, that doesn't do anything by default, but will load MessageSend.jsm when a pref value is true.

(In reply to Ping Chen (:rnons) from comment #7)

The overall roadmap is in comment 4. Let me try to rephrase it a bit

1. the current patch: introducing MessageSend.jsm, and fallback to nsMsgSend.cpp, support html and/or plain text
2. support attachments
3. support encryption
4. other things I'm not aware yet

A roadmap is not a design. The design I am looking for is the skeleton API of what the final product should look like: which classes will need to exist, who will be responsible for handling various steps of the process, what parameters need to be passed around, etc.

I'm learning by doing right now, I'm not exactly clear what will be involved in the next steps.

Please stop writing code for the moment and spend a few days just reading through the existing code and taking notes on the features it supports and some of the ways it goes about doing things. Then think about how the design should look to implement those features (although note that we don't want to continue supporting all features--multipart/apple-double for example we're likely to drop). Only once you have that first draft of a design should you start to go about writing the code. I'm saying this to try to save you hours of time.

I do realize that our compose code is dilapidated and cruddy, and that it can be hard to follow. If you need help, ping me in #maildev on matrix and I can try to respond as quickly as I'm available. (Or others might be able to answer your question as well).

Do you mean test the old and new separately? From a user's perspective, there is just one version. In the current patch, the tests are testing nsMsgSend.cpp wrapped inside MessageSend.jsm. When it's ready in a few months, tests will be testing MessageSend.jsm only.

The old and new versions will have to coexist for some time. While they coexist, it is necessary to test both. (And some time can be a long time; ical.js has been meant to replace libical for over 7 years now!).

Yes, I saw Components.classesByID in your prototype patch, but it doesn't seem to exist anymore. https://searchfox.org/mozilla-central/search?path=&q=classesByID returns anything.

I'm going to have to do some more research here... It may be advisable simply to do the choose-which-one in C++ code rather than JS, since C++ definitely can construct by CID instead of contractID.

Is it only the content-type incorrect, or are there other things incorrect? Yes, I'm aware this needs to be changed when handling attachments. I'm going to introduce something similar to the MimePart in your prototype patch and nsMsgSendPart in the next patch.

I don't know; I didn't spend the time to enumerate all the problems. If you're going to rip the code out shortly anyways, why should I bother trying to understand all the issues? This is why I am pushing you hard to do a design first: it will help you understand what you need to do, and you won't have to write (and I won't have to review) incorrect code.

Well many times just taking it one step at a time to see what needs to happen is the only feasible route.
Trying to understand it all beforehand is asking a lot. Of course there will be iterations. Especially since it's off by default until everything is working, I don't see a problem with that.

(In reply to Joshua Cranmer [:jcranmer] from comment #10)

A roadmap is not a design. The design I am looking for is the skeleton API of what the final product should look like: which classes will need to exist, who will be responsible for handling various steps of the process, what parameters need to be passed around, etc.

Maybe we have different understanding of this task. In my opinion the task is rewriting, the skeleton api will still be nsIMsgSend. I think you mentioned on matrix that you don't like 18 arguments got passed to createAndSendMessage, but I don't think it's good to change it in this task.

Please stop writing code for the moment and spend a few days just reading through the existing code and taking notes on the features it supports and some of the ways it goes about doing things. Then think about how the design should look to implement those features (although note that we don't want to continue supporting all features--multipart/apple-double for example we're likely to drop). Only once you have that first draft of a design should you start to go about writing the code. I'm saying this to try to save you hours of time.

I've already spent a few days reading the existing code and your prototype patch. I tried to understand every line of the code, but it doesn't seem to work for me. I don't think I can form a design different from nsIMsgSend by reading the existing code, but after I implement all the interfaces of nsIMsgSend in JS, I should have better idea. So I changed my mind to start with porting createAndSendMessage, try to support sending html/plain message as the first step, and fill in missing pieces step by step.

The old and new versions will have to coexist for some time. While they coexist, it is necessary to test both. (And some time can be a long time; ical.js has been meant to replace libical for over 7 years now!).

I was only aware of ical.js and libical yesterday, my new patch should support that, do you mind taking a look?

Is it only the content-type incorrect, or are there other things incorrect? Yes, I'm aware this needs to be changed when handling attachments. I'm going to introduce something similar to the MimePart in your prototype patch and nsMsgSendPart in the next patch.

I don't know; I didn't spend the time to enumerate all the problems. If you're going to rip the code out shortly anyways, why should I bother trying to understand all the issues? This is why I am pushing you hard to do a design first: it will help you understand what you need to do, and you won't have to write (and I won't have to review) incorrect code.

I'm going to adapt the code to support attachments, not throw it away. By introducing a MimePart class with a Write function (https://searchfox.org/comm-central/source/mailnews/compose/src/nsMsgSendPart.cpp#378), I still need to build the final message not too different from string concatenation. Getting the most basic html/plain text message right will help me with the next step.

BTW, my new patch in comment 9 is roughly the first item in your roadmap:

1. Add a JS implementation that handles the fallback mechanism, and does nothing else.

I will send a patch that supports html/plain message after that.

(In reply to Joshua Cranmer [:jcranmer] from comment #10)

A roadmap is not a design. The design I am looking for is the skeleton API of what the final product should look like: which classes will need to exist, who will be responsible for handling various steps of the process, what parameters need to be passed around, etc.

It just came to my mind that the the internal structure of MessageSend.jsm will become much more clear when supporting attachments. I haven't figured out how attachments are handled, and I don't want to put too much stuff in one patch. That's why I did the html/plain message as a separate patch. It helped me understand how a RFC822/RFC2045 message looks like. If we can get things right for html/plain text only message first, then we have less things to worry about in the next steps.

I don't trust my design if I can't see sending/receiving message actually works in TB. It will help me a lot if you can bare with me for the first two patches. We can discuss more with the actual code in the planned third patch, in which I will handle attachments.

As a pointer, note that there is https://searchfox.org/comm-central/rev/5edba8a2dd51172df411fbac749d944a9c65dae2/mailnews/db/gloda/modules/MimeMessage.jsm#354 which may be of relevance

This patch partially supports sending text message without any attachments. Content-Transfer-Encoding is missing right now. I will add a pickEncoding function and encode the body text when necessary in the next patch.

If you learn best by implementing code, that is totally fine and I have no issues with you doing that. If you need to implement the code in the context of Thunderbird to get that practical experience, again I have no objections. If you need more experience before you can develop a design, yet again, I will not object.

But to review a project of this magnitude, I do need that design. And I believe it would be a better use of both your time and my time were you to go ahead and complete your learning experiences without trying to work those patches for review. When you have those modifications done, then you should be able to complete a better design, and we can discuss that. And it may be that you can reuse a lot of your prior code; it may be that you cannot use it. Don't feel bad about throwing away code; I know I've got at least two or three attempts at rewriting this code in the past, even if most of them have never even graced a review comment.

This patch uses the categories field in components.conf.

(In reply to Joshua Cranmer [:jcranmer] from comment #18)

I'm in the middle of moving comm-central to static component registration in
bug 1652354, which will likely have knock-on effects for the registration
here.

Are you worrying there will be lots of conflicts? I didn't change the registration of any existing components. Seems to me the only conflict will be in the components.conf file.

(I think there's more work that needs to be done to be able to
override contract IDs in static component registration). I don't believe
it's worth appraising this patch before that code is ready.

The approach in this patch is the same as calBackendLoader.js, which loads ical.js dynamically. And it seems to work. Please tell me what more I need to do.

(In reply to Joshua Cranmer [:jcranmer] from comment #19)

Comment on attachment 9166755 [details] [diff] [review]
Think about what you need to do for 8-bit, non-UTF-8 parts...
... because JS strings are likely to cause you to do the wrong thing.

Can you enlighten me what do you mean 8-bit, non-UTF-8 part? And what is the problem of using JS string?

And it seems clear to me that you really love using JS's string formatting,
but in the long run, you are probably better off using Uint8Array instead of
strings.

[This is an example of why I want to see a design up-front: you could
potentially convince me otherwise that strings instead of Uint8Array are
going to be OK for this code, but convincing is going to be very difficult
without enough of a design to indicate to me that you've thought through the
implications.]

In my opinion, this is about details/things I'm unaware of. If I don't know using JS string is a problem, how would my design avoid it? I use JS string because it works so far. If you point out the problems, I would sure change to Uint8Array if necessary. And I already mentioned "I will add a pickEncoding function and encode the body text when necessary in the next patch." in comment 15.

(In reply to Joshua Cranmer [:jcranmer] from comment #19)

Comment on attachment 9166755 [details] [diff] [review] [diff] [review]
Think about what you need to do for 8-bit, non-UTF-8 parts...
... because JS strings are likely to cause you to do the wrong thing.

I think I understand the problem better now, it's not that JS can't represent 8-bit non-UTF-8 chars, but it becomes incorrect after converting back and forth, especially TextEncoder only supports UTF-8. So this patch stores 8-bit string in MimePart directly to avoid converting. I think string is more suitable than Uint8Array, because there is no .concat method of Uint8Array, I will need to use something like new Uint8Array([...a, ...b]) to combine two arrays. I also need to convert headers and part separator to Uint8Array. The code will be difficult to read and easy to make mistakes.

The more I work on this bug, the more details I know. I don't worry about the design, I worry about all kinds of cases. When I submitted the first patch, I didn't even know foramt=flowed only applies to text/plain. After adding class MimePart in this patch, and class MimeEncoder on my local, I'm satisfied with the design, I believe it can handle attachments without big change. Thank you for pointing out things I don't know.

Updates according to comment 23.

• use Components.Exception
• Remove the a prefix from argument names

Updates according to comment 24.

file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0o600);

Can use OS.Constants.Path.tmpDir and use the OS.File (or OS.Path) operators to get the file path. I don't think it needs to be created, you should just call writeAtomic directly.

The purpose is to create a unique file, similar to https://searchfox.org/comm-central/source/mailnews/compose/src/nsMsgCompUtils.cpp#84. I guess it's safer this way to avoid two TB instances operating the same file? I will change if you think it's unnecessary. I also changed to use OS.File.openUnique, which seems more efficient than nsIFile.createUnique.

Updates according to comment 27.

+        .msgGenerateMessageId(this._userIdentity);
+    }
+    let headers = new Map([
+      ["Message-Id", messageId],
+      ["Date", new Date()],

this is not quite the mail date formatting

My understanding is jsmime will handle the date formatting. https://searchfox.org/comm-central/source/mailnews/mime/jsmime/jsmime.js#3464

This patch adds a class MimeEncoder, that mainly provides two functions

• pickEncoding: determine the Content-Transfer-Encoding value
• encode: apply base64 or quoted-printable encoding to the body of MimePart

MimeEncoder will also be used to handle attachments in the next patch.

(In reply to Ping Chen (:rnons) from comment #22)

I think I understand the problem better now, it's not that JS can't represent 8-bit non-UTF-8 chars, but it becomes incorrect after converting back and forth, especially TextEncoder only supports UTF-8. So this patch stores 8-bit string in MimePart directly to avoid converting. I think string is more suitable than Uint8Array, because there is no .concat method of Uint8Array, I will need to use something like new Uint8Array([...a, ...b]) to combine two arrays. I also need to convert headers and part separator to Uint8Array. The code will be difficult to read and easy to make mistakes.

No. Thinking about the problem as concatenation--string or Uint8Array--is the wrong way to think about the problem. What you're doing is streaming a series of bytes to an output. This output may itself be a transformer, for example someone who is converting it into base64 or quoted-printable text, or maybe someone streaming it into an encryption algorithm to support encrypted messages. Or the target may simply be an output file. Or maybe you're piping it directly into a network connection. In any case, at no point would you be directly concatenating bytes together. As a bonus, at no point do you need to hold an entire copy of the message in memory.

The more I work on this bug, the more details I know. I don't worry about the design, I worry about all kinds of cases.

I worry about all kinds of cases, including weird edge conditions. And that's why I worry about the design: I need to see that the design handles all of the cases--and in lack of a design, I at least need confidence that you are at least worrying about all of them too. But I don't have that confidence here, and your replies have so far not been giving me confidence.

Let me start by apologizing for the length of this comment. It is a wall of text, but I don't think I can express what needs to be expressed without making it a wall of text. Please read all of it before moving to respond to any part.

I have concerns about the design of the rewrite that you are proposing. Getting the design right from the beginning is crucial; bad design has a way of leaking through APIs and spreading through the codebase. Furthermore, people and projects are usually extremely reluctant to tackle fixing bad design: it's easier to build yet another workaround rather than taking the time to rip out bad design.

An example of bad design in mailnews code that has infected beyond its initial APIs is that we store unparsed, rather than parsed, headers in our database, and so all the clients must parse them, which opens up inconsistencies if they don't use the correct API (and it's not easy to do so!).

In rewriting the code, we have a great opportunity to fix the design, especially since we can look back onto the existing code and evaluate design decisions that worked and those that didn't work. We do not need to rely on guesses as to how well the design will scale to future uses, when the uses are pretty well-known.

From my perspective, when I see your code, I have no prior experience to draw on to estimate how good you are at planning for future design. All I can do is extrapolate from the code you have written and from your replies to my review comments. If I were satisfied that you understood the design issues and could come up with a good design, I would not be pushing for a more formal design.

To be blunt: I did not like your design from the first patch, and your comments since then have only strengthened that assessment. Without confidence that you will be able to carry through this process through the end, it is hard for me to justify the immense time sink of thoroughly checking the existing code to understand what is and isn't being regressed.

Let's talk issues in more detail now. I'm not sure I've covered all of the issues in this list, but here they are as they cross my mind:

• API design. As I mentioned in the beginning of this bug, the current interface of nsIMsgSend is a bad interface design and needs to be replaced with something better. While the new interface does need to be the last step of a very long sequence of patches, it is a good idea to design to a new interface from the beginning. To that end, I expect to see the code implemented in a way that is not so strictly tied to needing to support the current API. I'd actually expect much of the work to exist outside of a MessageSend class.

• Blind reimplementation of existing code. This is a problem I've seen more with your later patches: there appears to be some copying of decisions from existing C++ code that is wrong and needs to be fixed, rewrite or not. An example here is the use of charsets influencing the type of encoding to use... just no. Now this doesn't mean that there isn't wisdom in existing code that can and should be ported, but it does require understanding the code, understanding why it's doing what it's doing, and then deciding if those reasons are still valid.

• Using strings for binary data. I realize that JS has really poor facilities for handling binary data, especially compared to its string libraries. But my historical experience is that using (nominally) Unicode strings for binary data is a route that only leads to problems down the line. I've mentioned this in my previous comment, but the way you get around array concatenation is by changing the API so that you never need to actually concatenate arrays. Indeed, even having to do a concatenation of any kind in this context is a code smell.

• Strings in other contexts. You're using a string to represent content-type, including parameters instead of a data structure that will serialize to the string value, but gives the necessary parameters in an easier format. Considering that you're already going to have to worry about RFC 2231 encoding parameters and writing that out anyways, it makes sense to convey other content-type parameters in a similar manner. In general, strings are bad data types, and you should only convert to strings at the last possible moment.

• File size and method size. I have a personal distaste for large files--I think code is optimally sized at no more than 1000-ish lines of code per file. So when code starts greatly exceeding that size, I would want to see it broken up into smaller pieces. I envision there being roughly three major stages here at the end: a low-level MIME assembly file, a low-level actual sending phase, and an orchestration component that mediates between the input parameters and the low-level phases. I suggest you separate these phases into separate JS files.

• Error handling. JS code is pretty notorious for causing errors to be thrown in unexpected places, much more so than our C++ code. The direct implementation of nsIMsgSend needs to have a relatively thin intermediary layer that will catch and propagate errors as needed to the listeners, else things like tests tend to hang without any clear indication why.

• Tests. Our current test coverage is actually kind of crappy, especially when it comes to ensuring corner cases are handled correctly. I'd expect any rewrite to be adding a lot more tests. I'd also actually expect some of the existing tests to have to be rewritten, because they have a nasty habit of being too specific to particular details that need not be kept (e.g., line break locations in encoded messages).

Now let's talk about next steps.

I don't want to see any more code for the moment; I'm not happy with the current design, and I want to make sure that we can agree on how the code should work before you spend more time pursuing a design that will only be rejected.

You've mentioned that you worry about all kind of cases. I think the best next step for us to take is to put together a list of all the existing features, features to add in the future, and features to remove from the current implementation. To that end, I'm attaching an incomplete list that should give you a starting point. Note that the list is incomplete: it's entirely based on what I can recall from memory after 15 minutes or so, and it has been quite some time since I went through compose code with a fine-toothed comb.

After that, the next step would be to put together what nsIMsgSend should look like if we weren't constrained to implement the current interface. This is the main essence of why we are rewriting the code: if we are not going to improve on what exists, then rewriting is just a waste of time that will introduce yet more bugs and regressions into our codebase.

As part of that, we also need to understand what new interfaces we should add to reduce code duplication. There are a few other places where we build MIME messages, and adding a new interface to support those use cases is a good idea to me.

Only once those steps are completed do I believe it is worth taking the time to actually look at the implementation of code.

In addition to comment 29, this patch:

• split out MimePart and MimeEncoder to separate files
• add a write method to MimePart so no need to hold the whole message in memory

Blind reimplementation of existing code. This is a problem I've seen more with your later patches: there appears to be some copying of decisions from existing C++ code that is wrong and needs to be fixed, rewrite or not. An example here is the use of charsets influencing the type of encoding to use... just no. Now this doesn't mean that there isn't wisdom in existing code that can and should be ported, but it does require understanding the code, understanding why it's doing what it's doing, and then deciding if those reasons are still valid.

I didn't know converting the existing C++ code to JS would be a problem, I still don't what's the problem of using charset to pick encoding, I will think about it. In the meantime, it seems safer for me to just copy the behavior of existing C++ code.

Using strings for binary data.

I changed to write out immediately, instead of holding the whole message in memory, is that better?

Strings in other contexts. You're using a string to represent content-type, including parameters instead of a data structure that will serialize to the string value, but gives the necessary parameters in an easier format. Considering that you're already going to have to worry about RFC 2231 encoding parameters and writing that out anyways, it makes sense to convey other content-type parameters in a similar manner. In general, strings are bad data types, and you should only convert to strings at the last possible moment.

Shouldn't this be handled by jsmime?

File size and method size.

MimePart and MimeEncoder are split into separated files in this patch.

I don't want to see any more code for the moment; I'm not happy with the current design, and I want to make sure that we can agree on how the code should work before you spend more time pursuing a design that will only be rejected.

Please tell me the problems of the current design so I can fix. I don't have enough knowledge to discuss with you on the big picture. The current approaching of rewriting bit by bit seems to work best for me.

After that, the next step would be to put together what nsIMsgSend should look like if we weren't constrained to implement the current interface. This is the main essence of why we are rewriting the code: if we are not going to improve on what exists, then rewriting is just a waste of time that will introduce yet more bugs and regressions into our codebase.

I think it's an improvement if by rewriting, the data flow becomes cleaner, the code becomes easier to make changes. I believe many of the problems you pointed out (e.g. charset used in pickEncoding) can be fixed in a very local place. And I'm thinking about enabling some tests in the next two patches. I'd like to know what are the critical design flaws of MessageSend, MimePart and MimeEncoder.

(In reply to Joshua Cranmer [:jcranmer] from comment #31)
Clearly the original API isn't ideal, but if we not kind of copy things fairly directly, making the both implementations live side by side until we're further down the road doesn't seem very possible.

(In reply to Magnus Melin [:mkmelin] from comment #36)

(In reply to Joshua Cranmer [:jcranmer] from comment #31)
Clearly the original API isn't ideal, but if we not kind of copy things fairly directly, making the both implementations live side by side until we're further down the road doesn't seem very possible.

Implementing the current API in terms of a new API is likely quite possible; I've done in that in prior rewrite attempts with no issues whatsoever.

Updates according to comment 34. Added jsdoc docs to MimePart, will add more in the future.

Updates according to comment 35.

(In reply to Joshua Cranmer [:jcranmer] from comment #31)

Now let's talk about next steps.

I don't want to see any more code for the moment; I'm not happy with the current design, and I want to make sure that we can agree on how the code should work before you spend more time pursuing a design that will only be rejected.

You've mentioned that you worry about all kind of cases. I think the best next step for us to take is to put together a list of all the existing features, features to add in the future, and features to remove from the current implementation. To that end, I'm attaching an incomplete list that should give you a starting point. Note that the list is incomplete: it's entirely based on what I can recall from memory after 15 minutes or so, and it has been quite some time since I went through compose code with a fine-toothed comb.

After that, the next step would be to put together what nsIMsgSend should look like if we weren't constrained to implement the current interface. This is the main essence of why we are rewriting the code: if we are not going to improve on what exists, then rewriting is just a waste of time that will introduce yet more bugs and regressions into our codebase.

As part of that, we also need to understand what new interfaces we should add to reduce code duplication. There are a few other places where we build MIME messages, and adding a new interface to support those use cases is a good idea to me.

Only once those steps are completed do I believe it is worth taking the time to actually look at the implementation of code.

Sorry I'm not convinced this is the only and best way to approach this task. It seems to me most of my code are not directly related to nsIMsgSend, for those that do, they are for the purpose of connecting with the existing code, so I can send/receive emails for testing. If it helps, I can split the current MessageSend to say class MsgSend and class MessageSend, MsgSend implements nsIMsgSend and MessageSend will be a new class with new interfaces. They might still be tightly coupled and limited by nsIMsgSend at first.

Why can't we fix problems one by one? For example, I added a write method to replace toString yesterday, is it better now? Or can we delay it and fix other problems first?

As mentioned in comment 40, this patch splits out a class MimeMessage from MessageSend.jsm. MessageSend is the class that implements nsIMsgSend, MimeMessage has more flexibility. And in long terms, we may want to replace nsIMsgSend with MimeMessage.

Will land these for easier iteration in tree

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/587fde4c6cd0
Init MsgSendModuleLoader and MessageSend to prepare for rewriting nsMsgSend. r=mkmelin
https://hg.mozilla.org/comm-central/rev/6c4eb4fc78a3
Partial support of sending text message in MessageSend.jsm. r=mkmelin
https://hg.mozilla.org/comm-central/rev/2253cbe6d5e5
Add MimeEncoder class to pick encoding and encode message in MessageSend.jsm. r=mkmelin DONTBUILD

Updates according to comment 45:

• Move EXPORTED_SYMBOLS to the top
• Fix @param comment

This patch supports sending local attachments.

I've made some progress on the configs to run a test for both nsMsgSend.cpp and MessageSend.jsm, but seems most tests rely on the Draft folder, which MessageSend.jsm doesn't support yet. So I will work on that in the next patch, and remove charset and delsp according to bug 862292.

Updates according to comment 48.

Maybe MimeMultiPart should be a subclass of MimePart, and the "init" would happen automatically/passed in to the constructor?

Good suggestion, I think this approach can be applied to attachment part and main part as well. So there will be classes like MimePartBase, MimeMultiPart, MimeMainPart and MimeAttachmentPart. I will play with this idea when handling cloud attachment. Thanks.

I'll add a Cu.importGlobalProperties(["fetch"]); to MimePart.jsm make eslint happy

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d204b1fe63f9
Split out MimeMessage class from MessageSend.jsm. r=mkmelin
https://hg.mozilla.org/comm-central/rev/def8329e7a26
Support local attachments in MessageSend.jsm. r=mkmelin

• Remove charset handling following bug 862292
• Support saving to the Draft folder
• Run test_messageHeaders against both nsMsgSend.cpp and MessageSend.jsm

Try link: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=652af3ff64bb8b9cf69ecc74ced94f6bd65cf1c3&selectedTaskRun=NOqXr335T46gD6szqBQIIQ.0

I'm aware the handling of bcc/fcc is incomplete in this patch, I will improve it later by enabling more tests or adding new tests if necessary.

+   * @param aMsgSendObj   The nsIMsgSend instance that listens to copy events`

Or is this really the nsIUrlListener?

I believe it's nsIMsgSend, they are tightly coupled.
https://searchfox.org/comm-central/source/mailnews/compose/src/nsMsgCopy.cpp#120
https://searchfox.org/comm-central/source/mailnews/compose/src/nsMsgCopy.cpp#58,64,70

+      try {
+        isCharsetMultiByte =
+          manager.getCharsetData(this._charset, ".isMultibyte") == "true";
+      } catch {}

But charset is always UTF-8 now isn't it, so is this needed?

I think for text/* attachments, we still need to use pickCharset to detect the charset, and pass it to the MimeEncoder.

+    let cloudParts = [];
+    let localParts = [];

Why not just parts? Is there difference in the final ordering that you want?

Added inline comments. The C++ version handles cloud attachments first, so the tests expect cloudParts first. https://searchfox.org/comm-central/source/mailnews/compose/test/unit/test_messageHeaders.js#523,549

+      "\n": "%0A",
+      "\r": "%0D",
+    };

Where to these come from? Please add a code comment. Why not just encodeURI?

From https://searchfox.org/comm-central/source/mailnews/compose/src/nsMsgCompUtils.cpp#729-736. Related test is here https://searchfox.org/comm-central/source/mailnews/compose/test/unit/test_messageHeaders.js#434-435. encodeURI will do more than needed, < will become %3C

+      if (col >= 38) {

What is this about.

According to https://tools.ietf.org/html/rfc2557#section-4.4.2, the folding *can be* done, using the algorithm defined in [URLBODY](rfc2017) section 3.1.. And RFC2017 says URL-word Must not exceed 40 characters in length. But RFC2557 says can be, not must be, so I removed the 40 chars limit, and leave it to be folded by jsmime.

+      value += transformMap[char] || char;
+    }
+    return `"${value}"`;

seems odd to return a quoted string.

Agreed, I also found bug 151031. I don't find quote is required in rfc2557, so I removed it and updated tests.

+    if (/[\x80-\xff]/.exec(dupParamValue)) {
+      needsEscape = true;
+      dupParamValue = Services.io.escapeString(

encodeURIComponent?

In this case, encodeURIComponent doesn't do as much as Services.io.escapeString, the related test is https://searchfox.org/comm-central/source/mailnews/compose/test/unit/test_messageHeaders.js#415-417, notice .txt is percent encoded as well. I guess when actually sending mail, encodeURIComponent should work as well, but then I would need to change the C++ and tests code.

It seems if two headers are closely related, get them from the same util function can save some code. There is this function https://searchfox.org/comm-central/source/mailnews/compose/src/nsMsgCompUtils.cpp#313-315 that gets two header values at the same time. To split them, I would need to write the same code twice.

I generally prefer {x, y} = func() than [x, y] = func(), I feel object keys are more stable than array index. I need to be more careful about the orders both when returning array and destructuring the array.

Perhaps we can remove Content-Base and only keep Content-Location? According to https://tools.ietf.org/html/rfc2557#section-2.2

Content-Base This header was specified in RFC 2110, but has been removed in this new version of the MHTML standard.

• Remove Content-Base and only keep Content-Location
• Break down two functions to return one header only
• Rename helper functions for X-header** to getXHeader**
• Fix a few jsdocs

• Enable test_saveDraft.js by implementing getXMozillaStatus and getXMozillaStatus2
• Enable test_detectAttachmentCharset.js by implementing pickCharset
• Enable a few other tests without changing code

+      headers.set("X-Mozilla-News-Host", newshost);

should this be lower-case?

To avoid confusion and duplicated header, I changed all header names to lowercase.

     let formatFlowed = Services.prefs.getBoolPref(

I'd just check this in the if clause.
And rename formatParams to format (it's only ever one now)

formatFlowed is used again a few lines below. I renamed formatParams to formatParam.

   void setCharAttribute(in string name, in ACString value);

Hmm, I think you'd then also need to update the setter to take AUTF8String...

I reverted this change, found that I can use getUnicharAttribute instead, https://searchfox.org/comm-central/source/mailnews/base/public/nsIMsgIdentity.idl#248-252. I guess one function is enough for getUnicharAttribute and getCharAttribute, but I'm not going to touch it in this patch.

Updates to comment 60.

• Enable test_attachment by rfc2047 encoding the name, while rfc2311 encoding the filename param
• Enable some other tests without changing code

There seems to be three modes of mail.strictly_mime.parm_folding https://searchfox.org/comm-central/source/mailnews/compose/test/unit/test_attachment.js#47-51. Is RFC 2047 without folding still relevant? Seems jsmime will always fold, to make the test passes I only kept RFC2047WithCRLF and RFC2231 in this patch.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/41824ba64913
Enable test_messageHeaders for MessageSend.jsm. r=mkmelin
https://hg.mozilla.org/comm-central/rev/5fafcb896ef7
Enable test_saveDraft and test_detectAttachmentCharset for MessageSend.jsm. r=mkmelin DONTBUILD

(In reply to Magnus Melin [:mkmelin] from comment #64)

::: mailnews/compose/test/unit/test_attachment.js
@@ +19,1 @@

"`abcdefghijklmnopqrstuvwxyz{|}~.txt";

what are these changes about?

The problem is the leading whitespace in the filename (whitespace inside might trigger similar problem as well). With
var input1 = " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_",
jsmime will format it to

Content-Disposition: attachment; filename*0="
 !\\"#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ";

While the C++ will format it to

Content-Disposition: attachment;
 filename*0=" !\\"#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ";

jsmime split header value with space first, then try to fit a line word by word. I don't see an easy fix to this, one solution might be doing rfc2311 encoding inside jsmime, but that would be a big change.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/0922c9a141ad
Enable test_attachment for MessageSend.jsm. r=mkmelin

• Enable test_sendBackground by implementing sendMessageFile
• Enable test_sendMessageLater3 by transforming some NS_ERRORs
• Enable some other tests

In test_sendBackground.js, I had to use two nsIMsgSend instances, because sendMessageFile is an async function in the JS implementation. If sendMessageFile is immediately called twice on the same nsIMsgSend instance, the second call will overwrite the internal state of the first call.

+        newExitCode = MsgUtils.generateNSError(12589);

What are the magic numbers? If there's no defined constant accessible, add a code comment about what it refers to.

Changed to newExitCode = MsgUtils.NS_ERROR_SMTP_SEND_FAILED_UNKNOWN_SERVER; for better readability.

+        new TextEncoder().encode(`From - ${new Date().toUTCString()}\r\n`)

This date format is not exactly what we use I believe. But I don't know if we really use it for anything either.

The date string is designed to be parsed by PR_ParseTimeString, which supports many formats, and I tested it can parse toUTCString(). On the other hand, I think there is a bug in nsParseMailbox.cpp. This envelope line is not parsed correctly, and the date is never used.

• Enable test_sendMessageFile by calling notifyListenerOnStopCopy at the right moment
• Enable test_autoReply by using writeBinaryString for MimePart
• Enable other tests, only five tests left in -cpp.ini now

I have to use two empty catch blocks in this patch. In C++,

  if (mListener != nullptr)
    mListener->OnStopSending(aMsgID, aStatus, aMsg, returnFile);
  return NS_OK;

Non zero return of OnStopSending is not a problem, since it's ignored. However, in JS,

    if (this._sendListener) {
        this._sendListener.onStopSending(msgId, status, msg, returnFile);

Non zero return of onStopSending will become an exception after going through XPConnect, to ignore the exception, I use try with an empty catch block.

(In reply to Ping Chen (:rnons) from comment #73)

    if (this._sendListener) {
        this._sendListener.onStopSending(msgId, status, msg, returnFile);

Non zero return of onStopSending will become an exception after going through XPConnect, to ignore the exception, I use try with an empty catch block.

Generally with listeners, it is a good idea to report exceptions to the console if they occur. Then they can be more easily tracked rather than the exception getting hidden. If there are onStopSending implementations that are permanently returning non-zero, then maybe they need to be investigated?

Yes, maybe better to not swallow errors too easily.

Print non-zero return of listeners to console, this only happens in tests so far, but we will see.

• Enable test_sendMailAddressIDN.js by encoding mail address before passing to SMTP service
• Enable test_temporaryFilesRemoved.js by reverting back to nsIFile.createUnique. OS.File.openUnique will get a random hash name, which is difficult to test.

All tests except one in mailnews/compose/test/unit have been enabled for MessageSend.jsm. Will see if I can do the same to the mail/test/browser/composition folder.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/8fb018ed386e
Enable test_sendBackground and test_sendMessageLater for MessageSend.jsm. r=mkmelin
https://hg.mozilla.org/comm-central/rev/3c083ed603d8
Enable test_sendMessageFile and test_autoReply for MessageSend.jsm. r=mkmelin DONTBUILD

• Setup configs to support running mail/test/browser/composition against MessageSend.jsm
• Enable browser_drafts by implementing get folderUri
• Enable some other tests

Many tests in mail/test/browser/composition are not directly related to message sending, but I'm going to enable them anyway.

Because the sending process is async, in browser_drafts, I had to add a sleep(1) to test_open_draft_again, so that saving to draft can finish before press_delete. Without the sleep, the press_delete happens first, and then saving to draft will create a new message in the draft folder, which breaks later tests. I don't know why I don't need to add sleep before every press_delete.

+    // In order throw any nserror, _deliverMessage can't be declared async.

Grammar error here. Not sure what it's suppose to read

https://searchfox.org/comm-central/source/mailnews/compose/test/unit/test_sendMailAddressIDN.js#128,150 expects to catch exception thrown by sendMessageFile, with async _deliverMessage(){}, the exception won't be caught in test. Remove the async only for one test seems not ideal and I didn't find a workaround, so I changed to disable two cases in test_sendMailAddressIDN.js for MessageSend for now.

+      copyFile = Services.dirsvc.get("TmpD", Ci.nsIFile);
+      copyFile.append("nscopy.tmp");

Seems backwards to revert this just for testing purposes. I think it would be better to adjust the test (maybe just count files, if names are not stable.)

Updated to count files.

Turns out sleep(1) is unreliable as the tests failed on Try, after looking into browser_drafts again, found that the following code appears in almost every test and is reliable.

  utils.waitFor(
    () => !cwc.window.gSaveOperationInProgress && !cwc.window.gWindowLock,
    "Saving of draft did not finish"
  );

So I moved it into head.js and used it after SaveAsDraft.

+  // Because sendMessageFile is an async function in MessageSend.jsm, exception
+  // can't be caught by DoSendTest.

Could you change this test file to split up the 4 cases to use add_task instead. Then everything is checked async and the test could work?

My comment was a bit misleading, the problem was sendMessageFile is sync according to nsIMsgSend.idl, but when an exception is thrown by an async function called inside sendMessageFile, the exception can't be caught by tests.

This patch updates nsIMsgSend.idl to return Promise from sendMessageFile, then uses add_task in test_sendMailAddressIDN.js. I put this in a separate patch because this is my first time using Promise in xpidl, I worry it may break something unexpected. A Try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4cd447b8aa83465a957c81d49c01f6

Due to bug 1665652, I moved test_sendMailAddressIDN.js back to xpcshell-cpp.ini, so it's not ran against MessageSend and I can move on.

Comment on attachment 9175978 [details] [diff] [review]
1211292-part3.6.patch

The original purpose of this patch is to make test_sendMailAddressIDN.js work with MessageSend.jsm. This patch has its value by returning Promise in xpidl, but then I also need to update test_sendMailAddressIDN.js to use add_task. I will re-submit this patch after bug 1665652.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/bf07498ab5ca
Init mochitest configs and enable browser_drafts for MessageSend.jsm. r=mkmelin
https://hg.mozilla.org/comm-central/rev/160781d72107
Enable test_temporaryFilesRemoved for MessageSend.jsm. r=mkmelin

• Enabled browser_attachmentReminder.js by implementing the error reporting when sending failed
• Enabled browser_emlActions.js by putting messageServiceFromURI in a a try..catch block

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b9a73d412e09fbf2963f3e5edd13559e43604a78

Please document what the 0x80000000 is about.
Maybe best to declare it as a constant somewhere

Added NS_FAILED helper to MsgUtils.

+          let msgHdr = Cc["@mozilla.org/messenger;1"]
+            .createInstance(Ci.nsIMessenger)
+            .messageServiceFromURI(originalMsgURI)
+            .messageURIToMsgHdr(originalMsgURI);
+          let folder = msgHdr.folder;

Instead of try/catch. To handle .eml messages, you should just check if folder is set. If it's not, it's a standalone .eml

The exception is thrown by messageServiceFromURI before I can check .folder, I moved code out of the try block to make it clearer.

Add newline to the end of .ini files.

Instead of these two, can we just use Components.isSuccessCode()?

Thanks, didn't know about isSuccessCode.

Maybe just use Cu.reportError(e) though, like

I feel it can't be considered an error here, so I used console.warn

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/13b7004235b7
Enable browser_attachmentReminder and browser_emlActions for MessageSend.jsm. r=mkmelin

• Support embedded images
• Enable browser_blockedContent

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4260dfb9bf0118d373ec114ad551b40350e1a933

Can we avoid passing in an editor. Seems more than a bit wrong... It shouldn't really have anything to do with MimeMessage

Good suggestion, I moved all interaction with editor to MessageSend.jsm and passed embeddedAttachments to MimeMessage.

+      // Convert UTF-16 string back to byte string.

You're all in js here. Is this needed? Looks to me it would happen automatically?

I added more comments. The reason is the body argument of createAndSendMessage is ACString. Also 8-bit string is used in MimeEncoder to pick encoding and do encoding. And when we fetch an attachment, we get the content as 8-bit string. So we handle message body text the same way as attachment content.

There are some test failures on treeherder related to this. Is this going to clear those up?

Yes, except for test_autoReply. Did a Try run earlier today, https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=849e0e6adcf8f363d1717538a3375c&selectedTaskRun=MDfxv4S0Q0G-UcWeueIIsQ.0 only test_autoReply failed in win10 debug build.

Maybe use use a real Map()

Plain object seems more light weight to me, I renamed it to urlCidCache

• Support forwarding as attachment
• Enable browser_forwardHeaders.js and other forwarding tests.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedTaskRun=D9t94zvyQaSaDQEhUqNkPw.0&revision=b83339f8aa9f4ed9f9d2e985113b705f79815194
The lint error already fixed.

• Add attribute AUTF8String contentId to nsIMsgAttachment and fix forwarding as attachment
• Enable remaining composition mochitests
• Enable test_sendMailAddressIDN.js

Will work on cases not covered by existing tests: like collecting addresses to address book, or sending to newsgroup.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/f4e236a8735f
Enable remaining composition mochitests for MessageSend.jsm. r=mkmelin

A small patch

• collects outgoing addresses to address book
• shows status message in each step
• passes mail.sanitize_date_header pref value to emitStructuredHeaders

Found that when copy operation failed, need to show a dialog with retry button. And need to FilterSentMessage when copy succeeded, will work on them in the next patch.

Not sure why, but the most recent patch that landed has increased the amount of memory leaked dramatically. 40% in the case I looked at. It's not because we're running more tests, all of the added tests are run separately. Log of a task that ran the C++ version. Log of a task that ran the JS version.

Now that the JS version of this code is running all of the tests, that test run is also reporting more memory leaked than our threshold.

Folder name can be empty, fix unit tests.

(In reply to Geoff Lankow (:darktrojan) from comment #112)

Not sure why, but the most recent patch that landed has increased the amount of memory leaked dramatically. 40% in the case I looked at.

Ping: please check that patch. It's very non-obvious what it could have caused a leak so probably need to take the patch apart and send a few try runs to compare, until you find the right spot.