1091295 - [Email] Failure to normalize newlines in composed messages. May cause some SMTP servers (ex: qmail) to refuse to send mail with a 451 error.

Status: VERIFIED FIXED

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

Description

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: logcat of issue on Flame 2.1

Bug was found while branch checking for bug 1084216.

Prerequisite:
1) Have an email account for Yahoo.co.jp
2) Add a Chinese or Japanese keyboard in Settings
3) Have a photo taken by Flame's camera

STR:
1) Set up the Yahoo.co.jp email account with a 6-character-or-more Chinese or Japanese name
2) Send an email with a photo attachment prepared at (3) above

Expected: Email can be sent

Actual: Email cannot be sent. An error message 'Error occurred. Email saved to outbox' is displayed.

Repro rate: 5/5

Attaching a logcat.

Notes:
1) Issue occurs on Flame 2.1 and 2.2 and does not occur on 2.0 (for 2.0 there is another bug 1084216)
2) Issue does NOT occur on affected branches if a photo is NOT attached (however there's still another bug in this scenario, see bug 1084216 comment 11)
3) Issue does NOT occur on gmail

Comment 1

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: screenshot of issue

Occurred on:


Device: Flame 2.1 (shallow flash)
BuildID: 20141029082611
Gaia: 2099fb0df60548cf7d4afc367f5048622cc29b3e
Gecko: f02f3fbd0bb0
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master (shallow flash)
BuildID: 20141029054810
Gaia: a9a847920b51b79c4ad4ad339f0a005777a6228c
Gecko: c6989e473f97
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

-----

Not occurred on (instead bug 1084216 occurred):

Device: Flame 2.0 (shallow flash)
BuildID: 20141029060208
Gaia: 9f5b6f025e528fabfcc068782cb9b492cb51a7f9
Gecko: e652d8489ee8
Version: 32.0 (2.0)
Firmware: v188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

-----

Attaching a screenshot.

Comment 2

[Joshua Mitchell (Inactive)]

NI to Email owner for blocking call

Comment 3

[James Burke [:jrburke]]

It would likely help to get a test account configured correctly for use by dev. Please feel free to privately send the info we could use to set up account in the email app to [e-mail] and I can share it with the other email developers.

Comment 4

[Edward Chen[:edchen]]

I try to use Yahoo! TW mail account, this issue cannot reproduce. Please send JP account to us and than we can figure out what's going on this issue.

Comment 5

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to James Burke [:jrburke] from comment #3)
> It would likely help to get a test account configured correctly for use by
> dev. Please feel free to privately send the info we could use to set up
> account in the email app to [e-mail] and I can share it with the
> other email developers.

Test account credentials has been sent to your email.

Comment 6

[Pi Wei Cheng [:piwei] (inactive)]

Further testing shows that unlike bug 1084216 requires, I actually don't need a 6 chinese characters name to repro the bug. I tried with 4 characters and it repros this bug.

Comment 7

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

From the log we're getting a 451 on the SMTP send, but we're failing to extract anything more useful from that.  Since we have a regression in actually saving/using the (display) name on the account, this is probably something different.

Comment 8

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

I ran with a slightly modified smtpclient and the SMTP response line we're getting is:
  451 See http://pobox.com/~djb/docs/smtplf.html.

This indicates that qmail (which yahoo.co.jp seems to use all over the place) saw an \n when it wanted \r\n.

It's also worth noting that email.js is freaking out when doing an IMAP sync of a welcome mail where there's unexpected whitespace in the BODYSTRUCTURE result.  Filing a separate bug for that, hopefully it will be fixed by our upgrade bug.

Comment 9

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

We're failing to normalize the body payload newlines under v2.1+ in certain circumstances; I modified mail-fakeservers' SMTP server to get sad when it sees \n instead or \r\n, and sad it got.  For v2.0 and earlier versions this appears to not be a problem because mailcomposer would pass the body payload through mimelib's encodeQuotedPrintable method which would perform newline normalization.

The wrinkle is that mailbuild.js which we're using for v2.1+ got smarter.  Without us explicitly specifying a transfer-encoding it now checks whether it needs to use quoted-printable or not.  When it doesn't need to do so, it just claims 7bit encoding and sends things verbatim, which means our \n's stay \n instead of becoming correct \r\n's.

