Last Comment Bug 401673 - Add support for IMAP COMPRESS Extension - rfc4978
: Add support for IMAP COMPRESS Extension - rfc4978
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: Thunderbird 3.0b3
Assigned To: Bron Gondwana
:
Mentors:
http://www.ietf.org/rfc/rfc4978.txt
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-30 01:06 PDT by Nikolay Shopik
Modified: 2010-02-24 16:44 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Implement RFC 4978 IMAP COMPRESS extention (10.58 KB, patch)
2009-05-12 19:12 PDT, Bron Gondwana
no flags Details | Diff | Splinter Review
this time with the newly created files included (18.88 KB, patch)
2009-05-12 20:06 PDT, Bron Gondwana
no flags Details | Diff | Splinter Review
fix condstore breakage (18.85 KB, patch)
2009-05-12 20:17 PDT, Bron Gondwana
no flags Details | Diff | Splinter Review
tidy up initialisation and incorrect usage checking (18.95 KB, patch)
2009-05-13 22:39 PDT, Bron Gondwana
no flags Details | Diff | Splinter Review
tweaks so far (19.06 KB, patch)
2009-05-14 11:36 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
get it building on mac (12.81 KB, patch)
2009-05-15 14:39 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
combine "build on mac" with nsMsgCompress* files (21.53 KB, patch)
2009-05-18 23:47 PDT, Bron Gondwana
no flags Details | Diff | Splinter Review
fix windows build issues... (31.62 KB, patch)
2009-05-19 08:13 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
rewrite compression/decompress paths to simplify buffer use (21.42 KB, patch)
2009-05-19 23:29 PDT, Bron Gondwana
no flags Details | Diff | Splinter Review
bring up to date against trunk. (25.77 KB, patch)
2009-05-20 16:38 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
use Z_SYNC_FLUSH instead of Z_FULL_FLUSH (26.05 KB, patch)
2009-05-21 07:12 PDT, Bron Gondwana
no flags Details | Diff | Splinter Review
now with better "Available()" values and cleaner error checking (28.45 KB, patch)
2009-05-22 22:15 PDT, Bron Gondwana
no flags Details | Diff | Splinter Review
use nsIAsyncInputStream as the input base class (29.51 KB, patch)
2009-05-28 18:47 PDT, Bron Gondwana
no flags Details | Diff | Splinter Review
use nsRefPtr, add some QI's, and use NS_ENSURE_SUCCESS(rv, rv) (22.53 KB, application/octet-stream)
2009-05-29 08:21 PDT, David :Bienvenu
no flags Details
and add nsIInputStream to the interfaces that icompress stream implements (22.62 KB, patch)
2009-05-29 09:01 PDT, David :Bienvenu
mozilla: review+
Details | Diff | Splinter Review
code review changes plus be _really_ strict about zlib logic and error checking (29.68 KB, patch)
2009-05-30 07:50 PDT, Bron Gondwana
no flags Details | Diff | Splinter Review
Check for Z_BUF_ERROR condition and return code of DoInflation() (29.96 KB, patch)
2009-05-31 22:57 PDT, Bron Gondwana
mozilla: review+
Details | Diff | Splinter Review
patch without the QI's in ::BeginCompress (22.88 KB, patch)
2009-06-04 13:50 PDT, David :Bienvenu
neil: superreview+
Details | Diff | Splinter Review

Description Nikolay Shopik 2007-10-30 01:06:58 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8
Build Identifier: 

Seems very new extension but worth adding it in future.

Reproducible: Always
Comment 1 Magnus Melin 2007-10-30 13:28:23 PDT
Confirming RFE.
Comment 2 Bron Gondwana 2009-05-04 03:53:09 PDT
Support for COMPRESS=DEFLATE was just added to Cyrus CVS this week.  It works very nicely.  I've written a basic proxy that injects the commands to enable compress into the IMAP stream and does the compression in Perl.  It reports approximately 80% savings (i.e. 1/5 of the bandwidth use) with typical usage patterns.

This is a pretty significant benefit!
Comment 3 Bron Gondwana 2009-05-12 19:12:38 PDT
Created attachment 377061 [details] [diff] [review]
Implement RFC 4978 IMAP COMPRESS extention

Here's a patch to implement COMPRESS=DEFLATE.  I've generated two new classes which wrap nsIInputStream and nsIOutputStream so they can be transparently layered into nsImapProtocol and everything just-works[tm].

I have also created a preference which allows the user to selectively disable "use_compress_deflate" for servers which have bugs with the implementation.

NOTE - this is only tested against Cyrus and on Ubuntu 9.04.  I have never written anything in the mozilla codebase before, and my C++ is pretty rusty...

Thanks,

Bron.
Comment 4 Bron Gondwana 2009-05-12 20:06:16 PDT
Created attachment 377065 [details] [diff] [review]
this time with the newly created files included
Comment 5 Bron Gondwana 2009-05-12 20:17:03 PDT
Created attachment 377073 [details] [diff] [review]
fix condstore breakage
Comment 6 Magnus Melin 2009-05-12 22:55:37 PDT
Bron, thx for the patch!
For the patch to move on you have to get it reviewed - see https://developer.mozilla.org/en/Getting_your_patch_in_the_tree
Comment 7 Bron Gondwana 2009-05-12 23:42:51 PDT
Thanks Magnus,

