1264302 - Random crash getting new email in nsImapServerResponseParser::msg_fetch_literal. Made worse by, but not caused by, fetch_by_chunks

Status: RESOLVED DUPLICATE of bug 1216951

(Thunderbird :: Message Compose Window, defect)

Description

[Richard Leger]

This bug was filed from the Socorro interface and is 
report bp-697da2f1-3eae-462f-a4d7-e68292160413.
=============================================================

Comment 1

[Richard Leger]

Attachment: CrashReportReport ID_ 697da2f1-3eae-462f-a4d7-e68292160413.zip

Thunderbird 38.7.2 Crash Report [@ nsImapServerResponseParser::msg_fetch_literal ]
Search Mozilla Support for Help
ID: 697da2f1-3eae-462f-a4d7-e68292160413 
Signature: nsImapServerResponseParser::msg_fetch_literal

Comment 2

[Richard Leger]

End user was replying to an email, and when typing in the body of the message, the application crashed unexpectedly. After re-opening application, end-user manage to resume msg from draft and send message.

Comment 3

[Richard Leger]

It happens repeatedly since last week about 5 times per week.

Comment 4

[Wayne Mery (:wsmwk)]

Thanks for the bug report. Not necessary to attach the crash report page - it's nice to access it online. See the link ...

bp-697da2f1-3eae-462f-a4d7-e68292160413
0 	xul.dll	nsImapServerResponseParser::msg_fetch_literal(bool, int)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:3151
1 	xul.dll	nsImapServerResponseParser::msg_fetch_content(bool, int, char const*)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:2157
2 	xul.dll	nsImapServerResponseParser::mime_part_data()	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:2683
3 	xul.dll	nsImapServerResponseParser::msg_fetch()	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:1348
4 	xul.dll	nsImapServerResponseParser::response_data()	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:702
5 	xul.dll	nsImapServerResponseParser::ParseIMAPServerResponse(char const*, bool, char*)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:205
6 	xul.dll	nsImapProtocol::ParseIMAPandCheckForNewMail(char const*, bool)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/imap/src/nsImapProtocol.cpp:1928
7 	xul.dll	nsImapProtocol::FetchMessage(nsCString const&, nsIMAPeFetchFields, char const*, unsigned int, unsigned int, char*)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/imap/src/nsImapProtocol.cpp:3568 

Perhaps is same issue as bug 610465. But it might be different line numbers.
We should see what happens after user/reporter can test the fix of bug 610465.

Comment 5

[Richard Leger]

Ok I agree. 

Let's check if Thunderbird 45 (https://www.mozilla.org/en-US/thunderbird/45.0/releasenotes/) that has just been released make a difference as it seems to sort bug 610465 in theory.

I will ask end-user to upgrade when possible and report back.

Thanks for the tip, I did not realised that the crash report was made available online... great feature!

As an unrelated question, if an issue raise within Thunderbird which does not crash application, is there a way to force creation of a crash report to provide information for analyse/bug report? Via some sort of command or button within Thunderbird?

