1097421 - [email] Upgrade email.js omnibus bug

Status: RESOLVED FIXED

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

Description

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

There's a bunch of things that will be fixed by taking email.js upgrades:

== Things we've seen:

- STARTTLS related:
  - Bug 1060558 was our goal to have email.js support STARTTLS and for us to converge.  This got implemented upstream by :andris9 as https://github.com/whiteout-io/browserbox/issues/20 (yeah!), we then requested a requireTLS variant as https://github.com/whiteout-io/browserbox/issues/28 which also got implemented upstream by :andris9 (woo!).  There was a security edge case, for which I provided a fix at https://github.com/whiteout-io/smtpclient/pull/20 which has been upstreamed and published.
  - We have a email.js/v2.1 LOGINDISABLED regression relating to STARTTLS on bug 1080543.  Doing the above switch basically addresses the problem.

- Folder sync bugs with new fixes:
  - Namespace NIL delimiter fix I provided to upstream: https://github.com/whiteout-io/browserbox/pull/36 (not yet taken)
  - Addressing our bug 925480, normalized Inbox/INBOX case handling (as observed on gmail), proposed by us on https://github.com/whiteout-io/browserbox/issues/18 and implemented by :andris9 (woo!) on https://github.com/whiteout-io/browserbox/issues/19
  - Our createFolder implementation broke, giving us 1096058 where if we go to create a Sent/Trash folder the account job-op queue hangs.  I'm fixing this now and it's going to entail providing an enhancement fix to upstream to implement a (trivial) API for CREATE.
- Folder sync bugs I'd fixed locally and provided to upstream but we had not taken the right way:
  - NIL delimiters for LIST https://github.com/whiteout-io/browserbox/pull/27 and LSUB https://github.com/whiteout-io/browserbox/pull/29 (discovered while investigating bug 1091295 and bug 1084216)

- Message sync bugs
  - trailing whitespace in bodystructure list fix I provided to upstream: https://github.com/whiteout-io/imap-handler/pull/8 (not yet taken)  (discovered while investigating bug 1091295 and bug 1084216)

- Misc:
 - We had a local fix to address corruption during body parsing that had not been upstreamed, I've issued an upstream pull request for that at https://github.com/whiteout-io/browserbox/issues/35 and we're effectively keeping our fix (not yet taken)

== Things we haven't seen but it's scary to not have the fix:
- IMAP parser issues:
  - atoms that start with [: https://github.com/whiteout-io/imap-handler/pull/7
  - zero length literal edge case: https://github.com/whiteout-io/imap-handler/pull/5

Comment 1

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

Attachment: GELAM PR, commits are split up by bug to the extent possible

Review should probably be done commit-by-commit since most of the changes are small orthogonal changes with the test changes potentially being the most confusing bits (but not so confusing in isolation.)  Please do read the commit comments as I tried to write useful things there.

Note that all changes in js/ext/ that aren't shims are part of upstream except my createFolder changes for bug 1096058 and I believed I've satisfactorily reviewed/audited all changes that weren't frmo me.  I do expect upstream to accept the patch or something very similar to the patch.  Whenever I've pulled a fix from upstream I've tried to do it with volo update.  The browserbox update is the exception, where I've just copied the files over from my branch, but you can see the diff there is just what I changed.

Comment 2

[James Burke [:jrburke]]

Comment on attachment 8528803 [details] [review]
GELAM PR, commits are split up by bug to the extent possible

I focused on the the code changes outside of js/ext, because as mentioned, they are upstreamed from repos that have had their own discussion/review. There are some that have not been merged upstream, but they look straightforward to me.

Tested on device with ActiveSync and Gmail/IMAP accounts. Those accounts did not exhibit the bug fixes rolled up in this ticket, so this just confirms that basic use is still fine as compared to before the change. Ran the gelam tests locally too.

While I am less versed with the protocols, all the changes are reasonable, and due to the tests that were added, give extra assurance of the changes.

The suggestion of taking each change in the separate commits were helpful to process the changes. Just a couple of small comments on the pull request, but just about comment cleanup/metadata listing that is not used in practice.

Comment 3

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

The following servers were broken on v2.1/v2.2 without these fixes.  I've just now tested them with the r+'d patch on v2.2.  IMAP is used in all cases.  I will retest them on a cherrypicked v2.1 uplift before requesting uplift.
- 163.com
- net-c.com
- yandex
- yahoo.co.jp (note that the ISPDB currently says to use POP3

landed on gaia-email-libs-and-more/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/353
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/bca7438c78c83db9eddd508340ab8a7974601a9b

landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/26566
https://github.com/mozilla-b2g/gaia/commit/7119da7a86cd803840678ca3a6067e5622adc481

Comment 4

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

Attachment: landed gaia pull request, cherry-picks cleanly to v2.1, r=jrburke

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Nominally, the email.js bug, bug 885110.  Note that it's not something that could ever be backed out.
[User impact] if declined:  See the various bugs that depended on this or comment 0 for more details.  But in short, there are a number of servers that are not usable in v2.1 without this family oof fixes (163.com/129.com, yahoo.co.jp, yandex.com, net-c.com, certainly others.)
[Testing completed]:
- Automated: Automated tests were added to all changes submitted to upstream (all of which were accepted and landed verbatim).  Automated back-end gaia-email-libs-and-more tests were added as well.  Please see the specific gaia-email-libs-and-more commits for added local tests, or the upstream pull requests referenced for the upstream tests.
- Manual testing on v2.2 flame for all the aforementioned servers plus other smoke-teting.  Manual testing on the patch cherry-picked to v2.1 using a v2.1 b2g-desktop.  
[Risk to taking this patch] (and alternatives if risky):  Low risk.  It is an assembly of fixes, but they're all simple fixes with good test coverage.  Also, this puts us closer to upstream which has their own testing (primarily as it relates to gmail, but they've since opened up to other servers too.)
[String changes made]: No strings changed.

Comment 5

[bhavana bajaj [:bajaj]]

Comment on attachment 8531265 [details] [review]
landed gaia pull request, cherry-picks cleanly to v2.1, r=jrburke

Approving the change on a condition that if we have  any serious blocking fallouts from this landing on 2.1 , we should immediately back this out.

Requesting QA verification by creating accounts on 163.com/129.com, yahoo.co.jp, yandex.com, net-c.com etc and verifying this. Please reach-out to :asuth directly if you neesd more info on the test cases.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/3e286a193cf0154c089a7dc5465da7be7f42ae79