issueCommandOnMsgs causes syntax error when server response is FETCH

RESOLVED FIXED in Thunderbird 17.0

Status

MailNews Core
Networking: IMAP
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dlech, Assigned: dlech)

Tracking

Trunk
Thunderbird 17.0
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird16 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 646672 [details] [diff] [review]
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.
Attachment #646672 - Flags: review?(mozilla)
Attachment #646672 - Flags: approval-comm-beta?
(Assignee)

Comment 2

5 years ago
Created attachment 646673 [details] [diff] [review]
xpcshell test
Attachment #646673 - Flags: review?(mozilla)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: issueCommandOnMsgs → issueCommandOnMsgs causes syntax error when server response is FETCH
(Assignee)

Updated

5 years ago
Attachment #646672 - Flags: review?(mbanner)
(Assignee)

Updated

5 years ago
Attachment #646673 - Flags: review?(mbanner)

Comment 4

5 years ago
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));
+          }
Attachment #646672 - Flags: review?(mozilla) → review+

Comment 5

5 years ago
Comment on attachment 646673 [details] [diff] [review]
xpcshell test

thx, looks good.
Attachment #646673 - Flags: review?(mozilla) → review+
(Assignee)

Updated

5 years ago
Attachment #646673 - Flags: review?(mbanner)
(Assignee)

Updated

5 years ago
Attachment #646672 - Flags: review?(mbanner)
(Assignee)

Comment 6

5 years ago
Created attachment 646935 [details] [diff] [review]
Revised patch to fix bug

nits nitted (hope I got the indent right)
Attachment #646672 - Attachment is obsolete: true
Attachment #646672 - Flags: approval-comm-beta?
Attachment #646935 - Flags: review?(mozilla)
(Assignee)

Comment 7

5 years ago
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.
Attachment #646935 - Flags: approval-comm-beta?

Comment 8

5 years ago
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

5 years ago
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...
Attachment #646935 - Flags: review?(mozilla) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 647010 [details] [diff] [review]
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.
Attachment #646935 - Attachment is obsolete: true
Attachment #646935 - Flags: approval-comm-beta?
Attachment #647010 - Flags: approval-comm-aurora?
(Assignee)

Comment 11

5 years ago
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.
Attachment #646673 - Flags: approval-comm-aurora?
(Assignee)

Comment 12

5 years ago
(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

5 years ago
add the checkin-needed keyword should get it checked in.
Keywords: checkin-needed
Status: UNCONFIRMED → NEW
Ever confirmed: true
https://hg.mozilla.org/comm-central/rev/111454f8e9a0
https://hg.mozilla.org/comm-central/rev/b3a09c3b06c9
Assignee: nobody → david
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
Attachment #647010 - Flags: approval-comm-aurora? → approval-comm-aurora+
Attachment #646673 - Flags: approval-comm-aurora? → approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/2da3d597b702
https://hg.mozilla.org/releases/comm-aurora/rev/db0cc7630dbc
status-thunderbird16: --- → fixed
You need to log in before you can comment on or make changes to this bug.