I tried to run Thunderbird in log mode but without great success at finding source of issue described in bug 1222691. Could this bug be re-opened? I do have users having this issue and I am ready to work on it with your support.

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Richard Leger from comment #5)
> Ok I agree. 
> 
> Let's check if Thunderbird 45
> (https://www.mozilla.org/en-US/thunderbird/45.0/releasenotes/) that has just
> been released make a difference as it seems to sort bug 610465 in theory.
> 
> I will ask end-user to upgrade when possible and report back.

good results?


 
> As an unrelated question, if an issue raise within Thunderbird which does
> not crash application, is there a way to force creation of a crash report to
> provide information for analyse/bug report? Via some sort of command or
> button within Thunderbird?

what issue?
if performance, a crash dump is not useful but...

> I tried to run Thunderbird in log mode but without great success at finding
> source of issue described in bug 1222691. Could this bug be re-opened? I do
> have users having this issue and I am ready to work on it with your support.

For your performance issue please create a support topic at https://support.mozilla.org/en-US/questions/new and post the URL here

Comment 7

[Richard Leger]

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #6)
> (In reply to Richard Leger from comment #5)
> > Ok I agree. 
> > 
> > Let's check if Thunderbird 45
> > (https://www.mozilla.org/en-US/thunderbird/45.0/releasenotes/) that has just
> > been released make a difference as it seems to sort bug 610465 in theory.
> > 
> > I will ask end-user to upgrade when possible and report back.
> 
> good results?
> 

Sorted by upgrade to TB 45.0

>  
> > As an unrelated question, if an issue raise within Thunderbird which does
> > not crash application, is there a way to force creation of a crash report to
> > provide information for analyse/bug report? Via some sort of command or
> > button within Thunderbird?
> 
> what issue?
> if performance, a crash dump is not useful but...

If Thunderbird is not crashing but regularly Not Responding, what is the best way to find out what the issue can be? Safe mode, console, or logs not showing information that could explain issue...
Had done some memory dump but Ibdon't have sufficient level of expertise to analyse them... though I could see two dump could be compared...

> > I tried to run Thunderbird in log mode but without great success at finding
> > source of issue described in bug 1222691. Could this bug be re-opened? I do
> > have users having this issue and I am ready to work on it with your support.
> 
> For your performance issue please create a support topic at
> https://support.mozilla.org/en-US/questions/new and post the URL here

Issue was application not responding randomely, with frequent delay when typing text in composing windows after 3H of use. Seems upgrade to 45.0 version improved situation.
Restarting application was sorting issue for a while before it came back.

Seems gone in 45.0 version but too early to tell for sure...

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to Richard Leger from comment #7)
> (In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #6)
> > (In reply to Richard Leger from comment #5)
> > > Ok I agree. 
> > > 
> > > Let's check if Thunderbird 45
> > > (https://www.mozilla.org/en-US/thunderbird/45.0/releasenotes/) that has just
> > > been released make a difference as it seems to sort bug 610465 in theory.
> > > 
> > > I will ask end-user to upgrade when possible and report back.
> > 
> > good results?
> > 
> 
> Sorted by upgrade to TB 45.0

Then let's close this WFM.
(although we do have other crash reports for this signature)

For your other issues, I have replied by email

Comment 9

[Richard Leger]

It seems issue re-occurred... so I re-opened this ticket...

Thunderbird 45.2.0 Crash Report [@ nsImapServerResponseParser::msg_fetch_literal ]
Search Mozilla Support for Help
ID: 126ee39a-735d-4d21-8138-ceecd2160802
Signature: nsImapServerResponseParser::msg_fetch_literal

If the bug report not sufficient to identify the issue, is there a way to increase debug log on the client side to find out the reason of issue?

Comment 10

[Wayne Mery (:wsmwk)]

bp-126ee39a-735d-4d21-8138-ceecd2160802
 0 	xul.dll	nsImapServerResponseParser::msg_fetch_literal(bool, int)	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:3153
1 	xul.dll	nsImapServerResponseParser::msg_fetch_content(bool, int, char const*)	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:2158
2 	xul.dll	nsImapServerResponseParser::mime_part_data()	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:2684
3 	xul.dll	nsImapServerResponseParser::msg_fetch()	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:1349
4 	xul.dll	nsImapServerResponseParser::response_data()	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:703
5 	xul.dll	nsImapServerResponseParser::ParseIMAPServerResponse(char const*, bool, char*)	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:206

Comment 11

[Wayne Mery (:wsmwk)]

> If the bug report not sufficient to identify the issue, is there a way to increase debug log on the client side to find out the reason of issue?

As far as I know, not as of today. That makes it frustrating of course.

FWIW, beta build crashes for this signature are much lower with version 50. Which doesn't of course mean the issue is fixed. https://crash-stats.mozilla.com/signature/?product=Thunderbird&release_channel=%21release&signature=nsImapServerResponseParser%3A%3Amsg_fetch_literal&date=%3E%3D2016-05-14T17%3A10%3A26.000Z&date=%3C2016-11-14T17%3A10%3A26.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-version&_sort=-date&page=1

If you want to try the beta, see http://www.mozilla.org/en-US/thunderbird/channel/

Comment 12

[Richard Leger]

Last seen occurrences of the msg_fetch crash issue seems to be sept 2016 TB 45.2... Not seem present in TB 45.3 or 45.4...(yet ;-)

Though in the past three months TB 42.2, 42.3 and 42.4 have been crashing on multiple computers (win7b64bits) multiple time per months with various bug reports...

Comment 13

[Richard Leger]

Correction 45.3 and 45.4 versions are affected as well:

bp-fc0508c9-2079-4a34-9db2-c4e372161010
    10/10/2016    14:51
Thunderbird 45.4.0 Crash Report [@ nsImapServerResponseParser::msg_fetch_literal ]

bp-ef1510d0-44e7-4c57-a7fd-1a1992160928
    28/09/2016    14:27
Thunderbird 45.3.0 Crash Report [@ nsImapServerResponseParser::msg_fetch_literal ]

Will wait for 50 stable version... I am afraid a beta could make things worth...

Comment 14

[Richard Leger]

> Though in the past three months TB 42.2, 42.3 and 42.4 have been crashing on
> multiple computers (win7b64bits) multiple time per months with various bug
> reports...

I meant 45.2, 45.3, 45.4 in sentence above...

Comment 15

[Richard Leger]

FYI, TB still crashing randomely and unexpectedly for various users on various machines when opening an email ... let's hope when TB 50 become stable that will reduce crashes to end-users...

Thunderbird 45.5.1 Crash Report [@ nsImapServerResponseParser::msg_fetch_literal ]
bp-5e8ce944-f209-484a-aa71-91ce02161207    07/12/2016    13:15 NR
https://crash-stats.mozilla.com/report/index/5e8ce944-f209-484a-aa71-91ce02161207
Note: End-user clicked email to open it

Thunderbird 45.4.0 Crash Report [@ nsImapServerResponseParser::msg_fetch ]
bp-dc4f9fc2-b0f2-4738-982d-a45b52161031     31/10/2016    10:06 MAK
bp-e0ff9b00-ca7c-48f3-82c2-ceb2c2161025     25/10/2016    13:59 NR
bp-fc0508c9-2079-4a34-9db2-c4e372161010     10/10/2016    14:51 NR

Thunderbird 45.3.0 Crash Report [@ nsImapServerResponseParser::msg_fetch_literal ]
bp-ef1510d0-44e7-4c57-a7fd-1a1992160928     28/09/2016    14:27 NR
bp-4f65ec71-179f-4aee-880f-2276e2160919     19/09/2016    16:49 MM

Thunderbird 45.2.0 Crash Report [@ nsImapServerResponseParser::msg_fetch_literal ]
bp-cec6145c-39ca-4d1b-bc07-5a5772160920     20/09/2016    12:55 SD
bp-bc62b1e9-c958-43c5-a73a-6eb7e2160907     07/09/2016    10:01 MAK
bp-d0a23af5-0fc2-4951-afc2-3f89e2160823     23/08/2016    14:33 MM

Comment 16

[Wayne Mery (:wsmwk)]

> let's hope when TB 50 become stable that will reduce crashes to end-users...

Version 50 doesn't get a grand public release, nor 51. The next big release is 52.
FWIW, the betas are more stable for some users than the current release.

I don't like these stacks.
What anvirus software do you run?

Comment 17

[Richard Leger]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #16)
> > let's hope when TB 50 become stable that will reduce crashes to end-users...
> 
> Version 50 doesn't get a grand public release, nor 51. The next big release
> is 52.
> FWIW, the betas are more stable for some users than the current release.

Well will have to wait for the rock solid non-crashing 52 version then :-) one can dream :-)
What is the approximate release date planned for version 52, would you know?

> I don't like these stacks.

Not sure what you mean by this...

> What anvirus software do you run?

Microsoft Security Essentials

Comment 18

[Wayne Mery (:wsmwk)]

(In reply to Richard Leger from comment #17)
> (In reply to Wayne Mery (:wsmwk, NI for questions) from comment #16)
> > > let's hope when TB 50 become stable that will reduce crashes to end-users...
> > 
> > Version 50 doesn't get a grand public release, nor 51. The next big release
> > is 52.
> > FWIW, the betas are more stable for some users than the current release.
> 
> Well will have to wait for the rock solid non-crashing 52 version then :-)
> one can dream :-)
> What is the approximate release date planned for version 52, would you know?

It's a long wait. See the release column at https://wiki.mozilla.org/RapidRelease/Calendar

> > I don't like these stacks.
> 
> Not sure what you mean by this...

Sometimes the stack for the crash isn't very helpful. Put another way, it is better if a bug report has steps to reliably cause the crash. (but we also know in most cases this is not possible)

Comment 19

[Wayne Mery (:wsmwk)]

FWIW, from bp-423cc4e5-069a-4c50-b4fb-edbbf2170123
0 	xul.dll	nsImapServerResponseParser::msg_fetch_literal(bool, int)	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:3153
1 	xul.dll	nsImapServerResponseParser::msg_fetch_content(bool, int, char const*)	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:2158
2 	xul.dll	nsImapServerResponseParser::mime_part_data()	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:2684
3 	xul.dll	nsImapServerResponseParser::msg_fetch()	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:1349
4 	xul.dll	nsImapServerResponseParser::response_data()	c:/builds/moz2_slave/tb-rel-c-esr45-w32_bld-0000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:703 


https://hg.mozilla.org/releases/comm-esr45/annotate/tip/mailnews/imap/src/nsImapServerResponseParser.cpp#l2158
  if (PL_strcasecmp(fNextToken, "NIL"))
  {
    if (*fNextToken == '"')
      fLastChunk = msg_fetch_quoted();
    else
2158    fLastChunk = msg_fetch_literal(chunk, origin);


https://hg.mozilla.org/releases/comm-esr45/annotate/tip/mailnews/imap/src/nsImapServerResponseParser.cpp#l3153
        *displayEndOfLine = 0;
        fServerConnection.HandleMessageDownLoadLine(fCurrentLine, specialLineEnding || !lastChunk);
        *displayEndOfLine = saveit;
        lastCRLFwasCRCRLF = (*(displayEndOfLine - 1) == '\r');
      }
      else
      {
3153     lastCRLFwasCRCRLF = (*(fCurrentLine + strlen(fCurrentLine) - 1) == '\r');

Comment 20

[Wayne Mery (:wsmwk)]

Richard, please try this from bug 434054 comment 9

lockPref("mail.imap.fetch_by_chunks", false);
lockPref("mail.server.default.fetch_by_chunks", false);
lockPref("mail.server.default.mime_parts_on_demand", false);

Comment 21

[Richard Leger]

This is do be set in Options > Advanced > General > Config Editor right?

Comment 22

[Wayne Mery (:wsmwk)]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #20)
> Richard, please try this from bug 434054 comment 9
> 
> lockPref("mail.imap.fetch_by_chunks", false);
> lockPref("mail.server.default.fetch_by_chunks", false);
> lockPref("mail.server.default.mime_parts_on_demand", false);

Richard, did this help?

Comment 23

[Richard Leger]

Haven't gt a chance to implement it yet, keep you posted...

Comment 24

[Richard Leger]

Sorry guys, I just realised that this crash was reported 11 months ago and I cannot find the computer that was encountering the issue to set the settings as advised to test in TB... I would say for now we can consider it closed as since then TB has been updated many times... Unless you would want me to test those settings on a different version of TB, I currently use 52 beta channel... Let me know. Happy to help.

Comment 25

[Wayne Mery (:wsmwk)]

Definitely test this on other computers that are crashing, regardless of OS and version, that also crash and are loading the same emails.

And we'd leave the bug open anyway, because other users are seeing crashes, including version 52
https://crash-stats.mozilla.com/signature/?release_channel=%21release&signature=nsImapServerResponseParser%3A%3Amsg_fetch_literal&date=%3E%3D2016-12-10T14%3A53%3A10.000Z&date=%3C2017-03-10T14%3A53%3A10.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1

Comment 26

[Richard Leger]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #22)
> (In reply to Wayne Mery (:wsmwk, NI for questions) from comment #20)
> > Richard, please try this from bug 434054 comment 9
> > 
> > lockPref("mail.imap.fetch_by_chunks", false);

Not available in TB 45.8 but implemented ("mail.imap.mime_parts_on_demand", false) instead

> > lockPref("mail.server.default.fetch_by_chunks", false);

Implemented

> > lockPref("mail.server.default.mime_parts_on_demand", false);

Implemented
 
> Richard, did this help?

Keep you posted :-)

Comment 27

[Richard Leger]

Just to confirm, it seems that Thunderbird has not crashed since the options above were implemented. Current user TB version is 45.8.0.

Comment 28

[Wayne Mery (:wsmwk)]

What can you make of comment 19?
Or, is bug 1222455 a better case to look at?

Comment 29

[Jorg K (CEST = GMT+2)]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #28)
> What can you make of comment 19?
I don't understand the twisted logic in that code:
3153     lastCRLFwasCRCRLF = (*(fCurrentLine + strlen(fCurrentLine) - 1) == '\r');
will crash if fCurrentLine is null. I see no evidence that this is guaranteed to be non-null. It would be good to see a reproducible case before adding any further checks here.

> Or, is bug 1222455 a better case to look at?
I don't think bug 1222455 has got anything to do with this.

Comment 30

[Makoto Kato [:m_kato]]

(In reply to Jorg K (GMT+2) from comment #29)
> (In reply to Wayne Mery (:wsmwk, NI for questions) from comment #28)
> > What can you make of comment 19?
> I don't understand the twisted logic in that code:
> 3153     lastCRLFwasCRCRLF = (*(fCurrentLine + strlen(fCurrentLine) - 1) ==
> '\r');
> will crash if fCurrentLine is null. I see no evidence that this is
> guaranteed to be non-null. It would be good to see a reproducible case
> before adding any further checks here.

If null, "charsReadSoFar += strlen(fCurrentLine);" may cause crash at first.  But we should add nullptr check for safety.

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #28)
> Or, is bug 1222455 a better case to look at?

Bug 1222455 will be that mMsgWindow is invalid in nsMessenger...  This isn't same.

Comment 31

[Wayne Mery (:wsmwk)]

jpr is another user who crashes 
bp-83dd2c8b-5494-4e54-af58-8ad410171003 nsMsgLineStreamBuffer::ReadNextLine   
bp-d89f4ffd-1ba7-44ab-bba2-748be0170928 nsImapServerResponseParser::msg_fetch_content   
bp-0623b806-6852-4571-a0c9-82dab0170927 nsImapProtocol::GetMessageSize   
bp-58d8c916-1118-4501-b715-05a450170920 nsImapProtocol::HandleMessageDownLoadLine   
bp-0538f4ac-5319-48ae-9146-fb4bc0170913 nsImapServerResponseParser::msg_fetch_literal  
bp-1303ab94-27f1-4ad7-bb30-02aab0170907 _invalid_parameter

Comment 32

[Wayne Mery (:wsmwk)]

crash rank in 52 has dropped toward #140

>  If null, "charsReadSoFar += strlen(fCurrentLine);" may cause crash at first.  But we should add nullptr check for safety.

Can we add this check as the first step?

Comment 33

[Jorg K (CEST = GMT+2)]

Hmm, I'm really confused, is there a recent crash ID pointing to the crash you want to fix? I need an up-to-date line number.

I looked myself
https://crash-stats.mozilla.com/signature/?signature=nsImapServerResponseParser%3A%3Amsg_fetch_literal&date=%3E%3D2017-11-26T20%3A24%3A07.000Z&date=%3C2017-12-03T20%3A24%3A07.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

and found some recent crashes:
bp-0472722a-2872-49bf-b797-f92fc0171203 - TB 52.5, crashed today on
nsImapServerResponseParser.cpp:3153
which is
  lastCRLFwasCRCRLF = (*(fCurrentLine + strlen(fCurrentLine) - 1) == '\r');

bp-e4998124-a5d3-427c-9ef8-f4e540171201 - TB 58 b1, crashed a few days ago on
nsImapServerResponseParser.cpp:3135
which is
  lastCRLFwasCRCRLF = (*(fCurrentLine + strlen(fCurrentLine) - 1) == '\r');
so the same line which is here:

https://dxr.mozilla.org/comm-central/rev/5c41c835bbc316c5f50e095741f75ccd4841c2fc/mailnews/imap/src/nsImapServerResponseParser.cpp#3135

So I don't understand how the comment
> If null, "charsReadSoFar += strlen(fCurrentLine);" may cause crash at first.
> But we should add nullptr check for safety.
would apply here.

Besides, that line is at
https://dxr.mozilla.org/comm-central/rev/5c41c835bbc316c5f50e095741f75ccd4841c2fc/mailnews/imap/src/nsImapServerResponseParser.cpp#3115
and *fCurrentLine is accessed before in
3104  if (lastCRLFwasCRCRLF && (*fCurrentLine == '\r'))
so
3115  charsReadSoFar += strlen(fCurrentLine);
shouldn't crash.

Makoto-san, can you please clarify.

I don't see how line 3135 can crash when 3104 and 3115 didn't crash. What am I missing?

Comment 34

[ISHIKAWA, Chiaki]

(In reply to Jorg K (GMT+1) from comment #33)

> Besides, that line is at
> https://dxr.mozilla.org/comm-central/rev/
> 5c41c835bbc316c5f50e095741f75ccd4841c2fc/mailnews/imap/src/
> nsImapServerResponseParser.cpp#3115
> and *fCurrentLine is accessed before in
> 3104  if (lastCRLFwasCRCRLF && (*fCurrentLine == '\r'))
> so
> 3115  charsReadSoFar += strlen(fCurrentLine);
> shouldn't crash.

A comment from a passer-by.

If lastCRLFwasCRCRLF is false, fCurrentLine on 3104 is not accessed.
So it is possible that the fCurrentLine on 3115 can be the first access.

Comment 35

[Jorg K (CEST = GMT+2)]

Oh, good catch! Thanks.

So check fCurrentLine before line 3115, and if null, do what? Continue, break, error?

Comment 36

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (GMT+1) from comment #35)
> Oh, good catch! Thanks.
> 
> So check fCurrentLine before line 3115, and if null, do what? Continue,
> break, error?

Comment 37

[ISHIKAWA, Chiaki]

(In reply to Wayne Mery (:wsmwk) from comment #36)
> (In reply to Jorg K (GMT+1) from comment #35)
> > Oh, good catch! Thanks.
> > 
> > So check fCurrentLine before line 3115, and if null, do what? Continue,
> > break, error?

I am trying to figure this out, Wayne.
But I found the code in this file sucks. The code parses the command and response of Imap protocol.
However, the code is handcrafted in a somewhat ad-hoc manner.
Error handling is not uniform or robust as we found here.

I would prefer to have an automated parser created by a parser generator such as bison since that type of parser is very robust and it gives a very high-level control of the parser when a parse error (i.e., incorrect response from the remote server) occurs.
I will keep an eye on the rewriting possibility while I try to figure out what course of action should take place.
(We may have to combine a lexical analyzer generated by a generator such as lex/flex for really robust parser of the IMAP protocol with good protection in case of parse error, i.e. command/response incompatibilities, etc.)

TIA

Comment 38

[Makoto Kato [:m_kato]]

When running this code, fCurrentLine isn't nullptr.

 charsReadSoFar += strlen(fCurrentLine);

But when running this code, fCurrentLine seems to be nullptr.

 lastCRLFwasCRCRLF = (*(fCurrentLine + strlen(fCurrentLine) - 1) == '\r');

So If PercentProgressUpdateEvent calls any method that updates fCurrentLine, fCurrentLine becomes nullptr.  But I don't know root cause.

Comment 39

[Jorg K (CEST = GMT+2)]

Gene is busily rewriting the chunking logic in bug 1216951, I'll make him aware.

Gene, the lastCRLFwasCRCRLF goes with your patch in bug 1216951, so let's see whether this crash here goes as well. NI just to attract attention ;-)

Comment 40

[gene smith]

Thanks, will watch for problems here. (NI still on.)

Comment 41

[Wayne Mery (:wsmwk)]

Mac signature strlen | nsImapServerResponseParser::msg_fetch_literal
bp-c0ee4c4d-4423-42af-8a0c-de2670180221
bp-f5c4a002-e59b-41cc-91a7-57dbc0180220
bp-f0c8535f-853f-45f5-a50c-142640180219

Comment 42

[Jorg K (CEST = GMT+2)]

Most recent crashed in TB 52.6 of 2018-02-04 crash here: nsImapServerResponseParser.cpp:3144 

In the c-esr52 repository that's
char *displayEndOfLine = (fCurrentLine + strlen(fCurrentLine) - (charsReadSoFar - numberOfCharsInThisChunk));
https://hg.mozilla.org/releases/comm-esr52/annotate/acc1e1efc4f4/mailnews/imap/src/nsImapServerResponseParser.cpp#l3144
That code is up for replacement in bug 1216951.

Comment 43

[gene smith]

(In reply to Jorg K (GMT+1) from comment #39)
> Gene is busily rewriting the chunking logic in bug 1216951, I'll make him
> aware.
> 
> Gene, the lastCRLFwasCRCRLF goes with your patch in bug 1216951, so let's
> see whether this crash here goes as well. NI just to attract attention ;-)

Submitted patch in bug 1216951. In my testing and usage with patch, haven't seen any crashes.

Comment 44

[Wayne Mery (:wsmwk)]

I don't know if it's feasible, but it would be

Comment 45

[Wayne Mery (:wsmwk)]

(In reply to Wayne Mery (:wsmwk) from comment #44)
> I don't know if it's feasible, but it would be

Hmm, incomplete thought and I don't remember what I intended.


Richard, it would be fantastic if you could test version 60 beta and give us a quick report.  It should be out milddle of this week at http://www.mozilla.org/en-US/thunderbird/channel/

Comment 46

[Richard Leger]

I will install 60 beta when available... on my laptop I dont have roaming profile enable and I mostly use TB outside local network for email remote location at the moment... will send crash reports that may occur...

Comment 47

[gene smith]

(In reply to Richard Leger from comment #46)
> will send crash reports that may occur...

Also, exactly what was been done when it crashes would be helpful. Thanks for testing!

Comment 48

[Wayne Mery (:wsmwk)]

looking at past several months of crash-stats I can't clearly tell if crash rate dropped as a result of bug 1216951 "Fix broken handling of split CR and LF between chunks" being patched in version 60. It might have dropped, might not.  But clearly the crash signature still appears, so we have more work to do.  bp-b096a822-5549-47b5-8351-232440180430 60.0b4

https://crash-stats.mozilla.com/search/?release_channel=beta&release_channel=nightly&signature=~nsImapServerResponseParser%3A%3Amsg_fetch&product=Thunderbird&date=%3E%3D2017-11-02T06%3A44%3A12.000Z&date=%3C2018-05-02T06%3A44%3A12.000Z&_sort=version&_sort=-date&_facets=signature&_facets=email&_columns=date&_columns=signature&_columns=version&_columns=platform&_columns=email&_columns=user_comments#crash-reports

Comment 50

[Wayne Mery (:wsmwk)]

(In reply to gene smith from comment #47)
> (In reply to Richard Leger from comment #46)
> > will send crash reports [with beta 60] that may occur...
> 
> Also, exactly what was been done when it crashes would be helpful. Thanks
> for testing!

Richard, 

Note - doesn't matter if you are using roamning

Comment 51

[gene smith]

Wayne, The line where the crash occurs (see Comment 42 above) actually occurs even when not fetching in chunks, although it probably occurs more often when you are. So I have my doubts that the changes in bug 1216951 will have any effect on the crash frequency.

Comment 52

[Richard Leger]

(In reply to gene smith from comment #47)
> (In reply to Richard Leger from comment #46)
> > will send crash reports that may occur...
> 
> Also, exactly what was been done when it crashes would be helpful. Thanks
> for testing!

What was done, to generate the crash originally reported, is indicated in Comment 2.

Comment 53

[Richard Leger]

(In reply to Wayne Mery (:wsmwk) from comment #50)
> (In reply to gene smith from comment #47)
> > (In reply to Richard Leger from comment #46)

> Richard,
> Note - doesn't matter if you are using roamning

What this a question to me? or a comment?

Comment 54

[Wayne Mery (:wsmwk)]

(In reply to Richard Leger from comment #53)
> (In reply to Wayne Mery (:wsmwk) from comment #50)
> > (In reply to gene smith from comment #47)
> > > (In reply to Richard Leger from comment #46)
> 
> > Richard,
> > Note - doesn't matter if you are using roamning
> 
> What this a question to me? or a comment?

That was just a comment, because iirc you use roaming.  Even if not, nevermind.

like bug 1333038 this signature still exists on beta.  But rare.

Comment 55

[Wayne Mery (:wsmwk)]

(In reply to gene smith from comment #51)
> Wayne, The line where the crash occurs (see Comment 42 above) actually
> occurs even when not fetching in chunks, although it probably occurs more
> often when you are.

(In reply to Wayne Mery (:wsmwk) from comment #48)
> looking at past several months of crash-stats I can't clearly tell if crash
> rate dropped as a result of bug 1216951 "Fix broken handling of split CR and
> LF between chunks" being patched in version 60. It might have dropped, might
> not.  But clearly the crash signature still appears, so we have more work to
> do.  bp-b096a822-5549-47b5-8351-232440180430 60.0b4

This beta+nightly graph from a longer period is a bit more conclusive https://crash-stats.mozilla.com/signature/?release_channel=%21release&signature=nsImapServerResponseParser%3A%3Amsg_fetch_literal&date=%3E%3D2017-12-17T13%3A03%3A45.000Z&date=%3C2018-06-17T15%3A03%3A45.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#graphs

So the crash rate is perhaps half and perhaps that is where bug bug 1216951 helped. 

Richard (our bellweather case) can you confirm whether you and your users still crash, or crash less, when using beta?

Comment 56

[Wayne Mery (:wsmwk)]

(In reply to Wayne Mery (:wsmwk) from comment #55)
> 
> Richard (our bellweather case) can you confirm whether you and your users
> still crash, or crash less, when using beta?

That is, with the original settings which caused the crash mentioned in comment 26

Comment 57

[Wayne Mery (:wsmwk)]

Gene, another beta user is Ravi, who has four crash signatures:

* nsImapServerResponseParser::msg_fetch_literal bp-7a74ef85-6565-40a7-a234-2938b0180612 C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:3120 

* nsImapProtocol::HandleMessageDownLoadLine bp-a2b84766-5de2-485b-a442-385430180612  comm/mailnews/imap/src/nsImapProtocol.cpp:3996 - Bug 1444389

*  nsImapServerResponseParser::ProcessOkCommand bp-73ef8797-7158-4b0e-8f76-ece930180606  C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapServerResponseParser.cpp:435 - Bug 628646 

*  nsImapProtocol::GetMessageSize bp-40a1298e-62a4-45a8-81ca-94d9d0180605  C:/builds/moz2_slave/tb-rel-c-beta-w32_bld-00000000/build/mailnews/imap/src/nsImapProtocol.cpp:4580 - Bug 825513 *** THIS IS THE BUG WITH THE HIGHEST CRASH RATE ***  Richard in Bug 825513 Comment 25 reports these crashes are gone (but that was two months ago).  But the signature is still a solid topcrash for 60 betas and version 52. 

Gene, which signature / bug report do you think is best to research? 
(I have noted Bug 825513 "needs deep code inspection")

Comment 58

[Richard Leger]

(In reply to Wayne Mery (:wsmwk) from comment #56)
> (In reply to Wayne Mery (:wsmwk) from comment #55)
> > 
> > Richard (our bellweather case) can you confirm whether you and your users
> > still crash, or crash less, when using beta?
> 
> That is, with the original settings which caused the crash mentioned in
> comment 26

I am still using beta (since about 3 months) and I haven't had crash related to this bug. Only one or two related to an LDAP issue reported in a separate bug report.

I have now reset the setting to their original default value in my TB. Keep you posted if any crash occurs in the future. 

I haven't hears from users in a long time but I would not be able to recall if it stopped following settings changes, TB ugrade or else... it has been too long :-) I will check on my setup and report back.

Comment 59

[Wayne Mery (:wsmwk)]

It seems like this was fixed by bug 1216951