778246 - issueCommandOnMsgs causes syntax error when server response is FETCH

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David Lechner (:dlech)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

Trying to use the nsIMsgImapMailFolder.issueCommandOnMsgs() method in an add-on. More specifically, I am sending the command STORE # -X-GM-LABELS (...) to a Gmail server. 


Actual results:

This command replies with with a FETCH # X-GM-LABELS (...) response, which is parsed by nsImapServerResponseParser::msg_fetch(). msg_fetch() generates a syntax error because it only expects the FETCH response to be the result of a standard IMAP command or the result of a nsImapUserDefinedFetchAttribute command.


Expected results:

the server response parser should be able to handle *any* response from a custom command. In this case, msg_fetch(), which handles a FETCH response should be able to parse the response without causing a syntax error.

Comment 1

[David Lechner (:dlech)]

Attachment: Patch to fix bug

This patch adds a test case to msg_fetch() so that if the url action is nsImapUserDefinedMsgCommand then CustomCommandResult is asigned the value of the response.

Comment 2

[David Lechner (:dlech)]

Attachment: xpcshell test

Comment 3

[David Lechner (:dlech)]

I have run the test with and without the patch to verify that the test fails without the patch and succeeds with the patch. I have also run all of the tests in mailnews/imap/test to verify that I did not break anything else.

Comment 4

[David :Bienvenu]

Comment on attachment 646672 [details] [diff] [review]
Patch to fix bug

thx for the patch, some nits:

second line needs to be indented so imapAction lines up with the ( above.

+        if ((imapAction == nsIImapUrl::nsImapUserDefinedFetchAttribute && !strcmp(userDefinedFetchAttribute.get(), fNextToken)) ||
+          imapAction == nsIImapUrl::nsImapUserDefinedMsgCommand)

don't need braces here:

+          if (imapAction == nsIImapUrl::nsImapUserDefinedFetchAttribute) {
+            fServerConnection.GetCurrentUrl()->SetCustomAttributeResult(nsDependentCString(fetchResult));
+          }
+          if (imapAction == nsIImapUrl::nsImapUserDefinedMsgCommand) {
+            fServerConnection.GetCurrentUrl()->SetCustomCommandResult(nsDependentCString(fetchResult));
+          }

Comment 5

[David :Bienvenu]

Comment on attachment 646673 [details] [diff] [review]
xpcshell test

thx, looks good.

Comment 6

[David Lechner (:dlech)]

Attachment: Revised patch to fix bug

nits nitted (hope I got the indent right)

Comment 7

[David Lechner (:dlech)]

Comment on attachment 646935 [details] [diff] [review]
Revised patch to fix bug

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: No impact on current users. Will be used with gmailbuttons extension to fetch and delete gmail labels on messages.
Testing completed (on c-c, etc.): Wrote test to verify that patch works. Also ran all other imap tests to make sure nothing broke.
Risk to taking this patch (and alternatives if risky): low

This goes with a couple of simmalar patchs I have done already that made it in TB 15 (bug 735542 and bug 750012). It would be nice if this could land there as well.

Comment 8

[Kent James (:rkent)]

David, two things:

1) The convention is that once you get r+ but with nits, you can just fix the nits and ask for checkin without another review.

2) it makes no sense in this case to ask for approval-comm-beta without also asking for approval-comm-aurora.  That being said (and I have no authority in these matters) the bar is and should be rather high for approval-comm-beta, and you have not given any justification for that here - and I doubt that you can.

Current comm-central is at Gecko/Thunderbird 17, which will be the ESR version and the stable version in the "new" Thunderbird management plan, so that should be sufficient to get this patch into use.

Comment 9

[David :Bienvenu]

Comment on attachment 646935 [details] [diff] [review]
Revised patch to fix bug

almost, one more char indent (I should have been specific about which open paren, but it's the one that belongs to the clause on the same level).

I don't need to review the next patch that you attach for checkin...

Comment 10

[David Lechner (:dlech)]

Attachment: Patch to fix bug

Added one more space.

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: No impact on current users. Will be used with gmailbuttons extension to fetch and delete gmail labels on messages.
Testing completed (on c-c, etc.): Wrote test to verify that patch works. Also ran all other imap tests to make sure nothing broke.
Risk to taking this patch (and alternatives if risky): low

I am the developer of https://addons.mozilla.org/en-US/thunderbird/addon/gmailbuttons/  I would like to add some new features to my extension, but this bug needs to be fixed in order for new features to work. I would like to get this patch in comm-aurora (or even comm-beta [wishful thinking perhaps]) to speed up the process of releasing the new features in my extension.

Comment 11

[David Lechner (:dlech)]

Comment on attachment 646673 [details] [diff] [review]
xpcshell test

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Goes along with actual bug patch. See comments there.

Comment 12

[David Lechner (:dlech)]

(In reply to Kent James (:rkent) from comment #8)
> David, two things:
> 
> 1) The convention is that once you get r+ but with nits, you can just fix
> the nits and ask for checkin without another review.

Is there a proper way I should be asking for checkin? My experience so far (first 3 bugs) was that is just happened 'magically' :)

> 
> 2) it makes no sense in this case to ask for approval-comm-beta without also
> asking for approval-comm-aurora.  That being said (and I have no authority
> in these matters) the bar is and should be rather high for
> approval-comm-beta, and you have not given any justification for that here -
> and I doubt that you can.

A comment on the first bug I worked on gave me the impression that it might be possible to get a trivial bug such as this into comm-beta. If I am causing people extra work by just asking, then I shall not do it again for my own selfish little cause here.

> 
> Current comm-central is at Gecko/Thunderbird 17, which will be the ESR
> version and the stable version in the "new" Thunderbird management plan, so
> that should be sufficient to get this patch into use.

Comment 13

[David :Bienvenu]

add the checkin-needed keyword should get it checked in.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/111454f8e9a0
https://hg.mozilla.org/comm-central/rev/b3a09c3b06c9

Comment 15

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/comm-aurora/rev/2da3d597b702
https://hg.mozilla.org/releases/comm-aurora/rev/db0cc7630dbc