Closed
Bug 401673
Opened 17 years ago
Closed 15 years ago
Add support for IMAP COMPRESS Extension - rfc4978
Categories
(MailNews Core :: Networking: IMAP, enhancement)
MailNews Core
Networking: IMAP
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b3
People
(Reporter: shopik, Assigned: brong)
References
()
Details
Attachments
(3 files, 15 obsolete files)
31.62 KB,
patch
|
Details | Diff | Splinter Review | |
25.77 KB,
patch
|
Details | Diff | Splinter Review | |
22.88 KB,
patch
|
neil
:
superreview+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
Confirming RFE.
Assignee: nobody → bienvenu
Status: UNCONFIRMED → NEW
Component: General → Networking: IMAP
Ever confirmed: true
OS: Windows XP → All
Product: Thunderbird → Core
QA Contact: general → networking.imap
Hardware: PC → All
Reporter | ||
Updated•16 years ago
|
Version: unspecified → Trunk
Updated•15 years ago
|
Product: Core → MailNews Core
Assignee | ||
Comment 2•15 years ago
|
||
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!
Assignee | ||
Comment 3•15 years ago
|
||
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.
Assignee | ||
Comment 4•15 years ago
|
||
Attachment #377061 -
Attachment is obsolete: true
Assignee | ||
Comment 5•15 years ago
|
||
Attachment #377065 -
Attachment is obsolete: true
Comment 6•15 years ago
|
||
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
Assignee: bienvenu → brong
Status: NEW → ASSIGNED
Updated•15 years ago
|
Attachment #377073 -
Flags: review?(bienvenu)
Assignee | ||
Comment 7•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
(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•15 years ago
|
||
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.
Assignee | ||
Comment 11•15 years ago
|
||
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.
Attachment #377073 -
Attachment is obsolete: true
Attachment #377073 -
Flags: review?(bienvenu)
Comment 12•15 years ago
|
||
(In reply to comment #11) > Other than that, it's the same. You need to request review ...
Assignee | ||
Updated•15 years ago
|
Attachment #377346 -
Flags: review?(bienvenu)
Assignee | ||
Comment 13•15 years ago
|
||
Ahh - thanks. Still learning my way around...
Comment 14•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
if you add ac_add_options --enable-debug to .mozconfig, you'll get a dynamic as opposed to static build.
Comment 18•15 years ago
|
||
my release mac build doesn't work either - the zlib makefile stuff seems even more arcane than our other makefile stuff.
Comment 19•15 years ago
|
||
(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•15 years ago
|
||
(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•15 years ago
|
||
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...
Assignee | ||
Comment 22•15 years ago
|
||
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.
Assignee | ||
Comment 23•15 years ago
|
||
Sorry - I should have redacted that messageid!
Comment 24•15 years ago
|
||
(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•15 years ago
|
||
Bron, any more idea what's going on with the server i/o error?
Assignee | ||
Comment 26•15 years ago
|
||
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...
Assignee | ||
Comment 27•15 years ago
|
||
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?
Assignee | ||
Comment 28•15 years ago
|
||
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.
Attachment #377346 -
Attachment is obsolete: true
Attachment #378266 -
Flags: review?(bienvenu)
Attachment #377346 -
Flags: review?(bienvenu)
Comment 29•15 years ago
|
||
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.
Attachment #377461 -
Attachment is obsolete: true
Attachment #377768 -
Attachment is obsolete: true
Attachment #378266 -
Attachment is obsolete: true
Attachment #378266 -
Flags: review?(bienvenu)
Comment 30•15 years ago
|
||
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).
Assignee | ||
Comment 31•15 years ago
|
||
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.
Assignee | ||
Comment 32•15 years ago
|
||
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.
Attachment #378534 -
Flags: review?(bienvenu)
Comment 33•15 years ago
|
||
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•15 years ago
|
||
I accidentally landed the mailnews.js change that added the default server compress pref - it won't break anything, though.
Comment 35•15 years ago
|
||
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•15 years ago
|
||
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...
Assignee | ||
Comment 37•15 years ago
|
||
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)
Assignee | ||
Comment 38•15 years ago
|
||
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
Attachment #378534 -
Attachment is obsolete: true
Attachment #378856 -
Flags: review?(bienvenu)
Attachment #378534 -
Flags: review?(bienvenu)
Comment 39•15 years ago
|
||
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•15 years ago
|
||
good news, it seems to be working with the latest patch! Thx! I'll proceed with the review.
Comment 41•15 years ago
|
||
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!
Assignee | ||
Comment 42•15 years ago
|
||
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)
Assignee | ||
Comment 43•15 years ago
|
||
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...
Attachment #378856 -
Attachment is obsolete: true
Attachment #379312 -
Flags: review?(bienvenu)
Attachment #378856 -
Flags: review?(bienvenu)
Comment 44•15 years ago
|
||
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.
Assignee | ||
Comment 45•15 years ago
|
||
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.
Attachment #379312 -
Attachment is obsolete: true
Attachment #380323 -
Flags: review?(bienvenu)
Attachment #379312 -
Flags: review?(bienvenu)
Comment 46•15 years ago
|
||
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•15 years ago
|
||
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...
Attachment #380323 -
Attachment is obsolete: true
Attachment #380323 -
Flags: review?(bienvenu)
Comment 48•15 years ago
|
||
hmm, I guess it's possible my changes caused the spin, though they seemed pretty straightforward. I'll try your patch again...
Comment 49•15 years ago
|
||
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.
Attachment #380441 -
Attachment is obsolete: true
Attachment #380446 -
Flags: superreview?(neil)
Attachment #380446 -
Flags: review+
Comment 50•15 years ago
|
||
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•15 years ago
|
||
> 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);
Assignee | ||
Comment 52•15 years ago
|
||
(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.
Assignee | ||
Comment 53•15 years ago
|
||
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.
Assignee | ||
Comment 54•15 years ago
|
||
(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.
Assignee | ||
Comment 55•15 years ago
|
||
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!
Attachment #380615 -
Flags: review?(bienvenu)
Comment 56•15 years ago
|
||
(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!
Assignee | ||
Comment 57•15 years ago
|
||
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.
Attachment #380615 -
Attachment is obsolete: true
Attachment #380778 -
Flags: review?(bienvenu)
Attachment #380615 -
Flags: review?(bienvenu)
Updated•15 years ago
|
Attachment #380778 -
Flags: superreview?(neil)
Attachment #380778 -
Flags: review?(bienvenu)
Attachment #380778 -
Flags: review+
Updated•15 years ago
|
Attachment #380446 -
Attachment is obsolete: true
Attachment #380446 -
Flags: superreview?(neil)
Comment 58•15 years ago
|
||
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.
Attachment #380778 -
Attachment is obsolete: true
Attachment #381604 -
Flags: superreview?(neil)
Attachment #380778 -
Flags: superreview?(neil)
Updated•15 years ago
|
Attachment #381604 -
Flags: superreview?(neil) → superreview+
Comment 59•15 years ago
|
||
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•15 years ago
|
||
fix checked in, with comments addressed - thx very much for the patches, Bron!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
You need to log in
before you can comment on or make changes to this bug.
Description
•