I've been emailing directly to David as well (and sent him a copy of each patch as I've done it) - and he looks like a good person to get code reviews from.  I'll wait until it's daytime for him!

I suspect I may want some input from someone experienced in the Makefile side of things to be sure that my ZLIB includes are correct.  I just sort of added them scattershot until things built for me, but that doesn't mean that they will build correctly on all platforms and combinations of build flags.
Comment 8 Ludovic Hirlimann [:Usul] 2009-05-13 00:21:37 PDT
(In reply to comment #7)
> Thanks Magnus,
> 
> I've been emailing directly to David as well (and sent him a copy of each patch
> as I've done it) - and he looks like a good person to get code reviews from. 

He his - I've setup the review flag to david.

> I suspect I may want some input from someone experienced in the Makefile side
> of things to be sure that my ZLIB includes are correct.  I just sort of added

Kairo can you have a look at that part of the patch ?
Comment 9 Robert Kaiser 2009-05-13 09:26:59 PDT
(In reply to comment #8)
> > I suspect I may want some input from someone experienced in the Makefile side
> > of things to be sure that my ZLIB includes are correct.  I just sort of added
> 
> Kairo can you have a look at that part of the patch ?

Standard8 is a better person for that, actually, I don't know much about includes.
Comment 10 David :Bienvenu 2009-05-13 10:29:14 PDT
Comment on attachment 377073 [details] [diff] [review]
fix condstore breakage

this is very cool; I'll try it out later today. I think I probably bit-rotted this patch with my xlist/gmail patch but it should be easy to fix.

one nit I noticed - you don't need to initialize comptrs to null, as that happens for free.
Comment 11 Bron Gondwana 2009-05-13 22:39:36 PDT
Created attachment 377346 [details] [diff] [review]
tidy up initialisation and incorrect usage checking

This version of the patch removes the un-needed initialisations, but adds a couple of cleanups.  m_zstr gets explicitly set to nsnull after being PR_Free()ed, and we check for m_[io]Stream already being defined in Init{Input,Output}Stream calls and return NS_ERROR_UNEXPECTED, since we don't expect to be called twice.

Other than that, it's the same.
Comment 12 Ludovic Hirlimann [:Usul] 2009-05-13 23:30:55 PDT
(In reply to comment #11)
 
> Other than that, it's the same.

You need to request review ...
Comment 13 Bron Gondwana 2009-05-14 00:04:52 PDT
Ahh - thanks.  Still learning my way around...
Comment 14 David :Bienvenu 2009-05-14 06:40:20 PDT
Comment on attachment 377346 [details] [diff] [review]
tidy up initialisation and incorrect usage checking

PR_CALLOC(?) will alloc and clear memory. But I think the current preferred way to allocate memory is NS_Alloc/NS_Free/NS_Realloc.

To free and null out a pointer, you can use PR_FREEIF.

Here, I think you should keep the rv of m_iStream->Available, so something like:

+  nsresult rv = NS_OK;
+  if (m_zlen)
+    *aResult = m_zlen;
+  else
+    rv = m_iStream->Available(aResult);
+
+  return rv;

There are a couple places where you lapse into K&R braces style ;-) our rule is just to be consistent with the prevailing braces style.

Those are all minor things - I can make those changes myself when I de-bitrot the patch and test it out, if you like.
Comment 15 David :Bienvenu 2009-05-14 11:20:32 PDT
I'm having a bit of fun trying to do a debug build with this patch - base/util isn't linking. Debug builds aren't static, so each mailnews dir produces its own dll.
Comment 16 David :Bienvenu 2009-05-14 11:36:23 PDT
Created attachment 377461 [details] [diff] [review]
tweaks so far

this fails to build in base/util in debug builds, which I'll try to figure out next. It contains the changes to memory allocation I suggested earlier, along with a couple other minor changes.
Comment 17 David :Bienvenu 2009-05-14 11:37:13 PDT
if you add ac_add_options --enable-debug to .mozconfig, you'll get a dynamic as opposed to static build.
Comment 18 David :Bienvenu 2009-05-14 15:06:04 PDT
my release mac build doesn't work either - the zlib makefile stuff seems even more arcane than our other makefile stuff.
Comment 19 Mark Banner (:standard8) 2009-05-14 15:16:26 PDT
(In reply to comment #18)
> my release mac build doesn't work either - the zlib makefile stuff seems even
> more arcane than our other makefile stuff.

Well the fact that its ZLIB_LIBS not LIBS_ZLIB isn't going to help. I should probably take a more detailed look at the build config on this patch, I'll try and remember to do that tomorrow.
Comment 20 David :Bienvenu 2009-05-14 15:24:34 PDT
(In reply to comment #19)

> 
> Well the fact that its ZLIB_LIBS not LIBS_ZLIB isn't going to help. I should
> probably take a more detailed look at the build config on this patch, I'll try
> and remember to do that tomorrow.

yes, I noticed that, but fixing it didn't help me. It would be really great if you could take a look at this. If I figure anything out tonight, I'll update the bug.
Comment 21 David :Bienvenu 2009-05-15 14:39:39 PDT
Created attachment 377768 [details] [diff] [review]
get it building on mac

this builds for me on the mac (I've got a little bit of cruft in here for some myrights stuff). It seems to work fine at for retrieving data, but I've seen some i/o errors from the fastmail.fm server when I try to upload 10 or so messages - I don't know if this patch is involved or not...
Comment 22 Bron Gondwana 2009-05-15 17:55:22 PDT
May 15 17:34:03 imap9 slot913/imap[28334]: login: heartbeat1.internal [10.202.2.160] bienvenu plaintext User logged in SESSIONID=<slot913-28334-1242423243-1>                                                                       
May 15 17:34:25 imap9 slot913/imap[28334]: auditlog: append sessionid=<slot913-28334-1242423243-1> mailbox=<user.bienvenu.new sub-folder> uniqueid=<387be9084936f491> uid=<1> guid=<6d7c7e294da7e7907f147f4f8dc05729fd11e291> message-id=<1199911955.6991@estore-bellsouth.com>                                                                       
May 15 17:34:25 imap9 slot913/imap[28334]: IOERROR: reading message: unexpected end of file                       
May 15 17:34:25 imap9 slot913/imap[28334]: Error decompressing data, closing connection                           
May 15 17:34:25 imap9 slot913/imap[28334]: auditlog: traffic sessionid=<slot913-28334-1242423243-1> bytes_in=<23711> bytes_out=<1311>                                                                                               
May 15 17:35:07 imap9 slot913/imap[28334]: login: heartbeat1.internal [10.202.2.160] bienvenu plaintext User logged in SESSIONID=<slot913-28334-1242423307-1>                                                                       
May 15 17:35:25 imap9 slot913/imap[28334]: IOERROR: reading message: unexpected end of file                       
May 15 17:35:25 imap9 slot913/imap[28334]: Error decompressing data, closing connection 

Interesting.  Especially that second one, which suggests you may have been given another connection to the _SAME_ process, which may have not cleaned up its compression state properly.  I'll do some auditing of the Cyrus code involved.

Now - as to the decompressing data error - that's more worrying.  I'll have another look through the Thunderbird code and see if there's any codepath I can find that writes directly to the output without going via the nsMsgCompressOStream object.
Comment 23 Bron Gondwana 2009-05-15 17:56:25 PDT
Sorry - I should have redacted that messageid!
Comment 24 David :Bienvenu 2009-05-16 06:11:34 PDT
(In reply to comment #23)
> Sorry - I should have redacted that messageid!

NP - it's essentially spam from my ISP :-) 

All our output code goes through the same path using m_outputStream. It looks like we're flushing after every write/SendData, so we must be giving all the data, despite the "unexpected end of file" error. My guess would be an issue with nsMsgCompressOStream::Write with biggish buffers - during appends, we send data in 16384 byte chunks.
Comment 25 David :Bienvenu 2009-05-18 14:03:11 PDT
Bron, any more idea what's going on with the server i/o error?
Comment 26 Bron Gondwana 2009-05-18 21:13:03 PDT
Sorry - I've had a crazy weekend.  Haven't had the time or brainspace to deal with this!

I'm just testing now, and totally unable to reproduce it on my machine.  I've tried copying a couple of megabytes email by itself, and copying 900 small messages (from a local folder to the server).  No errors.

What's the HG revision you're building on top of?

What are your compilation options? (.mozconfig)

Here's mine:

ac_add_options --enable-application=mail
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../objdir-tb
mk_add_options AUTOCONF=autoconf2.13

I'm going to try and build on a pristine hg checkout right about when I figure out how to make one of those not containing my experimental commits.  I'm too used to git, where removing random **** I've created is easy...
Comment 27 Bron Gondwana 2009-05-18 21:29:33 PDT
Your "build on mac" patch has a fair bit of "myrights" stuff involved as well.  I'm assuming that's just leakage across from something else you were working on?
Comment 28 Bron Gondwana 2009-05-18 23:47:44 PDT
Created attachment 378266 [details] [diff] [review]
combine "build on mac" with nsMsgCompress* files

This one builds cleanly on top of:

changeset:   2658:c1845fd77bd5
tag:         tip
user:        Bryan W Clark <clarkbw@gnome.org>
date:        Mon May 18 16:50:36 2009 -0700
summary:     Bug 493312 - pinstripe, online/offline button needs padding-start, r=philor

and I can't recreate the upload bug on my machine with it on top of a brand new checkout.
Comment 29 David :Bienvenu 2009-05-19 08:13:06 PDT
Created attachment 378347 [details] [diff] [review]
fix windows build issues...

I forgot to include the new files last time, so this includes them. I needed to add NS_MSG_BASE to the compress classes so that linking works in non-static builds.

I also moved the includes of the compress stream classes to nsImapProtocol.cpp from the .h file, which means nobody else needs to add ZLIB_REQUIRES to their REQUIRES Makefile.in line.

But, sadly, that same append fails. One difference is that I'm doing a debug build, and I'm now running on Windows (Mac, earlier, w/ same error).

ac_add_options --enable-debug
ac_add_options --disable-optimize

enabling debug initializes allocated memory to a well-known value, so if we're somehow using uninitialized memory, it may behave differently in a debug build.
Comment 30 David :Bienvenu 2009-05-19 08:41:37 PDT
I do have a 100% reproducable case - I have a particular message that always fail to append, both when copied alone, or with other messages. I can forward it to you, as an attachment, and you can save the .eml file, add a "From " line at the top, and copy it to your local folders dir, restart tb, and then selecting the .eml folder in local folders and try appending the message yourself...this bug might be very sensitive to the exact bytes in the file, but there's a chance this process will maintain the bytes (we strip off x-mozilla-status headers when we do the append, but that should be the same for you).
Comment 31 Bron Gondwana 2009-05-19 22:35:50 PDT
Hmm.. I don't like the debug build much...

Assertion failure: STOBJ_GET_CLASS(obj) != &js_BlockClass, at /extra/src/hg/commsrc/mozilla/js/src/jsscope.cpp:79

I suspect it's incredibly sensitive.  In fact, it's probably a precise message size issue.  Ouch.

The only thing I can think of is buffer size issues, but deflateBound SHOULD be taking care of those.  I guess it's theoretically possible.  I'll try rewriting that function to double-check them.
Comment 32 Bron Gondwana 2009-05-19 23:29:11 PDT
Created attachment 378534 [details] [diff] [review]
rewrite compression/decompress paths to simplify buffer use

Ok - this one includes a complete rewrite of the buffer handling.  Everything just uses 16k buffers now, and there is no realloc.  Instead it either batches the writeout or returns only partial data to read.

This is a pretty massive simplification of both functions, and it means:

a) no static buffer in the Read code
b) no memcpy in the Read code either - we write straight into the target buffer
c) fewer class variables, just a single struct z_stream and a single char * buffer of fixed size for each direction of each connection.

I have still been unable to recreate the issue even with your example message - and given that it compresses to just over 3k anyway (and the buffer was 8k) it's strange that it had an issue.

Hopefully this change solves it anyway.  It certainly tests the deflate output slightly more in line with the API reference way of doing it.

Bron.
Comment 33 David :Bienvenu 2009-05-20 16:33:56 PDT
sadly, this generates the exact same i/o error - could the problem be on the server? Or in our zlib library? I don't have any access to an other server that does COMPRESS...

I'm going to attach a new diff against the trunk that fixes some thread-safety assertions, and brings us up to date against the trunk.
Comment 34 David :Bienvenu 2009-05-20 16:38:30 PDT
Created attachment 378724 [details] [diff] [review]
bring up to date against trunk.

I accidentally landed the mailnews.js change that added the default server compress pref - it won't break anything, though.
Comment 35 David :Bienvenu 2009-05-20 16:45:33 PDT
FYI - I tried some tiny modifications to the message - change an E to e, and removed a character - neither fixed the problem. I'll try a few slightly more aggressive modifications...
Comment 36 David :Bienvenu 2009-05-20 16:55:49 PDT
I removed a whole paragraph without any difference. In a moment of paranoia, I turned off compression, but the error didn't occur w/o compression.

So perhaps it's something to do with the contents of the message...
Comment 37 Bron Gondwana 2009-05-21 06:53:54 PDT
I suspect it's something in the lead up actually.  I've still been unable to recreate it, even though I copied the exact message your client was appending without compression on from the telemetry logs.

Now - here's something interesting.  The cyrus telemetry log didn't contain anything after the append command:

<1242747079<22 IDLE
>1242747079>+ idling
<1242747096<DONE
>1242747096>22 OK Completed
<1242747096<23 append "INBOX" (\Seen NonJunk) "26-Feb-2009 23:13:57 -0700" {17570+}
>1242747096>23 NO System I/O error
>1242747096>* BAD Invalid tag
>1242747096>* BAD Invalid tag
>1242747096>* BAD Invalid tag
>1242747096>* BAD Invalid tag
>1242747096>* BAD Invalid tag
>1242747096>* BAD Invalid tag
>1242747096>* BAD Invalid tag
>1242747096>* BAD Invalid tag
>1242747096>* BAD Invalid tag
>1242747096>* BAD Invalid tag
>1242747096>* BYE Error decompressing data

So anyway.  I've rewritten the Cyrus end.  If nothing else, it will give us more logging information.  Can you try again?

(I've also created a new patch that uses Z_SYNC_FLUSH rather than Z_FULL_FLUSH and adds an additional loop just in case decompression fails to create an output character)
Comment 38 Bron Gondwana 2009-05-21 07:12:37 PDT
Created attachment 378856 [details] [diff] [review]
use Z_SYNC_FLUSH instead of Z_FULL_FLUSH

This is based on your last patch with the flush flag changed to Z_SYNC_FLUSH and a "do { } while (!*aResult);" loop on the read command so we never return zero bytes while there is more input.

It would be great if you can test with the older patch first though - just to try not to change too many things at once.  It's really annoying I haven't been able to recreate the issue!

again: changeset:   2672:d2af5fef51d9
Comment 39 David :Bienvenu 2009-05-21 08:06:38 PDT
I tried again w/ the older patch - same problem. 

When mucking with the test message, I duplicated a couple paragraphs and the issue still happened. Oddly, when I forwarded it as an attachment to myself, the problem didn't happen when I tried to append the forwarded message.
Comment 40 David :Bienvenu 2009-05-21 09:27:47 PDT
good news, it seems to be working with the latest patch! Thx! I'll proceed with the review.
Comment 41 David :Bienvenu 2009-05-21 09:49:19 PDT
Comment on attachment 378856 [details] [diff] [review]
use Z_SYNC_FLUSH instead of Z_FULL_FLUSH

ok, now that this is working, let the nit picking begin :-)

+  nsresult rv;
+  // wrap the streams in compression layers that compress or decompress
+  // all traffic.
+  nsCOMPtr <nsMsgCompressIStream> new_in = new nsMsgCompressIStream();
+  rv = new_in->InitInputStream(m_inputStream);

should move decl of nsresult to where its first used. Also, need to check for new_in null, i.e., new failing (we don't turn on exceptions)

+nsMsgCompressOStream::Write(const char *buf, PRUint32 count, PRUint32 *result)
+{
+  if (!m_oStream)
+    return NS_BASE_STREAM_CLOSED;
+
+  int rv = NS_OK;

should be nsresult rv instead of int.

instead of +      if (rv != NS_OK)

you want if (NS_FAILED(rv)) 

and conversely, NS_SUCCEEDED(rv) instead of rv == NS_OK, if you have any of those (I don't see any).

Can you add a comment about these magic numbers (-15 and 8) ? :

+  deflateInit2(m_zstr, Z_DEFAULT_COMPRESSION, Z_DEFLATED, -15, 8, Z_DEFAULT_STRATEGY);

Also, does deflateInit2 always succeed? Similarly for inflateInit2 and the -15 there.


I don't think we want a status update for the gui, so I'd just remove the comment.
+    SetFlag(IMAP_ISSUED_COMPRESS_REQUEST);
+    // XXX: status update for the GUI?

This is very cool that this is working!
Comment 42 Bron Gondwana 2009-05-22 19:06:33 PDT
Sorry not to respond to this yesterday.  I did some testing which showed that the Available() behaviour was distinctly sub-optimal in its interactions with ReadNextLine().  Basically, ReadNextLine calls Available, and reads the smaller of either that amount or the buffer size.

Unfortunately, this means that you get a read of 4096 bytes first, which leaves a little bit still in the z_stream input buffer.  There might be, say "3 characters" left there.  So you read 3 characters.  Then there is still 1 character left, so you read one character.  This might repeat a few times before you finally empty the zlib output and it can read a big block again.

Changing Available() to always return zero works, but reading the code it looks like it probably breaks idle - so I don't want to do that either.

So my plan is to find a few minutes peace some time over the weekend (lock the kids outside or something ;) ) and rewrite nsMsgCompressIStream to use a second buffer and decompress during the Available() call so that it can return an _accurate_ number of characters that are available.  It will still clear each stage before fetching more data, but at least it should stop single byte reads!

Oh - and I've tidied up BeginCompressing a fair bit - by making it return an nsresult and closing the stream on error return rather than having a bunch of complex conditionals within it.  I'll send a patch that fixes these things soon.

(and I found constants and docs for the magic numbers too)
Comment 43 Bron Gondwana 2009-05-22 22:15:12 PDT
Created attachment 379312 [details] [diff] [review]
now with better "Available()" values and cleaner error checking

Here we go :)

BeginCompressing() now returns a value for easier error checking, Available() and Read() are pretty much rewritten so that ReadNextLine doesn't go into single character read loops.  There are more comments, and I've addressed everything you mentioned in the nitpicking code review...
Comment 44 David :Bienvenu 2009-05-26 16:01:24 PDT
thx for the patch - when new fails, I think we want to return NS_ERROR_OUT_OF_MEMORY.

Everything seems to work fine, except that I'm not seeing IDLE work with compression turned on. Is there anything I can do to help diagnose this? It works fine w/o compression.
Comment 45 Bron Gondwana 2009-05-28 18:47:33 PDT
Created attachment 380323 [details] [diff] [review]
use nsIAsyncInputStream as the input base class

Sorry about the delay getting back to you on this one.

It looks like the problem was that my class didn't implement the nsIAsyncInputStream interface, so it just didn't listen for idle responses.

I've switched to using that as the parent class, and added an implementation of AsyncWait that just waits on the underlying stream.  Also had to rework the "Close" function to handle CloseWithStatus and pass that down to the underlying stream as well (if present)

Finally, I changed the "new" return codes to be out of memory errors :)

Enjoy.
Comment 46 David :Bienvenu 2009-05-29 08:10:37 PDT
Thx for the patch - the first time I ran with it, everything was fine, including idle, and append of the problematic message. But the second time I ran into this:

2009-04-29 15:02:29.172000 UTC - 5136[69dd390]: ReadNextLine [stream=69de560 nb=21 needmore=0]
2009-04-29 15:02:29.172000 UTC - 5136[69dd390]: 699c310:imap.fastmail.fm:A:CreateNewLineFromSocket: 6 OK DEFLATE active

2009-04-29 15:02:29.175000 UTC - 5136[69dd390]: 699c310:imap.fastmail.fm:A:SendData: 7 lsub "" "INBOX.*"

2009-04-29 15:02:29.175000 UTC - 5136[69dd390]: ReadNextLine [stream=0 nb=0 needmore=1]
2009-04-29 15:02:29.175000 UTC - 5136[69dd390]: ReadNextLine [stream=0 nb=0 needmore=1]

and those lines are repeated infinitely, so we're spinning trying to get more data.
Comment 47 David :Bienvenu 2009-05-29 08:21:29 PDT
Created attachment 380441 [details]
use nsRefPtr, add some QI's, and use NS_ENSURE_SUCCESS(rv, rv)

I've made some tweaks to the previous patch to get rid of some assertions, and fix some usage. I've also used NS_ENSURE_SUCCESS(rv, rv) in cases where we don't expect any errors (that adds a warning to the debug output) - if I'm wrong about not expecting errors in those case, they should switch back to 
if (NS_FAILED(rv))
  return rv;

Let me know if I can help debug the spinning waiting for more data...
Comment 48 David :Bienvenu 2009-05-29 08:24:55 PDT
hmm, I guess it's possible my changes caused the spin, though they seemed pretty straightforward. I'll try your patch again...
Comment 49 David :Bienvenu 2009-05-29 09:01:48 PDT
Created attachment 380446 [details] [diff] [review]
and add nsIInputStream to the interfaces that icompress stream implements

ok, the endless spinning was because the QI for the input stream was failing, so I fixed that by using NS_IMPL_THREADSAFE_ISUPPORTS2 and adding nsIInputStream.

I also added checks for the QI failing.

And I forgot to mention that BeginCompressing() wasn't returning rv at the end, so I added that in the prev patch.
Comment 50 neil@parkwaycc.co.uk 2009-05-29 16:12:14 PDT
Comment on attachment 380446 [details] [diff] [review]
and add nsIInputStream to the interfaces that icompress stream implements

Doesn't this need a pref("mail.server.default.use_compress_deflate, true); ?

Note: some nits not repeated for brevity.

> 	$(MOZ_UNICHARUTIL_LIBS) \
> 	$(MOZ_COMPONENT_LIBS) \
>+        $(ZLIB_LIBS) \
> 	$(NULL)
Nit: space/tab inconsistency

>+nsMsgCompressIStream::nsMsgCompressIStream() 
>+{
>+  m_zstr = nsnull;
>+  m_zbuf = nsnull;
>+  m_databuf = nsnull;
>+  m_dataptr = nsnull;
>+  m_dataleft = 0;
>+}
Nit: prefer C++ initialiser e.g.
nsMsgCompressIStream::nsMsgCompressIStream()
  : m_dataleft(0)

>+  m_iStream = rawStream;
Nit: Slightly nicer to do this last, in case of intermediate failure.

>+  // set up zlib object
>+  m_zstr = (z_stream *)PR_MALLOC(sizeof(z_stream));
Any particular reason you can't make this a member, or at least use new?

>+  m_zbuf = (char *)PR_MALLOC(sizeof(char) * BUFFER_SIZE);
Personally I don't see the point of sizeof(char) *

>+  if (!m_iStream) 
>+  {
>+    *aResult = 0;
>+    return NS_OK;
Should this return NS_BASE_STREAM_CLOSED too?

>+      nsresult rv = m_iStream->Read((char *)m_zbuf, (PRUint32)BUFFER_SIZE, &bytesRead);
m_zbuf is already a char *

>+      m_zstr->next_in = (Bytef*)m_zbuf;
Nit: style elsewhere seems to be (Bytef *) or static_cast<Bytef *>()

>+#include "msgCore.h"
>+#include "nsIAsyncInputStream.h"
>+#include "nsIInputStream.h"
>+#include "nsStreamUtils.h"
>+#include "nsCOMPtr.h"
>+#include "prio.h"
>+#include "prmem.h"
>+#include "zlib.h"
Do you need all of these here? Prefer to put them in the .cpp if possible.

>+  NS_IMETHOD Available(PRUint32 *_retval); 
>+  NS_IMETHOD Read(char * aBuf, PRUint32 aCount, PRUint32 *_retval); 
>+  NS_IMETHOD ReadSegments(nsWriteSegmentFun aWriter, void * aClosure, PRUint32 aCount, PRUint32 *_retval);
>+  NS_IMETHOD AsyncWait(nsIInputStreamCallback *callback, PRUint32 flags, PRUint32 amount, nsIEventTarget *target);
>+  NS_IMETHOD IsNonBlocking(PRBool*);
>+  NS_IMETHOD Close();
>+  NS_IMETHOD CloseWithStatus(nsresult reason);
NS_DECL_NSIINPUTSTREAM
NS_DECL_NSIASYNCINPUTSTREAM

>+  z_stream *m_zstr;
>+  char *m_zbuf;
>+  char *m_databuf;
Consider using nsAutoPtr/nsAutoArrayPtr

>+  } while (!m_zstr->avail_out);
This looks wrong. My guess is while (m_zstr->avail_in);

>+  NS_IMETHOD Write(const char * aBuf, PRUint32 aCount, PRUint32 *_retval); 
>+  NS_IMETHOD WriteFrom(nsIInputStream *inStr, PRUint32 count, PRUint32 *_retval);
>+  NS_IMETHOD WriteSegments(nsReadSegmentFun reader, void * closure, PRUint32 count, PRUint32 *_retval);
>+  NS_IMETHOD Flush(void);
>+  NS_IMETHOD IsNonBlocking(PRBool *);
>+  NS_IMETHOD Close();
NS_DECL_NSIOUTPUTSTREAM

>+  m_inputStream = do_QueryInterface(new_in, &rv);
Don't need to QI here. I don't know how to avoid the addref/release though.
Comment 51 David :Bienvenu 2009-05-29 16:20:45 PDT
> Doesn't this need a pref("mail.server.default.use_compress_deflate, true); ?
my bad, forgot to include that in the patch

>+  m_inputStream = do_QueryInterface(new_in, &rv);
>Don't need to QI here. I don't know how to avoid the addref/release though.

If I don't do that, I get an assertion about query interface needed. Which goes away when I add the QI.

thx for looking at this! I'll try to address the rest of the comments this weekend and put up a new patch...

Bron, if you could look at this comment and get back to me:

>+  } while (!m_zstr->avail_out);
> This looks wrong. My guess is while (m_zstr->avail_in);
Comment 52 Bron Gondwana 2009-05-30 06:26:31 PDT
(In reply to comment #50)
> Note: some nits not repeated for brevity.

This is pretty much what I was looking for :)  Advice on how to do all this stuff.  My C++ experience is pretty limited, so I wrote in a mostly C style!

> > 	$(MOZ_UNICHARUTIL_LIBS) \
> > 	$(MOZ_COMPONENT_LIBS) \
> >+        $(ZLIB_LIBS) \
> > 	$(NULL)
> Nit: space/tab inconsistency

trivially fixed.

> >+nsMsgCompressIStream::nsMsgCompressIStream() 
> >+{
> >+  m_zstr = nsnull;
> >+  m_zbuf = nsnull;
> >+  m_databuf = nsnull;
> >+  m_dataptr = nsnull;
> >+  m_dataleft = 0;
> >+}
> Nit: prefer C++ initialiser e.g.
> nsMsgCompressIStream::nsMsgCompressIStream()
>   : m_dataleft(0)

Yeah, done.

> >+  m_iStream = rawStream;
> Nit: Slightly nicer to do this last, in case of intermediate failure.

Yeah, I guess.  We always drop out later anyway if we get an error, but I accept the logic.  Fixed.

> >+  // set up zlib object
> >+  m_zstr = (z_stream *)PR_MALLOC(sizeof(z_stream));
> Any particular reason you can't make this a member, or at least use new?

No, there isn't.  I've made it a member and called it m_zstream, because "str" is a dodgy multi-meaning acronym.

> >+  m_zbuf = (char *)PR_MALLOC(sizeof(char) * BUFFER_SIZE);
> Personally I don't see the point of sizeof(char) *

It's ok, it became new.

> >+  if (!m_iStream) 
> >+  {
> >+    *aResult = 0;
> >+    return NS_OK;
> Should this return NS_BASE_STREAM_CLOSED too?

No, I don't think so, looking at similar modules in the util directory (I used nsMsgFileStream as a template here)

> >+      nsresult rv = m_iStream->Read((char *)m_zbuf, (PRUint32)BUFFER_SIZE, &bytesRead);
> m_zbuf is already a char *

Not any more :)  But yeah, removed it.

> >+      m_zstr->next_in = (Bytef*)m_zbuf;
> Nit: style elsewhere seems to be (Bytef *) or static_cast<Bytef *>()

(Bytef *) is fine.  Changed it.

> >+#include "msgCore.h"
> >+#include "nsIAsyncInputStream.h"
> >+#include "nsIInputStream.h"
> >+#include "nsStreamUtils.h"
> >+#include "nsCOMPtr.h"
> >+#include "prio.h"
> >+#include "prmem.h"
> >+#include "zlib.h"
> Do you need all of these here? Prefer to put them in the .cpp if possible.

I've moved the couple I can.  Still need everything that defines the types of members.

> >+  NS_IMETHOD Available(PRUint32 *_retval); 
> >+  NS_IMETHOD Read(char * aBuf, PRUint32 aCount, PRUint32 *_retval); 
> >+  NS_IMETHOD ReadSegments(nsWriteSegmentFun aWriter, void * aClosure, PRUint32 aCount, PRUint32 *_retval);
> >+  NS_IMETHOD AsyncWait(nsIInputStreamCallback *callback, PRUint32 flags, PRUint32 amount, nsIEventTarget *target);
> >+  NS_IMETHOD IsNonBlocking(PRBool*);
> >+  NS_IMETHOD Close();
> >+  NS_IMETHOD CloseWithStatus(nsresult reason);
> NS_DECL_NSIINPUTSTREAM
> NS_DECL_NSIASYNCINPUTSTREAM

Yeah, that's nice.  Done.

> >+  z_stream *m_zstr;
> >+  char *m_zbuf;
> >+  char *m_databuf;
> Consider using nsAutoPtr/nsAutoArrayPtr

I read up on nsAutoPtr.  Looking fine.

> >+  } while (!m_zstr->avail_out);
> This looks wrong. My guess is while (m_zstr->avail_in);

No, it's correct.  Since both of you have queried it, I'll put a comment explaining what's going on in the code!

Just to explain here: you give the z_stream a fixed sized buffer to copy output into and a count of input bytes.  It reads from the input and updates its internal state, then pushes any bytes it can to the output buffer.  If you have filled the entire output buffer, then you don't know if there are more potential bytes in the internal state that haven't been able to flush yet.

So - if zstream->avail_out is zero, then you need to give it some more space and repeat the call to see if it produces any more bytes - regardless of the remaining input bytes (avail_in).  It may have consumed all the input without having space to produce all the output.

> >+  m_inputStream = do_QueryInterface(new_in, &rv);
> Don't need to QI here. I don't know how to avoid the addref/release though.

Yeah, neither did I.  I messed around for ages with this and tried a few different hacks, but nothing worked.  Is this going to break anything?  Is it actually wrong, or just inefficient?  For something called once per imap connection, inefficient doesn't bother me much.

I'll give you another patch in a second once I've added the comment about how ->avail_out works and checked that it works for me in basic tests.
Comment 53 Bron Gondwana 2009-05-30 06:33:25 PDT
Well, $expletive.  I see that I have broken IStream.  What a pain.  I could make the buffer resizable, or I could bite the bullet and add a retry flag.  Ouch.  I'll give it a hack.
Comment 54 Bron Gondwana 2009-05-30 07:30:14 PDT
(In reply to comment #51)
> > Doesn't this need a pref("mail.server.default.use_compress_deflate, true); ?
> my bad, forgot to include that in the patch

You appear to have committed it to mercurial actually.

changeset:   2672:d2af5fef51d9

> >+  m_inputStream = do_QueryInterface(new_in, &rv);
> >Don't need to QI here. I don't know how to avoid the addref/release though.
> 
> If I don't do that, I get an assertion about query interface needed. Which goes
> away when I add the QI.

Yeah, I got stuck on those too.  Hence removing the IInputStream interface, but
that broke things too obviously.

> thx for looking at this! I'll try to address the rest of the comments this
> weekend and put up a new patch...

I've already done it :)  I have to learn somehow.

Also, I _think_ I found your problem.  I wasn't handling the CompressIStream case of full buffer properly per the docs.  I re-read them, and tidied up the code considerably.  I've added a flag that can persist between Read calls that tracks if we expect to have to fetch more from the z_stream, and we'll do that before trying another read.

> Bron, if you could look at this comment and get back to me:
> 
> >+  } while (!m_zstr->avail_out);
> > This looks wrong. My guess is while (m_zstr->avail_in);

Yeah, I answered that too.  The code is right, but I've added a comment to clarify and point to the docs for further information.

Bron.
Comment 55 Bron Gondwana 2009-05-30 07:50:23 PDT
Created attachment 380615 [details] [diff] [review]
code review changes plus be _really_ strict about zlib logic and error checking

Ok, here it is :)

