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: version 2.0.0.6 (20070728) Many IMAP servers can announce their capability in greetings. TB must not request capability command if server already announce it in greetings. At least there should be option force it on or off. Reproducible: Always Steps to Reproduce: 1. Start your favorite sniffer to cap packets 2. Run TB 3. Analyze packets in sniffer 4. Look for capability request even if server show already capability before.

does the imap rfc really say servers can return the capabilities in the greeting? That's news to me.

Yes its part of RFC3501 (7.1.) which clearly says: "Status responses MAY include an OPTIONAL "response code". A response code consists of data inside square brackets in the form of an atom, possibly followed by a space and arguments."

Though not the same bug, this is closely related to one I've just filed https://bugzilla.mozilla.org/show_bug.cgi?id=404323 Where Thunderbird isn't asking for changed capabilities after not getting these responses in the login OK result. Cheers, Duncan

still reproducible on trunk 3.0a1pre (2008040204) Windows XP sp2

Making new since its really obvious

is bug 404323 really related to the extent that anything done here in this bug might impact 404323 ?

bug 404323 violates RFC2060 clause 6.1.1.

My understanding from section 7.1 is even CAPABILITY is appeared in the initial OK or PREAUTH response, there is no harm to ask for it explicitly later, although it is unnecessary. Nikolay thoughts?

That's why I marked this as RFE and NOT a bug. This is usually save few packets roundtrip, still important when you are on slow connection or/and high latency.

Gotcha, makes sense.

