Closed Bug 401293 Opened 12 years ago Closed 11 years ago

Do not request IMAP capability command if server announce it in greetings

Categories

(MailNews Core :: Networking: IMAP, enhancement)

1.8 Branch
enhancement
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: shopik, Assigned: dwiggins)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Version: unspecified → 2.0
Assignee: nobody → bienvenu
Component: General → Networking: IMAP
Product: Thunderbird → Core
QA Contact: general → networking.imap
Version: 2.0 → 1.8 Branch
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 
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
OS: Windows XP → All
Hardware: PC → All
Summary: TB must not request IMAP capability command if server announce it in greetings → Do not request IMAP capability command if server announce it in greetings
Dale, this is an IMAP bug that we'd like to fix...
Sure, I'll have a look.
Assignee: bienvenu → dwiggins
Successfully parses CAPABILITY in greeting, doesn't ask for CAPABILITY later in this case.
Attachment #332735 - Flags: superreview?(bienvenu)
Attachment #332735 - Flags: review?(mnyromyr)
No substantive changes.
Attachment #332735 - Attachment is obsolete: true
Attachment #335917 - Flags: superreview?(bienvenu)
Attachment #335917 - Flags: review?(mnyromyr)
Attachment #332735 - Flags: superreview?(bienvenu)
Attachment #332735 - Flags: review?(mnyromyr)
No substantive changes.
Attachment #335917 - Attachment is obsolete: true
Attachment #336852 - Flags: superreview?(bienvenu)
Attachment #336852 - Flags: review?(mnyromyr)
Attachment #335917 - Flags: superreview?(bienvenu)
Attachment #335917 - Flags: review?(mnyromyr)
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.
Attachment #336852 - Flags: review?(mnyromyr) → review-
This should hopefully address all of the concerns in the previous comment.
Attachment #336852 - Attachment is obsolete: true
Attachment #340200 - Flags: superreview?(bienvenu)
Attachment #340200 - Flags: review?(mnyromyr)
Attachment #336852 - Flags: superreview?(bienvenu)
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.
Attachment #340200 - Attachment description: Patch that addresses Karstern's feedback in comment #22. → Patch that addresses Karsten's feedback in comment #22.
Attachment #340200 - Flags: review?(mnyromyr) → review+
> > +++ 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...
Status: NEW → ASSIGNED
The only changes in this patch are the changes that Karsten suggested.
Attachment #340200 - Attachment is obsolete: true
Attachment #347762 - Flags: superreview?(bienvenu)
Attachment #347762 - Flags: review?(mnyromyr)
Attachment #340200 - Flags: superreview?(bienvenu)
Attachment #347762 - Flags: review?(mnyromyr) → review+
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.
Attachment #347762 - Attachment is obsolete: true
Attachment #348071 - Flags: superreview?(bienvenu)
Attachment #347762 - Flags: superreview?(bienvenu)
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.
Attachment #348071 - Attachment is obsolete: true
Attachment #348071 - Flags: superreview?(bienvenu)
fix checked in, with a minor tweak to avoid an extra long line...thx, Dale.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Thanks guys, works flawlessly.
Status: RESOLVED → VERIFIED
Product: Core → MailNews Core
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.
Depends on: 470650
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.
You need to log in before you can comment on or make changes to this bug.