885110 - [email] Switch to whiteout-io email.js libraries from AMD-wrapped and shipped node libraries (mailparser, mimelib, mailcomposer, simplesmtp, addressparser)

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

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

We use a bunch of Andris Reinman's node libraries that have now been ported to be web-platform friendly (including some APIs that Mozilla is trying to get standardized like TCPSocket) in https://github.com/andris9/firemail.  We should switch to firemail from the node libraries.

The node libraries are currently AMD-wrapped using volo and then rely on our Buffer shim and/or other shimmed modules we provide.  In some cases, we have embedded useful logic in our shims that is required because of differences between the web platform and node that should be contributed back to "firemail".  For example, some regexp transforms are required for encoding names that iconv understands but TextDecoder does not understand.  Also, base64 decoding by window.atob is much more strict than node's Buffer's base64 decoding.

A related bug is bug 885105 wherein we want to switch to the 'inbox' library's IMAP protocol implementation; the 'inbox' library is not yet in the firemail library, but may end up there.

Comment 1

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

The state of the art has advanced.  Andris's efforts are now http://emailjs.org/.

Comment 2

[andris.reinman]

Where can I find information about encoding names that need to be modified before passed to TextDecoder? Currently there is some minimal normalization done in email.js, like renaming latin-1 (TextDecoder understands latin1 but not latin-1) to iso-8859-1 but most probably this is not enough (https://github.com/whiteout-io/mimefuncs/blob/master/src/mimefuncs.js#L1001-L1011)

Additionally, what about the atob vs node buffers issue mentioned above? Is there some modified base64 scheme that atob does not understand but node buffers do? We need to do something with the atob anyway, since it is not supported in iOS WebWorkers, so maybe it would be more reasonable to replace it entirely, not just use a polyfill for iOS.

Comment 3

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

(In reply to andris.reinman from comment #2)
> Where can I find information about encoding names that need to be modified
> before passed to TextDecoder? Currently there is some minimal normalization
> done in email.js, like renaming latin-1 (TextDecoder understands latin1 but
> not latin-1) to iso-8859-1 but most probably this is not enough
> (https://github.com/whiteout-io/mimefuncs/blob/master/src/mimefuncs.js#L1001-
> L1011)

https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/js-shims/faux-encoding.js

Related unit tests:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/test/unit/test_intl_unit.js

> Additionally, what about the atob vs node buffers issue mentioned above? Is
> there some modified base64 scheme that atob does not understand but node
> buffers do? We need to do something with the atob anyway, since it is not
> supported in iOS WebWorkers, so maybe it would be more reasonable to replace
> it entirely, not just use a polyfill for iOS.

https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/node-buffer.js#L24
(The comment explains the differences)

Comment 5

[Edward Chen[:edchen]]

No testcase, this is regression test corver it.

Comment 6

[Marcus Cavanaugh [:mcav]]

An update, as I mentioned I'd have something ready today:

TLDR: You can look at the branch; the tests pass, I've confirmed that it works in nightly (sending and receiving IMAP), and I'm now in the process of taking another pass through the diff to catch the inevitable "WTF were you thinking" parts that I missed while getting things nailed down. Ugly/potentially-rough diff, things clearing up within the next couple of days.

As we talked about briefly on Monday, since this change required a giant upheaval of dependencies, I took the liberty to move a bunch of stuff around, clean up the build process, and pay off a bit of technical debt.

Presently (and I'll describe this more later), most of our stuff lives in js/, and the dependencies live in js/ext/ (just like Gaia), and any module in either of those directories can be referenced from an absolute path. There's a unified RequireJS config in js/gelam-loader.js, which is invoked for both the tests and in Gaia.

If you'd rather rename/relocate the 'js' and 'js/ext' directories, that's cool; I'm not particularly attached to either (on Monday it was 'src'!). My rationale for these was that (a) they're the same as most other apps in gaia, and (b) we didn't really use any directories except for the data/lib/mailapi and deps/.

And, since apparently I couldn't help myself, as I dove into the build/require/optimization/paths stuff I took a stab at cleaning that up too: rather than doing the concatenation in gelam, I think an argument could be made to do all of that within Gaia: All of the previous r.js optimization lived in 'install-into-gaia', so it was already Gaia-specific, and having two different RequireJS optimization layers was really hurting my brain.

However, I got stuck in a tar pit of path stuff (TMI) and haven't had a chance to finish my speculative attempt at all the build optimization. There's no reason to block the rest of the review cycle on that, so skip the configuration stuff for now, or alternately note that it isn't ready for review yet.

Similarly, I'm still cleaning up the rest of the patch, but this should give you a pretty good idea of whether or not there's anything in your error-handling-stuff that might collide.

I feel time-pressure to ship this off ASAP so that you can review, so here's some scheduley things: 
I expect to complete my first cleanup pass on this tomorrow. If left to my own devices, I'd have the real review request ready for you on Friday, which would give us a week-to-ten-days for back-and-forth. If that's swingable for you, great, otherwise just know that you'll see things-to-fix if you look at it too closely within the next day or two.

https://github.com/mcav/gaia-email-libs-and-more/compare/mozilla-b2g:master...mcav:emailjs

Comment 7

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

Apologies, didn't mean to emit time-pressure at Tuesday's meeting.  Just wanted a ballpark on ETA and churn factor to figure out if my own schedule ordering between error domains and integration tests made sense.

I think it makes sense to wait to do the review on Friday or whenever you're happy with the code or just have code-blindness and want someone else to take a look at it.  Neither of us gain anything if I'm telling you to fix things you already know you need to fix.

For my purposes, I'm good knowing that things are sufficiently stable that in the event I get the integration tests sufficiently squared away that I can just base my branch on yours.  I have the critically-important:
"""
[rerere]
	enabled = true
"""
in my ~/.gitconfig so as long as all your file re-arranging is pretty much done, I should be able to rebase on top of any rebases you make/etc. without any troubles.

The path stuff is fine; I think we've briefly discussed that elsewhere and a cleanup was absolutely overdue.

In terms of install-gaia and optimizations, I've often thought the same thing myself.  Just straight-up propagating the unoptimized GELAM into gaia I think would do amazing things for our sanity and for improving branching/uplifts.  I do agree that it makes sense to defer cleaning that up until after the other core stuff in this patch is done.

Thanks again for undertaking this very significant set of improvements and cleanup for GELAM!  It's going to be so fancy we'll have to call it GLAM afterwards... ;)

Comment 8

[Marcus Cavanaugh [:mcav]]

Attachment: GELAM patch

Andrew, here's the GELAM patch. I'm still working through some Gaia configuration weirdness, but the GELAM side should be more or less ready for review.

Things left to do on the Gaia side:

- Something's wrong with the requirejs config causing 'mailbuild' to fail to load. Going to ping james about this shortly.

- The gaia tests aren't yet running (again, some sort of config thing); working on that today.

All the GELAM tests pass unless I've screwed something up this morning.

I'll post continued updates here.

Comment 9

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

Comment on attachment 8478469 [details] [review]
GELAM patch

We did the review largely on the etherpad at:
https://etherpad.mozilla.org/emailjs

r=asuth on all the GELAM non-packaging parts.  :jrburke reviewed/revised the packaging stuff in conjunction with mcav.

Major props to :mcav on this significant and important undertaking!  Hip hip, hooray!


I'm archiving my requested follow-up things below so that they don't get lost in event of etherpad problems.  Otherwise there's just the newline issues mcav is finalizing fixes of currently and potentially dealing with the rootDelim problem in composite/configurator.

### upstream tracking ###
the following monkeypatches need to be tracked for upstreaming in some combination of BMO bugs and upstream github issues:

    IMAP (browserbox)

    STARTTLS

    error reporting (the English error messages without a specific type/identifier are troublesome and have been hacked around)

    should also put forth exposure of our regexping/inference of server state from the specific messages the server tells us as a useful thing to put in browserbox.  There isn't a ton of benefit to anyone if everyone duplicates this.  (The one pyrrhic upside is that if everyone rolls their own, you don't run into problems where you don't have the strings/UI for a newly detected error scenario.)

    SMTP (smtpclient)

    eh, same as the IMAP cases.  but it's SMTP.

### follow-up bugs to file ###

    High-priority: get browserbox APPEND to support Blobs.  This is potentially a non-trivial regression when attachments are involved.

    get our other encoding aliases upstreamed and thereby back into our codebase.  from asuth's notes:

    encoding  aliases: faux-encoding.js had a bunch of aliases for what TextEncoder  didn't provide.  verify those went to the right place.  (we do have unit  tests for these, so we are probably fine if they were translated/etc.  appropriately)

    mimefuncs.js' mimefuncs.charset.normalizeCharset covers:

    /^utf[\-_]?(\d+)$/i is identical to our /^utf[\-_]?(\d+)$/ yay

    /^win[\-_]?(\d+)$/i is NOT identical to our /^(?:(?:win(?:dows)?)|ms)[\-_]?(\d+)$/

    missing various things like "ms949", "windows_949".

    /^latin[\-_]?(\d+)$/i is identical to our /^latin[\-_]?(\d+)$/ yay

    missing: /^us_?ascii$/ mapping to ascii

Comment 10

[James Burke [:jrburke]]

Comment on attachment 8478469 [details] [review]
GELAM patch

r+ on the packaging. Some of these notes are more relevant when also considering the matching changes in the gaia branch, but included here, since they are part of the overall goal and guide the choices in gelam:

1) gelam -> gaia code transfer is now a plain source copy instead of an optimized build copy. We decided to try this to allow easier source debugging, and to also match with a general practice of only doing optimization steps in the final app, which may have optimizations goals that cannot be guessed upstream.

This means the gaia build for the email app is just a bit longer than before. However, when uglification is being done, uglifying is still the more noticeable delay.

2) The loader config is now only needed inside the worker -- main-frame-setup and its friends can all be loaded using loader conventions.