(In reply to comment #6) > is bug 404323 really related to the extent that anything done here in this bug > might impact 404323 ? > Not at all, I thought at the time that they'd probably be easily addressed by the same person at the same time. Sorry.

(In reply to comment #7) > bug 404323 violates RFC2060 clause 6.1.1. > RFC2060 is obsoleted by 3501 which doesn't have this restriction.

(In reply to comment #11) > (In reply to comment #6) > > is bug 404323 really related to the extent that anything done here in this bug > > might impact 404323 ? > > > > Not at all, I thought at the time that they'd probably be easily addressed by > the same person at the same time. Sorry. > Actually, on second thoughts, they're both related to whether or not youhould/shouldn't re request CAPABILITIES from a server and when, so could be closely related (in the source) but I haven't looked at the code and don't really know.

This bug contradicts bug 404323.

Definitely its contradicts and nobody notice this :) I see here only one solution create new boolean value to control behavior of TB. Like mail.imap.check_capability_in_greeting and make it false by default, so this means by default it will act like described in bug 404323 because its more important. And if you are sure you don't have different CAPABILITIES you can turn this is by setting TRUE.

(In reply to comment #14) > This bug contradicts bug 404323. > I don't think it does, If the server does advertise capabilities in a response (login or greetings) then the client shouldn't rerequest capabilities (401293). In 404323 I'm saying if the server doesn't advertise capabilities in the response, then the client should rerequest capabilities.

Dale, this is an IMAP bug that we'd like to fix...

Sure, I'll have a look.

Successfully parses CAPABILITY in greeting, doesn't ask for CAPABILITY later in this case.

No substantive changes.

No substantive changes.

Comment on attachment 336852 [details] [diff] [review] New version of my last patch done with Mercurial Sorry for the delay. >+++ b/mailnews/imap/src/nsImapProtocol.cpp Wed Sep 03 12:48:35 2008 -0400 >@@ -1321,6 +1321,25 @@ void nsImapProtocol::EstablishServerConn > if (!PL_strncasecmp(serverResponse, "* OK", 4)) > { > SetConnectionStatus(0); Please use -U8 for 8 lines of context next time. :) >+ nsCAutoString tmpstr(serverResponse); >+ PRInt32 startIndex = tmpstr.Find("CAPABILITY", PR_TRUE/*aIgnoreCase*/); 1. Why do you copy the string before you even know you'll need it? Only create the temp string once you found the CAPABILITY response. 2. That's dangerous, since "CAPABILITY" might e.g. appear in a [ALERT] response. Actually, in the case of a CAPABILITY response, the serverResponse has to begin *exactly* with "* OK [CAPABILITY"! 3. And please leave a space before the comment, but only just a single space between type and variable. >+ if (startIndex >= 0) >+ { >+ PRInt32 endIndex = tmpstr.FindChar(']', startIndex); Only just a single space between type and variable, please. >+ if (endIndex >= 0) >+ { >+ // Allocate the new buffer here -- it will be freed when the parser is destructed. >+ char *fakeServerResponse = (char*)PR_Malloc(PL_strlen(serverResponse)); And again. >+ // Munge the greeting into something that would pass for an IMAP server's response >+ // to a "CAPABILITY" command. Please wrap comments after column 78 (here and elsewhere in your code), if decently possible. >+ strcpy(fakeServerResponse, "* "); >+ strcat(fakeServerResponse, serverResponse + startIndex); >+ fakeServerResponse[endIndex - startIndex + 2] = '\0'; // + 2 to account for the "* " string >+ // Tell the response parser that we just issued a "CAPABILITY" and got the following back. >+ GetServerStateParser().ParseIMAPServerResponse("1 capability", PR_TRUE, fakeServerResponse); Any reason why to use a lowercase CAPABILITY here? >+++ b/mailnews/imap/src/nsImapServerResponseParser.cpp Wed Sep 03 12:51:10 2008 -0400 >@@ -170,7 +170,9 @@ void nsImapServerResponseParser::Initial > // response-data = "*" SP (resp-cond-state / resp-cond-bye / > // mailbox-data / message-data / capability-data) CRLF > // continue-req = "+" SP (resp-text / base64) CRLF >-void nsImapServerResponseParser::ParseIMAPServerResponse(const char *currentCommand, PRBool aIgnoreBadAndNOResponses) >+void nsImapServerResponseParser::ParseIMAPServerResponse(const char *currentCommand, >+ PRBool aIgnoreBadAndNOResponses, >+ char *capaInGreeting) Since you're touching this, please make it fully adhere to our naming scheme, which requires the "a" prefix for all method arguments. And please use understandable names like "aGreetingWithCapability". >+ if (capaInGreeting) >+ fCurrentLine = capaInGreeting; Who is going to free the former fCurrentLine? >+++ b/mailnews/imap/src/nsImapServerResponseParser.h Wed Sep 03 12:52:45 2008 -0400 >+ virtual void ParseIMAPServerResponse(const char *currentCommand, >+ PRBool aIgnoreBadAndNOResponses, >+ char *capaInGreeting = NULL); See above.

This should hopefully address all of the concerns in the previous comment.

Comment on attachment 340200 [details] [diff] [review] Patch that addresses Karsten's feedback in comment #22. > +++ mailnews/imap/src/nsImapProtocol.cpp Wed Sep 24 16:09:39 2008 > if (!PL_strncasecmp(serverResponse, "* OK", 4)) While this hard-coded length is probably acceptable in situations where string and length are used only once, it's usually a bad idea, and especially so in your code below. Please make the entire EstablishServerConnection method define (and undef later!) the strings and their lengths as preprocessor macros, eg. #define ESC_LENGTH(x) (sizeof(x) - 1) #define ESC_CAPABILITY_OK "* OK [" #define ESC_CAPABILITY_OK_LEN ESC_LENGTH(ESC_CAPABILITY_OK) #define ESC_CAPABILITY_GREETING (ESC_CAPABILITY_OK "CAPABILITY") #define ESC_CAPABILITY_GREETING_LEN ESC_LENGTH(ESC_CAPABILITY_GREETING) That way, changes to the actual string won't require extensive searches and thoughts like "is this hard-coded 16 a length or where does it come from?". The actual value of EST_CAPA_GREETING_LEN etc. will be computed at compile time! > + strcpy(fakeServerResponse, "* "); > + strcat(fakeServerResponse, serverResponse + 6); // +6 to skip "* OK [" > + fakeServerResponse[endIndex - 4] = '\0'; // -6 for "* OK [" and + 2 for "* " See above. > +++ mailnews/imap/src/nsImapServerResponseParser.cpp Wed Sep 24 15:03:57 2008 > + if (fCurrentLine) > + PR_Free(fCurrentLine); You can use just PR_FREEIF(fCurrentLine). > @@ -1965,17 +1974,17 @@ > - else if (!PL_strcasecmp(fNextToken, "NOMODSEQ]")) > + else if (!PL_strcasecmp(fNextToken, "NOMODSEQ")) I see no justification for this change, and it wasn't in the first patch either. Drop it, unless you have a very compelling explanation. ;-) r=me with these changes.

> > +++ mailnews/imap/src/nsImapServerResponseParser.cpp Wed Sep 24 15:03:57 2008 > > + if (fCurrentLine) > > + PR_Free(fCurrentLine); > > You can use just PR_FREEIF(fCurrentLine). Or you can just use PR_Free, since free checks for null input :-) > > > @@ -1965,17 +1974,17 @@ > > - else if (!PL_strcasecmp(fNextToken, "NOMODSEQ]")) > > + else if (!PL_strcasecmp(fNextToken, "NOMODSEQ")) > > I see no justification for this change, and it wasn't in the first > patch either. Drop it, unless you have a very compelling explanation. > ;-) > Karsten, thanks very much for catching that - it should stay NOMODSEQ] - that was a recent fix I made.

> > > + if (fCurrentLine) > > > + PR_Free(fCurrentLine); > > > > You can use just PR_FREEIF(fCurrentLine). > > Or you can just use PR_Free, since free checks for null input :-) Sure, free() does, but what would be the use of having PR_Free/PR_FREEIF then? I supposed that an abstraction layer like this would have these different functions for reason, else we could just write free() in the first place... > > > > > > @@ -1965,17 +1974,17 @@ > > > - else if (!PL_strcasecmp(fNextToken, "NOMODSEQ]")) > > > + else if (!PL_strcasecmp(fNextToken, "NOMODSEQ")) > > > > I see no justification for this change, and it wasn't in the first > > patch either. Drop it, unless you have a very compelling explanation. > > ;-) > > > Karsten, thanks very much for catching that - it should stay NOMODSEQ] - that > was a recent fix I made.

PR_FREEIF nulls the pointer after freeing the memory. That's the real difference...

The only changes in this patch are the changes that Karsten suggested.

Comment on attachment 347762 [details] [diff] [review] Patch that addresses Karsten's feedback in comment #24. > void nsImapProtocol::EstablishServerConnection() ... > if (!PL_strncasecmp(serverResponse, "* OK", 4)) ... > else if (!PL_strncasecmp(serverResponse, "* PREAUTH", 9)) I'd like these two occasions to follow the sizeof macro pattern, too, for consistency's sake - it's in the same function. >+ if (aGreetingWithCapability) >+ { >+ PR_Free(fCurrentLine); I'd still prefer PR_FREEIF, since PR_Free is just a macro, too, which might change unexpectdly, but I leave that to David's discretion. r=me with that on checkin, no need for a new patch just for this.

Minor changes for Karsten. Continuing r=mnyromyr@tprac.de.

Comment on attachment 348071 [details] [diff] [review] Patch that addresses Karsten's feedback in comment #29. thx for doing this, Dale. Can you add a comment as to why we're using an allocated string here? I think this comment isn't quite right: + // Allocate the new buffer here -- it will be freed when the parser + // is destructed. the parser will free the string, when done parsing, but the parser itself isn't destroyed. I think you can use an nsDependentCString wrapper here, instead of tmpStr: + nsCAutoString tmpstr(serverResponse); + PRInt32 endIndex = tmpstr.FindChar(']', ESC_CAPABILITY_GREETING_LEN); This would all be so much nicer if the imap parser used nsCStrings!

Continuing r=mnyromyr@tprac.de.

fix checked in, with a minor tweak to avoid an extra long line...thx, Dale.

Thanks guys, works flawlessly.

I think this is going to break stuff. Newer IMAP servers (e.g. Dovecot v1.2) send a reduced list of capabilities in the greeting CAPABILITY and then send an updated full list of capabilities in tagged OK reply for LOGIN/AUTHENTICATE. Looking at your patch it seems you only handle the untagged OK reply, but not the tagged OK reply. You really should be using a generic resp-code parser that handles them no matter where and when they happen..

Timo - yes, you're talking about bug 470650, I believe.

David what decision is made you still parse both capabilities? I'm talking about comment #14 in bug 470650

we now parse the capability response whether or not we issued a capability command. I don't think that matches any of the choices in that comment.