610465 - crash @ nsImapServerResponseParser::ParseIMAPServerResponse after chunking message body

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsImapServerResponseParser::ParseIMAPServerResponse(char const*, int, char*)]

Comment 1

[Wayne Mery (:wsmwk)]

(In reply to comment #0)
> crash [@ nsImapServerResponseParser::ParseIMAPServerResponse(char const*, int,
> char*)]

all releases

bp-28bf0735-803a-4d65-9e8b-3bcf02101108
0	thunderbird.exe	nsImapServerResponseParser::ParseIMAPServerResponse	mailnews/imap/src/nsImapServerResponseParser.cpp:284
1	thunderbird.exe	nsImapProtocol::ParseIMAPandCheckForNewMail	mailnews/imap/src/nsImapProtocol.cpp:1891
2	thunderbird.exe	nsImapProtocol::FetchMessage	mailnews/imap/src/nsImapProtocol.cpp:3452
3	thunderbird.exe	nsImapProtocol::FetchTryChunking	mailnews/imap/src/nsImapProtocol.cpp:3517
4	thunderbird.exe	nsIMAPBodypart::GeneratePart	mailnews/imap/src/nsIMAPBodyShell.cpp:504
5	thunderbird.exe	nsIMAPBodypartLeaf::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:639
6	thunderbird.exe	nsIMAPBodypartMultipart::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:1050
7	thunderbird.exe	nsIMAPBodypartMultipart::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:1050
8	thunderbird.exe	nsIMAPBodypartMessage::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:899
9	thunderbird.exe	nsIMAPBodyShell::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:309
10	thunderbird.exe	nsImapProtocol::ProcessSelectedStateURL	mailnews/imap/src/nsImapProtocol.cpp:2533
11	thunderbird.exe	nsImapProtocol::ProcessCurrentURL	mailnews/imap/src/nsImapProtocol.cpp:1741

Comment 2

[Wayne Mery (:wsmwk)]

 also nsImapServerResponseParser::ParseIMAPServerResponse(char const*, bool, char*) - https://crash-stats.mozilla.com/report/list?product=Thunderbird&query_search=signature&query_type=contains&query=nsImapServerResponseParser&reason_type=contains&date=05%2F25%2F2012%2019%3A58%3A50&range_value=2&range_unit=weeks&hang_type=any&process_type=any&do_query=1&admin=1&signature=nsImapServerResponseParser%3A%3AParseIMAPServerResponse%28char%20const*%2C%20bool%2C%20char*%29

Comment 3

[Wayne Mery (:wsmwk)]

(#2 ranked imap crash)

0	XUL	nsImapServerResponseParser::ParseIMAPServerResponse(char const*, bool, char*)	mailnews/imap/src/nsImapServerResponseParser.cpp
hg@0 244 // check and see if the server is waiting for more input
hg@0 245 // it's possible that we ate this + while parsing certain responses (like cram data),
hg@0 246 // in these cases, the parsing routine for that specific command will manually set
hg@0 247 // fWaitingForMoreClientInput so we don't lose that information....
hg@0 248 if (*fNextToken == '+' || inIdle) 

1	XUL	nsImapProtocol::FetchMessage(nsCString const&, nsIMAPeFetchFields, char const*, unsigned int, unsigned int, char*)	mailnews/imap/src/nsImapProtocol.cpp
hg@0 3525 nsMemory::Free(cCommandStr);
hg@0 3526 if (NS_SUCCEEDED(rv))
hg@0 3527 ParseIMAPandCheckForNewMail(protocolString); 

2	XUL	nsImapProtocol::FetchTryChunking(nsCString const&, nsIMAPeFetchFields, bool, char*, unsigned int, bool)	mailnews/imap/src/nsImapProtocol.cpp
hg@0 3581 // small message, or (we're not chunking and not doing bodystructure),
hg@0 3582 // or the server is not rev1.
hg@0 3583 // Just fetch the whole thing.
mconley@13187 3584 FetchMessage(messageIds, whatToFetch, nullptr, 0, 0, part); 

3	XUL	nsIMAPBodypart::GeneratePart(nsIMAPBodyShell*, bool, bool)	mailnews/imap/src/nsIMAPBodyShell.cpp

Comment 4

[Wayne Mery (:wsmwk)]

still #2 imap crash
bp-ca56bae6-f2d2-4e32-bd3d-339032150428 version 38

Comment 5

[Kent James (:rkent)]

I have not really looked at this one before. Unlike most of our top crashers, the resolution of this seems pretty straight forward. I'll needinfo myself as a reminder.

Comment 6

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 7

[Wayne Mery (:wsmwk)]

magnus, can you take a shot at this?

#2 crash for beta 42.0b2.
#3 crash for 38.3.0.

Comment 8

[Magnus Melin [:mkmelin]]

Sorry, don't know why http://mxr.mozilla.org/comm-esr38/source/mailnews/imap/src/nsImapServerResponseParser.cpp#243 would crash. But it sounds like Kent had some idea.

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to Kent James (:rkent) from comment #5)
> I have not really looked at this one before. Unlike most of our top
> crashers, the resolution of this seems pretty straight forward. I'll
> needinfo myself as a reminder.

Kent, can you give a hint of what to dig into, so that someone can carry the torch on this?

Comment 10

[Richard Leger]

Attachment: crashReport-bp-43e5960e-3124-4617-8536-bd8c82160307.json

Also affected by this bug with Thunderbird 38.6.0 on Win7 pro 64bits machine.
Here attached it the full raw crash report if that can be of any help. 

Similar bug reports appears in the crash reports list showing that this issue already appeared in Thunderbird 38.5.1

Comment 11

[Richard Leger]

End-users report crash occurs at least once a day with some enduring it at much higher frequency (7 or more times a day). If a quick workaround can be applied manually while the issue is being fixed and published for next Thunderbird update, please let us know, that would be much appreciated :-)

Comment 12

[Kent James (:rkent)]

I believe this crash happens on null fNextToken here in nsImapServerResponseParser.cpp:

      // check and see if the server is waiting for more input
      // it's possible that we ate this + while parsing certain responses (like cram data),
      // in these cases, the parsing routine for that specific command will manually set
      // fWaitingForMoreClientInput so we don't lose that information....
      if (*fNextToken == '+' || inIdle)
      {
        fWaitingForMoreClientInput = true;
      }

So a simple null check is likely to stop the crash. What still needs doing is deciding what actions, if any, to take if null. But perhaps just this would do:

if ((fNextToken && *fNextToken == '+') || inIdle)

Comment 13

[Kent James (:rkent)]

Attachment: Check for null fNextToken

Simple patch that should fix the crash.

Comment 14

[Wayne Mery (:wsmwk)]

Thanks for taking a look at this.

Some data, which may help in finding testers and assessing results:
- many crashes have various [e-mail] [1]. Most have an addon obel, and/or lightning.  
- a different but similar stack is nsImapServerResponseParser::msg_fetch_literal [2] bp-11788af9-4f25-45af-a193-a0c312160304 (NOTE ParseIMAPServerResponse is frame 5) - the user is  pawel bp-e8a638d1-8630-44be-aef0-b9a7a2160310 
- one beta user bp-596ca7a1-60c6-4eec-9829-361ec2160229 also has a different stack nsCOMPtr_base::assign_with_AddRef | nsMsgBodyHandler::nsMsgBodyHandler  
- it is a rare user that has more than one crash in a period of months, i.e. the vast majority (70%?) are one and done
-- simon has several bp-8722bb8e-7874-4a9e-a409-59b582160310 

[1] @Oracle crashes
bp-cf2bff8a-d5d9-4726-9da5-b834b2160307
bp-2abcbe82-b337-4c37-9d6b-117e32160217
bp-bd4aa66e-64e4-4f14-b1c8-74fa52160309
bp-f26fd31b-ce4f-448f-9694-6ec022160217
bp-9384bcba-5687-48cb-bb3b-b27162160308


[2] https://crash-stats.mozilla.com/search/?signature=%3DnsImapServerResponseParser%3A%3Amsg_fetch_literal&product=Thunderbird&_facets=signature&_columns=date&_columns=signature&_columns=version&_columns=build_id&_columns=platform&_columns=user_comments#crash-reports

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 8729716 [details] [diff] [review]
Check for null fNextToken

Review of attachment 8729716 [details] [diff] [review]:
-----------------------------------------------------------------

Let's try it. r=mkmelin

Comment 16

[Kent James (:rkent)]

Comment on attachment 8729716 [details] [diff] [review]
Check for null fNextToken

http://hg.mozilla.org/releases/comm-aurora/rev/cd63248f548a

Comment 17

[Kent James (:rkent)]

http://hg.mozilla.org/releases/comm-aurora/rev/cd63248f548a
http://hg.mozilla.org/releases/comm-beta/rev/d6fcf3c1deb1

Comment 18

[Kent James (:rkent)]

Comment on attachment 8729716 [details] [diff] [review]
Check for null fNextToken

[Triage Comment]

Comment 19

[Kent James (:rkent)]

Comment on attachment 8729716 [details] [diff] [review]
Check for null fNextToken

[Triage Comment]

Comment 20

[Kent James (:rkent)]

http://hg.mozilla.org/releases/comm-esr45/rev/98035e8a63be

Comment 21

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/4cdf4ffd43e0

Comment 22

[Wayne Mery (:wsmwk)]

so far none in b3 https://crash-stats.mozilla.com/search/?product=Thunderbird&version=45.0b0&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 23

[Richard Leger]

Could this patch be back ported to the next minor update of Thunderbird 38.7 (e.g 38.7.3 or 38.8)? Waiting for upgrade to 48.0 seems to be far fetched... End-users would need this patch sooner than later...

What does comm-central repository stand for (as per Comment 21)? 
Is it for core feature?

Comment 24

[Wayne Mery (:wsmwk)]

(In reply to Richard Leger from comment #23)
> Could this patch be back ported to the next minor update of Thunderbird 38.7
> (e.g 38.7.3 or 38.8)? Waiting for upgrade to 48.0 seems to be far fetched...
> End-users would need this patch sooner than later...

I have no objections landing on 38 if Kent has none - so far no reported regressions.


> What does comm-central repository stand for (as per Comment 21)? 
> Is it for core feature?

Not core feature. c-c is the development channel of Thunderbird.

Comment 25

[Richard Leger]

Wayne thanks for feedback.

Just discovered that Thunderbird 45 was just released:
https://www.mozilla.org/en-US/thunderbird/45.0/releasenotes/

It fixed "Crashed in some cases while parsing IMAP messages." which I believe relate to this bug (hopefully).

Thank you guys for your work on this issue. Most appreciated.

Comment 26

[Kent James (:rkent)]

Yes it is a trivial patch, but 38 is almost EOL at this point. I'll at least flag this.

Comment 27

[Kent James (:rkent)]

Comment on attachment 8729716 [details] [diff] [review]
Check for null fNextToken

http://hg.mozilla.org/releases/comm-esr38/rev/66db30aed2d9

Comment 28

[Kent James (:rkent)]

Pushed for TB 38.8

Comment 29

[Wayne Mery (:wsmwk)]

crashing is significantly reduced in version 45 compared to version 38:
hunderbird	50.0a1	1	0.1%	1
Thunderbird	45.1.1	46	4.2%	44
Thunderbird	45.0	1	0.1%	1
Thunderbird	44.0b1	5	0.5%	5
Thunderbird	43.0a2	1	0.1%	1
Thunderbird	42.0b1	1	0.1%	1
Thunderbird	38.7.2	280	25.4%	232
Thunderbird	38.7.1	59	5.4%	34
Thunderbird	38.7.0	23	2.1%	21
https://crash-stats.mozilla.com/signature/?signature=nsImapServerResponseParser%3A%3AParseIMAPServerResponse&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#summary

However, some potentially related crashes continue - bug 1284302, bug 1222455