nsImapServerResponeParser fails while parsing custom message attribute data that is not enclosed in parentheses

RESOLVED FIXED in Thunderbird 16.0

Status

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

People

(Reporter: dlech, Assigned: dlech)

Tracking

({imap-interop})

Thunderbird 16.0
imap-interop
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird15 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 619356 [details]
10031.patch

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

I am using the nsIMsgImapMailFolder.fetchCustomMsgAttribute function in javascript to get the X-GM-MSGID attribute from a Gmail message. After fixing Bug 735542, this bug became apparent.


Actual results:

Since I am using a debug build of Thunderbird, I got an assertion error in nsIMAPGenericParser::CreateParenGroup() that the data being parsed did not begin with '(' - exact error is "we don't have a paren group!"


Expected results:

if a single value is returned by the IMAP server, the returned value does not have to be in a parentheses group. The single value should have been read without trying to parse it as a parentheses group.
(Assignee)

Updated

5 years ago
Keywords: imap-interop
(Assignee)

Comment 1

5 years ago
Comment on attachment 619356 [details]
10031.patch

There is no unit test for this particular part of the code. I have verified that the patch works manaully.
Attachment #619356 - Flags: review?(dbienvenu)

Comment 2

5 years ago
Comment on attachment 619356 [details]
10031.patch

it looks like there are tabs in your patch...

I wonder if this could be tested by just pretending that some built-in attributes are custom attributes and running a fetch custom attributes url on the built-in attributes in fake server.

Also, FYI, we're going to be adding built-in support for those X_GM attributes as part of bug 721316
(In reply to David Lechner from comment #1)
> Comment on attachment 619356 [details]
> 10031.patch
> 
> There is no unit test for this particular part of the code. I have verified
> that the patch works manaully.

Would be nice to add some :-)
(Assignee)

Comment 4

5 years ago
Comment on attachment 619356 [details]
10031.patch

># HG changeset patch
># User David Lechner <gismho@gmail.com>
># Date 1335674483 18000
># Node ID a5a683c23932bfe58e4b4e38622f3103c0dbc882
># Parent  8b3a9cb529b842ba842c6f42e8e617bcf13dee2c
>[mq]: 2012-04-28_23-40-54_r10030+.diff
>
>diff --git a/mailnews/imap/src/nsImapServerResponseParser.cpp b/mailnews/imap/src/nsImapServerResponseParser.cpp
>--- a/mailnews/imap/src/nsImapServerResponseParser.cpp
>+++ b/mailnews/imap/src/nsImapServerResponseParser.cpp
>@@ -1349,20 +1349,26 @@ void nsImapServerResponseParser::msg_fet
>         if (!fServerConnection.GetCurrentUrl())
>           return;
>         fServerConnection.GetCurrentUrl()->GetImapAction(&imapAction);
>         nsCAutoString userDefinedFetchAttribute;
>         fServerConnection.GetCurrentUrl()->GetCustomAttributeToFetch(userDefinedFetchAttribute);
>         if (imapAction == nsIImapUrl::nsImapUserDefinedFetchAttribute && !strcmp(userDefinedFetchAttribute.get(), fNextToken))
>         {
>           AdvanceToNextToken();
>-          char *fetchResult = CreateParenGroup();
>-          // look through the tokens until we find the closing ')'
>-          // we can have a result like the following:
>-          // ((A B) (C D) (E F))
>+          char *fetchResult;
>+          if (fNextToken[0] == '(') 
>+            // look through the tokens until we find the closing ')'
>+            // we can have a result like the following:
>+            // ((A B) (C D) (E F))
>+            fetchResult = CreateParenGroup();
>+          else {
>+            fetchResult = CreateAstring();
>+            AdvanceToNextToken();
>+          }          
>           fServerConnection.GetCurrentUrl()->SetCustomAttributeResult(nsDependentCString(fetchResult));
>           PR_Free(fetchResult);
>         }
>         else
>           SetSyntaxError(true);
>       }
>       
>         }
(Assignee)

