437193 - Implement an IMAP fake server testing scheme for /mailnews

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Mark Banner (:standard8)]

This bug is for implementing a IMAP fake server testing scheme for /mailnews.

See URL above for background information on xpcshell tests.

Bug 413077 implemented the original fake server for news, Bug 436847 extended it for SMTP and is therefore a good reference for this bug.

There may be some threading issues with this bug, making a server implementation complicated, but I think we can probably get around those by being careful in what we call when.

Comment 1

[Joshua Cranmer [:jcranmer]]

I'll start on this.

The biggest potential issue I see is that I'm not sure of fakeserver's capability to handle multiple connections at once without dieing. Also, I'll probably need to modify fakeserver to handle the fact that IMAP really doesn't like closing connections.

Comment 2

[Joshua Cranmer [:jcranmer]]

And why can't I reassign to myself and change to ASSIGNED at the same time?

Another few points:
* Some hackery will be needed to handle the IDLE command and other server-proffered response.
* Comprehensive test suiting could be difficult as servers can be more diverse. I'm not intimately familiar with the local IMAP code like I am with NNTP, and I'm still learning IMAP myself. But I'm confident it will work out in the end.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> * Comprehensive test suiting could be difficult as servers can be more diverse.

As you probably can guess, having anything basic in the first instance would be better than nothing (e.g. just being able to check the protocol for checking new mail)...

Comment 4

[David :Bienvenu]

Writing an IMAP server, even a fake one, is a huge task :-)

Here are a few basic things that have given us trouble in the past (they all work now, of course) :

Hierarchy delimiters other than '/', e.g., '.'
Having all folders under the Inbox
UID's > 2MB (i.e., a 32 bit int with the high bit set)

Adding CONDSTORE support would be helpful for me, but that's a long way down the road.

Comment 5

[Joshua Cranmer [:jcranmer]]

Update for those interested:

My fakeserver locally can now support the following commands:
CAPABILITY, NOOP*, LOGOUT, STARTTLS*, AUTHENTICATE*, CREATE, SUBSCRIBE, UNSUBSCRIBE, LIST, LSUB. *s indicate that it doesn't work fully (NOOP doesn't do status updates, STARTTLS is just there to fool mailnews, and AUTHENTICATE doesn't do any checking whatsoever, so don't try to use anything other than PLAIN!) RENAME, DELETE, and LOGIN support should be finished shortly.

Logic for mailbox manipulation is nearly complete, but message support is still not existant, and is looking to be proving a problem. Specifically, I've not found anything accessible from script in libmime that would help with FETCH BODY[] semantics, which means I'll either have to fix libmime (yuck) or roll my own MIME parsing semantics (double yuck).

I also have the start of guides on how to use IMAP fakeserver, obviously depending on how I go about the future completion of fakeserver. Namespace support is implicit in my design (good generic handling without implicit namespace-esque designs borders on the depths of insanity); however, the "" namespace and INBOX are finicky little buggers requiring special handling. The breakout character handling is not yet fully usable in LIST/LSUB, as I am still pondering the best method to go about handling this.

Trying to implement i18n concerns in IMAP is proving to be difficult, as the modified UTF-7 syntax and proper case-insensitivity, combined with the loveliness of JS strings present multiple headaches. UTF8 support for IMAP is still very much in draft stages, but according to Crispin, "IMHO, a server that offers case-insensitive mailbox names and UTF-8 mailbox names should be case-insensitive for all of Unicode." Once this draft becomes RFC, I think it is very worthwhile to make mailnews use it and push it if only for my sanity.

Since btoa/atob are not accessible from xpcshell JS, I rolled my own functions for these values and am wondering where would be the best place to put them.

The RFC extensions are a veritable problem: with 2 dozen separate features and a few more in the terms of capabilities, not to mention different interpretations of MAY and SHOULD in RFCs (represented via daemon flags), a full feature cross would be in the millions of entries, making the theoretically strong cross impossible. However, NAMESPACE support shouldn't make any bearing on whether or not we handle CONDSTORE, for example. Handling will end up being different than how I handle NNTP (amended fakeserver as described in bug 16913), although how it works is still mostly undecided. What I'm looking at, though, is something along the lines of |makeDaemon(flags, "CONDSTORE,NAMESPACE")| with predefined |makeUWDaemon| and |makeZimbraDaemon| (as examples).

In other news, imapd.js is at 613 lines already, more than the other extant daemons combined, and I'm not through with the basic RFC 3501 support. Complex spec...

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: WIP

For those interested in seeing what it looks like right now, this is where I'm at.

Unimplemented commands:
LOGIN
STARTTLS
SEARCH
parts of FETCH
AUTHENTICATE if failure

I don't handle literals yet, and I've found a better way to handle the arg matching, that should also handle data in a better way. Status updates are not supported (so neither NOOP or CHECK will update!), and multiple connections are still something I need to play around with.