Probably the simplest/safest thing is for us to just run a \n => \r\n transform before issuing calls to setContent on the MimeNode.  Unless I hear otherwise, I'll go with that and clean up the mail-fakeserver's mechanism so it causes tests to fail instead of just getting sad.

Comment 10

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

Attachment: Normalize newlines, modify smtpd server (in mail-fakeservers) to fail without fix, pass with fix

Note that this is not the end of the yahoo.co.jp stuff.  It's still angry about a BODYSTRUCTURE thing.  More fixes coming, plus the test I made to let us ensure that the generated rfc2822 Blob we create looks like what we expect.  That will happen on different bugs for sanity purposes, however.

One extra thing on this bug is tunneling through the server's error messages as errorData.

Comment 11

[James Burke [:jrburke]]

Comment on attachment 8519128 [details] [review]
Normalize newlines, modify smtpd server (in mail-fakeservers) to fail without fix, pass with fix

r+, commented in pull request, but it is more of a comment validating the regexp choice.

Comment 12

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

landed, cursing various system integration tests:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/352
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/f9a111ac1db644b850d1ca59da061e4117fbdbef

https://github.com/mozilla-b2g/gaia/pull/25951
https://github.com/mozilla-b2g/gaia/commit/688c7a2e9d1b163c21b4431c1d67722a144092a9

Comment 13

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

Comment on attachment 8519128 [details] [review]
Normalize newlines, modify smtpd server (in mail-fakeservers) to fail without fix, pass with fix

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): the email.js changes in bug 885110 (which can't be backed out, so don't be getting any ideas!)
[User impact] if declined: People using mail servers that require conformance to internet standards will be unable to send email unless they put something in their message body that is not 7-bit friendly.  The qmail SMTP server (which is popular), for one, gets really upset about people violating standards, which is what is causing the yahoo.co.jp problem.
[Web impact] if declined: Standards will be violated and we will look dumb.
[Testing completed]: Back-end tests have new coverage.  Trunk b2g-desktop tests against yahoo.co.jp.
[Risk to taking this patch] (and alternatives if risky): No risk.
[String changes made]: No l10n string changes made.

Comment 14

[bhavana bajaj [:bajaj]]

BLocking given the regression and QA comments.

:edchen/:piwei, can you help verify this issue on trunk/2.1 ?

Comment 15

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to bhavana bajaj [:bajaj] from comment #14)
> :edchen/:piwei, can you help verify this issue on trunk/2.1 ?

Issue is verified fixed on trunk. Email can now be sent with attachment & Chinese name. Even the issue in bug bug 1084216 is fixed on trunk.

Device: Flame 2.2 Master
BuildID: 20141110053355
Gaia: e02facadb0bc3fe32227b52b31ca47822f7f4322
Gecko: c0d559389a5c
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

2.1 still needs to be checked after it's been uplifted.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/43139177439333d20df5ca4f240ec66f0f9faf5f

Comment 17

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> v2.1:
> https://github.com/mozilla-b2g/gaia/commit/
> 43139177439333d20df5ca4f240ec66f0f9faf5f

The URL doesn't look like a 2.1 merge?

Comment 18

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

(In reply to Pi Wei Cheng [:piwei] from comment #17)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> > v2.1:
> > https://github.com/mozilla-b2g/gaia/commit/
> > 43139177439333d20df5ca4f240ec66f0f9faf5f
> 
> The URL doesn't look like a 2.1 merge?

it's good:

$ git branch -a --contains 43139177439333d20df5ca4f240ec66f0f9faf5f
  remotes/origin/v2.1

Comment 19

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to Andrew Sutherland [:asuth] from comment #18)
> it's good:
> 
> $ git branch -a --contains 43139177439333d20df5ca4f240ec66f0f9faf5f
>   remotes/origin/v2.1

Thanks for checking. Issue is verified fixed on 2.1, and the issue for bug 1084216 is not happening on 2.1 either.

Device: Flame 2.1
BuildID: 20141111072405
Gaia: 4c159e75a1568afbbf0c83c1235ec56facfbe87d
Gecko: d1930a36f9c3
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0