Comment 5

5 years ago
Created attachment 619411 [details] [diff] [review]
same patch with no tabs
Attachment #619356 - Attachment is obsolete: true
Attachment #619356 - Flags: review?(dbienvenu)
Attachment #619411 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #619411 - Flags: review? → review?(dbienvenu)
(Assignee)

Comment 6

5 years ago
(In reply to David :Bienvenu from comment #2)
> Comment on attachment 619356 [details]
> 10031.patch
> 
> it looks like there are tabs in your patch...
> 
> I wonder if this could be tested by just pretending that some built-in
> attributes are custom attributes and running a fetch custom attributes url
> on the built-in attributes in fake server.
> 
> Also, FYI, we're going to be adding built-in support for those X_GM
> attributes as part of bug 721316

No, using the built in commands does not work. I tried it and it turns out the the return value is parsed independently of what function made the request to the server. So, if I used the fetchCustomAttribute function to fetch FLAGS, I don't get anything returned in customAttributeResult.

Thanks for the heads up on the related bug.
(Assignee)

Comment 7

5 years ago
(In reply to Ludovic Hirlimann [:Usul] (away until may 7th) from comment #3)
> (In reply to David Lechner from comment #1)
> > Comment on attachment 619356 [details]
> > 10031.patch
> > 
> > There is no unit test for this particular part of the code. I have verified
> > that the patch works manaully.
> 
> Would be nice to add some :-)

I don't have any experience with test in Mozilla, but I guess that means its time to learn!
(In reply to David Lechner (:dlech) from comment #7)
> 
> I don't have any experience with test in Mozilla, but I guess that means its
> time to learn!

Looking at what you are patching it seems that xpcshell are the way to go see https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests
https://developer.mozilla.org/en/MailNews_xpcshell-tests
https://developer.mozilla.org/en/MailNews_fakeserver
Assignee: nobody → gismho+mozbug
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 9

5 years ago
Created attachment 634072 [details] [diff] [review]
xpcshell test for this bug

At long last! Here is a test to go with this bug. As a bonus feature, the imapd.js changes can be extended for use with bug 721316.
Attachment #634072 - Flags: review?(dbienvenu)

Comment 10

5 years ago
Hey David,
  Thx very much for the xpcshell test. When I run with both patches installed, I still see an assertion:

TEST-PASS | c:/builds/tbirdhq/objdir-tb/mozilla/_tests/xpcshell/mailnews/imap/te
st/unit/test_bug750012.js | [null : 114] 0 == 0
Doing test 1
###!!! ASSERTION: we don't have a paren group!: 'fNextToken[0] == '('', file c:/
builds/tbirdhq/objdir-tb/mailnews/imap/src/../../../../mailnews/imap/src/nsIMAPG
enericParser.cpp, line 413
<<<<<<<
PROCESS-CRASH | c:\builds\tbirdhq\objdir-tb\mozilla\_tests\xpcshell\mailnews\ima
p\test\unit\test_bug750012.js | application crashed (minidump found)
Crash dump filename: c:\builds\tbirdhq\objdir-tb\mozilla\_tests\xpcshell\mailnew
s\imap\test\unit\5e4a8531-6009-4da9-9cda-40c96c5b5fa3.dmp

and assertions make xpcshell tests fail in debug builds. Do you see the same assertion with your patches installed and a debug build?
(Assignee)

Comment 11

5 years ago
Created attachment 634185 [details]
Actual bug fix

It seems that the patch contents disappeared- or more likely, I messed something up when I uploaded 2 months ago.

Please give it a try again.
Attachment #619411 - Attachment is obsolete: true
Attachment #619411 - Flags: review?(dbienvenu)
Attachment #634185 - Flags: review?(dbienvenu)
(Assignee)

Comment 12

5 years ago
Created attachment 634196 [details] [diff] [review]
Actual bug fix

Ha! I messed it up again! 3rd times a charm, right?
Attachment #634185 - Attachment is obsolete: true
Attachment #634185 - Flags: review?(dbienvenu)
Attachment #634196 - Flags: review?(dbienvenu)

Comment 13

5 years ago
Comment on attachment 634196 [details] [diff] [review]
Actual bug fix

thx for the patch!
Attachment #634196 - Flags: review?(dbienvenu) → review+

Comment 14

5 years ago
Comment on attachment 634072 [details] [diff] [review]
xpcshell test for this bug

typo - extensions:
+ * uses Gmail extensios as test case - also useful for bug 721316

I prefer test names to describe what they're testing, not bug numbers, so something like test_fetchCustomAttributes.js would be better.

should remove the commented out lines (occurs twice)

+    //do_check_eq(aUrl.listOfMessageIds, 1);

better test would be to allow the test to tell the server what attributes to set for a given message uid, but I'm so happy that you got the test working that I'm not going to insist on that.

we do prefer let over var where possible, e.g., (and a bunch of other places)

+    var msgHdr = gIMAPInbox.msgDatabase.getMsgHdrForMessageID(gSynthMessage.messageId);
+    var uri = gIMAPInbox.fetchCustomMsgAttribute("X-GM-MSGID", msgHdr.messageKey, gMsgWindow);

Newer imap tests use imapPump.js, which does a lot of the grunt work for you. You might look at using that infrastructure, but again, I won't insist on that.

minusing mainly for the filename issue...
Attachment #634072 - Flags: review?(dbienvenu) → review-
(Assignee)

Comment 15

5 years ago
Created attachment 634295 [details] [diff] [review]
xpcshell test for this bug

fixed typo and file name, removed commented lines, changed to use imapPump, got rid of hard coded test values in server and now using values passed to message, oh, and declared all local variables with let instead of var
Attachment #634072 - Attachment is obsolete: true
Attachment #634295 - Flags: review?(dbienvenu)

Comment 16

5 years ago
Comment on attachment 634295 [details] [diff] [review]
xpcshell test for this bug

great, thx!
Attachment #634295 - Flags: review?(dbienvenu) → review+

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/3cc2a51baba8
https://hg.mozilla.org/comm-central/rev/7631dde8734b

Thanks for the patch, David! To make life easier for those checking in patches on your behalf in the future, please include a commit message in the patch that summarizes what changes it's actually making. Thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
(Assignee)

Comment 18

5 years ago
Will this (can this) patch be included in TB15 when it becomes beta in a few weeks? I am anxious to continue improving my add-on. :-)
It landed for TB16. Feel free to request approval comm-aurora on each patch for consideration for TB15, though.
(Assignee)

Comment 20

5 years ago
Comment on attachment 634196 [details] [diff] [review]
Actual bug fix

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Gmailbuttons add-on uses will have to wait 6 more weeks for improvements.
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): low

I could use some feedback on filling this out. I've never done it before.
Attachment #634196 - Flags: approval-comm-aurora?
(Assignee)

Comment 21

5 years ago
Comment on attachment 634295 [details] [diff] [review]
xpcshell test for this bug

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Gmailbuttons add-on uses will have to wait 6 more weeks for improvements.
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky): low

I could use some feedback on filling this out. I've never done it before.
Attachment #634295 - Flags: approval-comm-aurora?
Comment on attachment 634196 [details] [diff] [review]
Actual bug fix

In the approval section it would be useful to have an executive summary about the issue and the potential impacts. Generally this looks fine though so a=me (we'll get this landed later).
Attachment #634196 - Flags: approval-comm-aurora? → approval-comm-aurora+
Attachment #634295 - Flags: approval-comm-aurora? → approval-comm-aurora+
comm-aurora: 

https://hg.mozilla.org/releases/comm-aurora/rev/4bc10d6ac415
https://hg.mozilla.org/releases/comm-aurora/rev/517923eb47b9
status-thunderbird15: --- → fixed
You need to log in before you can comment on or make changes to this bug.