I've been able to confirm that, at least in the case of UTF8, fakeserver doesn't have any problem with handling that data, except that it comes in form of "\u00a0\u008f" instead of the UTF-16 people are used to. I18n mailbox names fall under the "don't go there category." Documentation is still very much WIP.

I've been testing locally using a hand-written qaserver.sh file. That file is a way to start up a fakeserver (IMAP, NNTP, POP, or SMTP, although I've only played with IMAP and NNTP) so as to be able to triage bugs dependent on server environments. There are still bugs in the file, but it is reliably usable for basic QA'ing.

Comment 7

[Joshua Cranmer [:jcranmer]]

Attachment: patch, version 1

And here is a completed version of the fakeserver.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 332250 [details] [diff] [review]
patch, version 1

>Passes 2 of 20 tests



>diff --git a/mailnews/imap/test/unit/head_server.js b/mailnews/imap/test/unit/head_server.js
>+function setupDaemon() {
>+  var daemon = new imapDaemon();
>+}

This function isn't used / doesn't do anything.


>diff --git a/mailnews/imap/test/unit/test_mailboxes.js b/mailnews/imap/test/unit/test_mailboxes.js

>+  // Make sure that the folders that we want to exist exist and that the others
>+  // do not. Also make sure that i18n works.
>+  var rootFolder = localserver.rootFolder;
>+  do_check_true(rootFolder.containsChildNamed("Inbox"));
>+  do_check_true(rootFolder.containsChildNamed("I18N box\u00E1"));
>+  do_check_false(rootFolder.containsChildNamed("Unsubscribed box"));

I don't see how you are check that "others do not" here...

>diff --git a/mailnews/test/fakeserver/imapd.js b/mailnews/test/fakeserver/imapd.js
>+    if (wantHeaders) {
>+      // It's an XML string... we now need to parse it
>+      var dmParser = Cc["@mozilla.org/xmlextras/domparser;1"]
>+                       .createInstance(Ci.nsIDOMParser)
>+                       .QueryInterface(Ci.nsIDOMParserJS);
>+      var doc = dmParser.parseFromString(requestListener.answer, "text/xml");
>+      var children = doc.documentElement.firstChild.childNodes;
>+      var headers = {}
>+      for (var i=0; i < children.length; i++) {
>+        var element = children.item(i);
>+        headers[element.getAttribute("field")] = element.lastChild.nodeValue;
>+      }
>+
>+      return headers;
>+    } else {
>+      return requestListener.answer;
>+    }

No need for the else here.

This patch currently breaks (at least) the test_filter.js test (in news).

I'm happy with the general structure, but I'd like David to review this as well. He is far more familiar with IMAP servers than I am, and is more likely to be using it and getting into the guts of it.

Comment 9

[David :Bienvenu]

Some questions:

do you enforce tag uniqueness? If not, probably worth mentioning in a comment. We've never had a problem with generating unique tags since we use a monotonically increasing number, but still worth mentioning.

Are you not supporting IDLE? If not, worth noting. It's a useful thing for us to be able to test, since we've had issues in the client with our idle support.

I have not had a chance to test this, but it's very impressive so far! I'll wait for your next patch to do the sr.

Comment 10

[Joshua Cranmer [:jcranmer]]