I've also updated "Available()" to continue fetching from the z_stream buffer if there's more data in there, so the counts stay accurate.  I really think that's everything!
Comment 56 neil@parkwaycc.co.uk 2009-05-30 12:36:03 PDT
(In reply to comment #51)
> >+  m_inputStream = do_QueryInterface(new_in, &rv);
> >Don't need to QI here. I don't know how to avoid the addref/release though.
> If I don't do that, I get an assertion about query interface needed. Which goes
> away when I add the QI.
That's... weird. There should only be one way to do the cast!
Comment 57 Bron Gondwana 2009-05-31 22:57:26 PDT
Created attachment 380778 [details] [diff] [review]
Check for Z_BUF_ERROR condition and return code of DoInflation()

It appears that if you fill the output buffer exactly you can wind up getting a Z_BUF_ERROR return code (basically, means no progress was possible).  This is not actually an error.

Further, because I was ignoring the return code for DoInflation(), this caused an infinite loop.

This patch fixes BOTH those issues.
Comment 58 David :Bienvenu 2009-06-04 13:50:10 PDT
Created attachment 381604 [details] [diff] [review]
patch without the QI's in ::BeginCompress

this is the same as the prev patch, but w/o the QI's in BeginCompress.  I don't see assertions when I run with this.
Comment 59 neil@parkwaycc.co.uk 2009-06-04 13:56:58 PDT
Comment on attachment 381604 [details] [diff] [review]
patch without the QI's in ::BeginCompress

>+  m_zbuf(nsnull),
>+  m_databuf(nsnull),
Nit: these auto-init to nsnull anyway, but it doesn't hurt, so leave them in if
you think it's clearer.

>+  nsAutoPtr<char> m_zbuf;
>+  nsAutoPtr<char> m_databuf;
These need to be nsAutoArrayPtr (sorry if I confused you earlier but I couldn't
remember which was right at the time).

Both these points apply similarly for the OStream of course.
Comment 60 David :Bienvenu 2009-06-05 08:11:09 PDT
fix checked in, with comments addressed - thx very much for the patches, Bron!

Note You need to log in before you can comment on or make changes to this bug.