3) With the updated gaia build changes to match, this ends up with an application.zip file size of 595,136 bytes vs 579,762 on master. Given that the new whiteout IO libraries are slightly bigger, a size increase is expected, and it seems like we contained it fairly well.

4) This is a test-perf capture of the gaia-related branch for this change vs. master. It is not a completely direct comparison, the gaia branch is a bit behind master now, but fairly close. I could not grab the memory usage via test-perf due to bug 1059642. I cut out the individual runs for brevity, here are the averages:

## no account

```
[
{
  "passes": [
    {
      "title": "startup > ",
      "old-mozPerfDurationsAverage": 1678.3095832499998,
      "new-mozPerfDurationsAverage": 1768.27609375
    },
    {
      "title": "startup > moz-chrome-dom-loaded",
      "old-mozPerfDurationsAverage": 444.9304425,
      "new-mozPerfDurationsAverage": 467.37446675
    },
    {
      "title": "startup > moz-app-visually-complete",
      "old-mozPerfDurationsAverage": 445.58680974999993,
      "new-mozPerfDurationsAverage": 467.764974,
      "mozPerfGoal": 1000
    },
    {
      "title": "startup > moz-content-interactive",
      "old-mozPerfDurationsAverage": 1339.9293619999999,
      "new-mozPerfDurationsAverage": 1388.88622425
    },
    {
      "title": "startup > moz-chrome-interactive",
      "old-mozPerfDurationsAverage": 1340.3024607500001,
      "new-mozPerfDurationsAverage": 1389.2609377499998
    },
    {
      "title": "startup > moz-app-loaded",
      "old-mozPerfDurationsAverage": 1888.0595832499998,
      "new-mozPerfDurationsAverage": 2003.27609375
    }
  ]
}
]
```