(In reply to comment #9)
> do you enforce tag uniqueness? If not, probably worth mentioning in a comment.
> We've never had a problem with generating unique tags since we use a
> monotonically increasing number, but still worth mentioning.

No, I don't enforce tag uniqueness, in fact, I don't even support certain tags (i.e. "APPEND" won't work as a tag).

> Are you not supporting IDLE? If not, worth noting. It's a useful thing for us
> to be able to test, since we've had issues in the client with our idle support.

It's something that would require a bit more fiddling with code for notifications. My plan was to implement (most of it, at least, the harder parts being saved until later) the official IMAPv4r1 RFC as the base, as well as an easy-to-implement extending RFC (the NAMESPACE RFC) to show how it was done.

Comment 11

[Joshua Cranmer [:jcranmer]]

Attachment: patch, version 2

I've changed the processNextEvents to all false instead of true, which should fix the hanging problems. The only way to reliably reproduce them for me was to make -s tier_app and then immediately |make -C mailnews/news check|.

Also did Standard8's nits.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 335783 [details] [diff] [review]
patch, version 2

Unfortunately, test_filter.js still seems to be hanging (note: I'm running under heavy load at the moment).

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 335783 [details] [diff] [review]
patch, version 2

After several hours of debugging, I found the hang/crash problem.

This bit is effectively unchanged:

324   onInputStreamReady : function (stream) {
325     if (this._server.observer.forced)
326       return;
327 
328     this._server.timer.cancel();
329     try {
330       var bytes = stream.available();
331     } catch (e) {
332       // Someone, not us, has closed the stream. This means we can't get any
333       // more data from the stream, so we'll just go and close our socket.
334       this.closeSocket();
335       return;
336     }

This bit is the change that breaks the above code:

   closeSocket : function () {
+    this._signalStop = true;
+  },
+  _realCloseSocket : function () {
     this._isRunning = false;
     this._transport.close(Cr.NS_OK);
     this._server.stopTest();

The change to this function means that if there is no data available, this._signalStop is set to true, but _realCloseSocket never gets called :-(

I think for some reason this must be more likely to happen on mac.

I fixed it by adding a call to _realCloseSocket before the return statement.

Running unit tests, I now get another error in test_pop3GetNewMail.js:

*** TEST-UNEXPECTED-FAIL | ../../../mozilla/_tests/xpcshell-simple/test_mailnewslocal/unit/test_pop3GetNewMail.js | TypeError: real.them is undefined
JS frame :: /Users/moztest/comm/main/src/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 101
JS frame :: ../../../mozilla/_tests/xpcshell-simple/test_mailnewslocal/unit/test_pop3GetNewMail.js :: anonymous :: line 52
*** test finished

Comment 14

[Joshua Cranmer [:jcranmer]]

(In reply to comment #13)
> The change to this function means that if there is no data available,
> this._signalStop is set to true, but _realCloseSocket never gets called :-(
> 
> I think for some reason this must be more likely to happen on mac.
> 
> I fixed it by adding a call to _realCloseSocket before the return statement.

The better fix is to leave closeSocket() for the external API usages only and use _realCloseSocket instead.

> Running unit tests, I now get another error in test_pop3GetNewMail.js:
> 
> *** TEST-UNEXPECTED-FAIL |
> ../../../mozilla/_tests/xpcshell-simple/test_mailnewslocal/unit/test_pop3GetNewMail.js
> | TypeError: real.them is undefined

This popped it into an array... I made the necessary fixes locally, will post a patch this afternoon.

Comment 15

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 3

This patch passes all tests locally on my computer, and includes the fix mentioned above.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 344112 [details] [diff] [review]
Patch, version 3

>diff --git a/mailnews/test/fakeserver/imapd.js b/mailnews/test/fakeserver/imapd.js
>new file mode 100644
>--- /dev/null
>+++ b/mailnews/test/fakeserver/imapd.js
>@@ -0,0 +1,1526 @@
>+// This file implements test IMAP servers
>+

>+    var daemon = this;
>+    function part2num(part) {
>+      if (part == '*') {
>+        if (uid)
>+          return daemon._selectedMailbox._highestuid;
>+        else
>+          return daemon._selectedMailbox._messages.length;
>+      } else {
>+        return parseInt(part);
>+      }

No need for the else statements here.

>     var thread = gThreadManager.currentThread;
>     while (!this.isTestFinished())
>-      thread.processNextEvent(true);
>+      thread.processNextEvent(false);
>   },

I'm not sure we need to do these changes now we've fixed the problem with the hanging. Could we try backing them out? (I'm willing to try it again on my machine).

>+/**
>+ * Converts a base64-encoded to a string with the octet data.
>+ *
>+ * The extra parameters are optional arguments that are used to override the
>+ * official base64 characters for values 62 and 63. If not specified, they
>+ * default to '+' and '/'.
>+ *
>+ * No unicode translation is performed during the conversion.
>+ *
>+ * @param str    A string argument representing the encoded data
>+ * @param c62    The (optional) character for the value 62
>+ * @param c63    The (optional) character for the value 63
>+ * @return       An string with the data
>+ */
>+function btoa(str, c62, c63) {
...
>+/**
>+ * Converts a string or array of octets to a base64-encoded string.
>+ *
>+ * The extra parameters are optional arguments that are used to override the
>+ * official base64 characters for values 62 and 63. If not specified, they
>+ * default to '+' and '/'.
>+ *
>+ * Data is treated as if it were modulo 256.
>+ *
>+ * @param str    A string or array with the data to be encoded
>+ * @param c62    The (optional) character for the value 62
>+ * @param c63    The (optional) character for the value 63
>+ * @return       An string with the encoded data
>+ */
>+function atob(arr, c62, c63) {

As discussed on irc these need to be the other way around according to devmo's documentation (https://developer.mozilla.org/en/DOM/window.atob).

r=me with those fixed, but please get additional review from bienvenu as he's the IMAP expert.

Comment 17

[David :Bienvenu]

in general, the imap stuff looks good. However, can you either add UIDPLUS support (meaning an APPEND returns the UID of the newly appended message), or SEARCH? Otherwise, drafts will proliferate.

Do you support user-defined flags/keywords? That would be a great thing to have as an option, that we can turn on and off.

Comment 18

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 4

Standard8's comments, along with some more things:
1. Minor fix: I forgot to check for nullity on one argument.
2. I added (partial) support for RFC 4315 (UIDPLUS), per bienvenu's request (I don't support UID EXPUNGE yet).
2a. IMAP extensions now have a preload so they can save off parent functions to call.

(carrying forward r+).

Comment 19

[David :Bienvenu]

Comment on attachment 344488 [details] [diff] [review]
Patch, version 4

thx, jcranmer, this will be very helpful.

Comment 20

[Joshua Cranmer [:jcranmer]]

Checked into comm-central:
changeset:   742:a8d4959143e5