Bug 832924 - [Email] Support POP3 accounts

[mcav]

No description provided.

[lightsofapollo]

these two look almost identical maybe just define incomingInfo once and add the differences in the if statement (looks like just blacklistedCapabilities) ?

[mcav]

++

[lightsofapollo]

Mostly I just left some style nits and codes suggestions (was curious so I skimmed the patch!).

I would also say the pop3 code looks isolated enough that it could (and probably should) be its own lib. The pop3 code is soooo much nicer then our imap.js thing but (IMO) both should be standalone things that don't directly rely on GELAM.

[asutherland]

I think it makes sense to keep this in GELAM until stabilized, then break it out into its own lib. It can be a hassle to propagate across N repositories, so I think it's best to wait to to that until we think the lib is relatively stable.

[asutherland]

put existing debug back

[mcav]

ah, yeah. Done in next commit (and runOp_end as well)

[asutherland]

It looks like the pop3.js fake-server does not perform byte-stuffing so that a line in a mail message of "." gets sent across the wire as "..". I see you've got the byte-unstuffing, but we'll want to modify pop3d.js to run a regexp transform or something and then add a test case.

Right now I'm thinking just add a message to test_mime.js that includes a message with some single-period lines in it. It won't hurt IMAP much to run the test and it provides a little bit of a sanity check.

[asutherland]

The prober is, sadly, too much of an optimist. We need to check that UIDL and TOP work here. I wish I could be an optimist too and have us deal with this when we actually break/see such a POP3 server, but the fact that partners are indicating they only have POP3 available in this day and age also suggests they may have sucky POP3 servers available in this day and age to boot.

Suggest doing "UIDL 1" and looking for an +OK. If we get an -ERR, possibly because the mailbox is somehow empty, we then follow up with just "UIDL". If we still get an -ERR we can conclude the server sucks and refuse to use it. If UIDL worked on "UIDL 1", we issue "TOP 1 0" and take an +OK as success. If UIDL claimed there were no messages we give the server the benefit of the doubt because I fear QA will test with empty folders and file bugs if we fail to create an account in that case.

In the event UIDL or TOP fail, we return a new error code which we shall document in mailapi.js as 'pop-server-not-great'. I was thinking 'pop-server-sucks', but we do surface the error code to the UI and that is perhaps a bit rude. This will need to propagate to your gaia patch too. I suggest an error string like "Server Unsupported. The POP3 server does not implement required features."

We need a canned test for this to go in test_incoming_prober.js.

[mcav]

I added those checks to prober and a couple tests for it; take a look and see if that's what you were thinking.

[asutherland]

For the rename of "test_activesync_general.js" to "test_general.js", the new name is a bit on the "general" side, if you know what I mean. I suggest "test_nonimap_sync_general.js". "test_nonimap_general.js" could also work. Really anything that implies sync and does't imply that IMAP falls into the same bucket.

[asutherland]

can we add a line that has only a period in it too? so "\n.\n" in there somewhere.

[asutherland]

That concludes my initial-ish review pass (including your first big set of review fixes). Still to do for me is to take a look at how you resolve the limited number of outstanding issues and do some on-device testing. I expect to be able to complete device testing on tuesday, so I can r+ once you've fixed the outstanding issues and the GELAM tests are sufficiently green again.

POP3 landing first will cause some bit-rot to my streaming send patch plus there is some fall-out from the mozTCPSocket, so I will need you to do another round on that, so be awares!

[asutherland]

I just ran with your gaia patch on device and POP3 reported a "server-maintenance" error on "server-maintenance | undefined | SecurityUntrustedCertificateIssuerError | undefined" for when my server was accidentally using a self-signed cert.

[asutherland]

More device testing: POP3 downloaded a message that contains a 3087K attachment and for which the body-part is really short, so the initial snippet fetch probably got all of the body part. When I clicked on the message no warning was displayed about it being a huge message. The attachment displayed in the list, but when I clicked on the download button, it just span forever and I got "unsupported op" errors on local_do and do for "download". I need to look at the new size limit logic; I'm assuming the problem is the size is either not allocated or allocated to the attachment but now we're not checking that. (My plan there was to just have a bogus body part in that case, I think.)

Related issue, modtags "do" causes an unsupported op warning to be issued. That's going to be really common and misleading, so let's just put an explicit stub in there for that one for now. (We had discussed removing that warning, but that was in the context of having a unit test that would sanity check the expected ops are implemented instead.)

[asutherland]

I love the breakdown of scope.

[asutherland]

hm, it appears that your gaia commit did not include your recent GELAM commits so all my testing is probably wrong. I'll re-generate the gaia from gelam.

[asutherland]

under the new build, the bad cert error now seems to end up as "POP3 sad. un-responsive-server | Socket exception: [boject Object] | undefined"

[asutherland]

Running with this, I think we need to make the POP3 sync process chattier on logcat for the time being. I would propose indicating the number of messages and UIDLs we are planning to download/downloaded, plus the number of already known messages we are ignoring. (Obviously, don't show the UIDLs of the ones we are ignoring since that will grow without bound and destroy us all.)

[asutherland]

We need to update this value when the (IMAP) body parts are downloaded too. This would go in imapchew.js updateMessageWithFetch I believe.

[asutherland]

Er, but we will also need to modify the log in imap/folder.js in _handleBodyFetcher where we do "if (req.createSnippet)" as the conditional for updating the message header. We will now want to call that unequivocally, probably adding a comment about us always updating the bytes to download number.

[asutherland]

Spin-off bug(s) required to track follow-up coverage of these cases:

• Take smaller sync bites in terms of database commits. Right now POP3 will pull down the snippeted entire contents of the mail store without flushing intermediary state to disk. ActiveSync has a similar problem but is saved by our use of GetItemEstimate to choose a sync range so that there are only ~200 (or less) messages to sync, so it is less capable of foot-gunning itself. This mainly entails calling saveAccountState periodically.
• UX action item bug to deal with the case where it's clear there are a metric ton of messages in the spool. Propose actions like arbitrarily only picking some finite number of them and putting the rest into the UIDL map so we ignore them and only loook for new messages.

[mcav]

Okay @asutherland, I'm ready for another review pass. This time the Gaia update also includes the GELAM changes.

I marked two followon bugs as you described above.

To handle the attachments, I added a fake bodypart to pop3 messages, so that the mailapi doesn't think bodyRepsDownloaded is true until the entire message+attachments have been downloaded. This required a couple minor fixes in the tests/mailchew to ensure that we always have a guard for if (bodyPart.type === 'html') and plain, rather than just "plain vs not plain".

GELAM tests pass apart from a couple flaky AS/IMAP tests which I don't think are my fault but always seems suspicious. ("Cannot create account" and "wrong number of folders")

'bad-security' errors appear to trigger properly now.

I added a bit more logging. Tested on device and it seems to work, including downloading attachments and large message warnings, except that (as described before) large attachments just crash the app with OOM. I was able to download and view a smaller picture though.

[asutherland]

proper bad-security errors confirmed for POP3 being borked by manual testing. And we trust SMTP to do the right thing since it is already known/tested/believed to work.