## one account configured (IMAP)

```
[
{
  "passes": [
    {
      "title": "startup > ",
      "old-mozPerfDurationsAverage": 3546.846666,
      "new-mozPerfDurationsAverage": 3678.743762
    },
    {
      "title": "startup > moz-chrome-dom-loaded",
      "old-mozPerfDurationsAverage": 597.07334625
      "new-mozPerfDurationsAverage": 585.2658590000001
    },
    {
      "title": "startup > moz-app-visually-complete",
      "old-mozPerfDurationsAverage": 598.017526,
      "new-mozPerfDurationsAverage": 585.86888
      "mozPerfGoal": 1000
    },
    {
      "title": "startup > moz-content-interactive",
      "old-mozPerfDurationsAverage": 1731.9482552499999,
      "new-mozPerfDurationsAverage": 1758.658281
    },
    {
      "title": "startup > moz-chrome-interactive",
      "old-mozPerfDurationsAverage": 1732.3910282499999,
      "new-mozPerfDurationsAverage": 1759.0235415000002
    },
    {
      "title": "startup > moz-app-loaded",
      "old-mozPerfDurationsAverage": 3776.846666,
      "new-mozPerfDurationsAverage": 3904.493762
    }
  ]
}
]
```

On the mozPerfGoal tracked item, we are still well below the threshold, but we do have a slight increase in the numbers. Some of the longer numbers, where we wait for full worker and account spin up, might have taken a hit because the gelam optimization layers are simpler, just one for the worker-bootstrap, composite/configurator and activesync/configurator.

Some of the other dynamically loaded modules were bundled in old master, but the bundling led to some duplication in some layers. So this new approach is fewer bundles which means more separate module loads at the tail end, but avoiding the duplication of loaded code.

Since this changeset is needed to set up some other future features, and is the library back-end that is likely to get good support in the future, I think it is worth delivering it now, and we can do another sanity check once on master and look for further perf optimizations as part of bug 1059358.

Comment 11

[andris.reinman]

We are already working on STARTTLS both for Browserbox and smtpclient but we have to implement upgradeToSecure in the TCPSocket Chrome shim first. Once we have the upgrade working, we'll add STARTTLS as well.

Comment 12

[James Burke [:jrburke]]

Comment on attachment 8478469 [details] [review]
GELAM patch

Forgot to actually flip the flag.

Comment 13

[Marcus Cavanaugh [:mcav]]

Landed! Thanks a bunch to Andrew and James for the quick turnaround and assistance with modules and optimization.

GELAM master: https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/04f59329a2405f5c51d6d98f76eff975f0e24161

Gaia master: https://github.com/mozilla-b2g/gaia/commit/8d965e7182500fd1849e8eec5ae2aca35a55af22

Will close momentarily after filing tracking bugs for the followup issues.

Comment 14

[Marcus Cavanaugh [:mcav]]

The corresponding Bugzilla bugs for the aforementioned issues are listed below; they include links to the corresponding GitHub issues on whiteout-io/browserbox and whiteout-io/smtpclient.

https://bugzil.la/1060558
https://bugzil.la/1060566
https://bugzil.la/1060564

Comment 15

[Tony Chung [:tchung]]

Verified per code inspection.