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

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David Lechner (:dlech)]

Attachment: 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.

Comment 1

[David Lechner (:dlech)]

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.

Comment 2

[David :Bienvenu]

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

Comment 3

[Ludovic Hirlimann [:Usul]]

(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 :-)

Comment 4

[David Lechner (:dlech)]

Comment on attachment 619356 [details]
10031.patch

># HG changeset patch
># User David Lechner <[e-mail]>
># 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);
>       }
>       
>         }

Comment 5

[David Lechner (:dlech)]

Attachment: same patch with no tabs

Comment 6

[David Lechner (:dlech)]

(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.

Comment 7

[David Lechner (:dlech)]

(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!

Comment 8

[Ludovic Hirlimann [:Usul]]

(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

Comment 9

[David Lechner (:dlech)]

Attachment: 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.

Comment 10

[David :Bienvenu]

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?

Comment 11

[David Lechner (:dlech)]

Attachment: 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.

Comment 12

[David Lechner (:dlech)]

Attachment: Actual bug fix

Ha! I messed it up again! 3rd times a charm, right?

Comment 13

[David :Bienvenu]

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

thx for the patch!

Comment 14

[David :Bienvenu]

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...

Comment 15

[David Lechner (:dlech)]

Attachment: 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

Comment 16

[David :Bienvenu]

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

great, thx!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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!

Comment 18

[David Lechner (:dlech)]

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. :-)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

It landed for TB16. Feel free to request approval comm-aurora on each patch for consideration for TB15, though.

Comment 20

[David Lechner (:dlech)]

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.

Comment 21

[David Lechner (:dlech)]

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.

Comment 22

[Mark Banner (:standard8)]

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).

Comment 23

[Mike Conley (:mconley) (:⚙️)]

comm-aurora: 

https://hg.mozilla.org/releases/comm-aurora/rev/4bc10d6ac415
https://hg.mozilla.org/releases/comm-aurora/rev/517923eb47b9