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

RESOLVED DUPLICATE of bug 1216951

Status

defect
--
critical
RESOLVED DUPLICATE of bug 1216951
3 years ago
10 days ago

People

(Reporter: richard.leger, Unassigned)

Tracking

(Depends on 1 bug, Blocks 6 bugs, {crash, steps-wanted})

38 Branch
x86_64
Windows NT
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [need testing with fetch_by_chunks disabled], crash signature, )

User Story

(from bug 1294082)

user environment...
Win7 Pro 64bits
imap autosync DISALBED
roaming profile 
local network, IMAP server Cyrus 
Microsoft Security Essentials antivirus

Attachments

(1 attachment)

Reporter

Description

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

Updated

3 years ago
Version: unspecified → 38 Branch
Reporter

Comment 1

3 years ago
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
Reporter

Comment 2

3 years ago
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.
Reporter

Comment 3

3 years ago
It happens repeatedly since last week about 5 times per week.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 5

3 years ago
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.
(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
Flags: needinfo?(richard.leger)
Reporter

Comment 7

3 years ago
(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...
(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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(richard.leger)
Resolution: --- → WORKSFORME
Reporter

Comment 9

3 years ago
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?
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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

Updated

3 years ago
See Also: → 1294082
> 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/
Reporter

Comment 12

3 years ago
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...
Reporter

Comment 13

3 years ago
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...
Reporter

Comment 14

3 years ago
> 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...
Reporter

Comment 15

3 years ago
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
> 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?
Keywords: steps-wanted
Summary: Crash in nsImapServerResponseParser::msg_fetch_literal → Random crash in nsImapServerResponseParser::msg_fetch_literal

Updated

3 years ago
See Also: → 1323579
Reporter

Comment 17

3 years ago
(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
(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)
URL: 825513
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');

Updated

2 years ago
User Story: (updated)

Updated

2 years ago
Blocks: 1333031
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);
Reporter

Comment 21

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

Updated

2 years ago
Depends on: 1284302
(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?
Flags: needinfo?(richard.leger)
Reporter

Comment 23

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

Updated

2 years ago
Blocks: 1323596

Updated

2 years ago
Blocks: 1333027, 1333037

Updated

2 years ago
Blocks: 1333040, 1333032

Updated

2 years ago
Whiteboard: [need testing with fetch_by_chunks disabled]

Updated

2 years ago
Blocks: 1323579
See Also: 1323579
Reporter

Comment 24

2 years ago
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.
Flags: needinfo?(richard.leger)
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
Summary: Random crash in nsImapServerResponseParser::msg_fetch_literal → Random crash in nsImapServerResponseParser::msg_fetch_literal. Related to fetch_by_chunks?
Reporter

Comment 26

2 years ago
(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 :-)
Reporter

Comment 27

2 years ago
Just to confirm, it seems that Thunderbird has not crashed since the options above were implemented. Current user TB version is 45.8.0.
What can you make of comment 19?
Or, is bug 1222455 a better case to look at?
Blocks: 1222455
Flags: needinfo?(m_kato)
Keywords: crash

Comment 29

2 years ago
(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.
(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.
Flags: needinfo?(m_kato)
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
Blocks: 1333038
Crash Signature: [@ nsImapServerResponseParser::msg_fetch_literal] → [@ nsImapServerResponseParser::msg_fetch_literal] [@ _invalid_parameter ]
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?

Updated

2 years ago
Blocks: 1333342

Comment 33

2 years ago
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?
No longer blocks: 1333342
Flags: needinfo?(m_kato)

Updated

2 years ago
Blocks: 1333342

Comment 34

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

2 years ago
Oh, good catch! Thanks.

So check fCurrentLine before line 3115, and if null, do what? Continue, break, error?
Flags: needinfo?(m_kato)
(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?
Flags: needinfo?(m_kato)
Flags: needinfo?(ishikawa)
(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
Flags: needinfo?(ishikawa)
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.
Flags: needinfo?(m_kato)

Comment 39

a year ago
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 ;-)
Flags: needinfo?(gds)

Comment 40

a year ago
Thanks, will watch for problems here. (NI still on.)
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
Crash Signature: [@ nsImapServerResponseParser::msg_fetch_literal] [@ _invalid_parameter ] → [@ nsImapServerResponseParser::msg_fetch_literal] [@ _invalid_parameter ] [@ strlen | nsImapServerResponseParser::msg_fetch_literal ]
Depends on: 1216951

Comment 42

a year ago
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

a year ago
(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.
Flags: needinfo?(gds)
Blocks: 1444201
I don't know if it's feasible, but it would be
Blocks: 1294074
(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/
Flags: needinfo?(richard.leger)
Reporter

Comment 46

a year ago
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...
Flags: needinfo?(richard.leger)

Comment 47

a year ago
(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!
Blocks: 1294082
See Also: → 825513
Duplicate of this bug: 645586
(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
Flags: needinfo?(richard.leger)

Comment 51

a year ago
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.
Reporter

Comment 52

a year ago
(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.
Flags: needinfo?(richard.leger)
Reporter

Comment 53

a year ago
(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?
(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

11 months ago
(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?
Flags: needinfo?(richard.leger)
Summary: Random crash in nsImapServerResponseParser::msg_fetch_literal. Related to fetch_by_chunks? → Random crash getting new email in nsImapServerResponseParser::msg_fetch_literal. Made worse by, but not caused by, fetch_by_chunks

Comment 56

11 months ago
(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

11 months ago
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")
Flags: needinfo?(gds)
Reporter

Comment 58

11 months ago
(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.
Flags: needinfo?(richard.leger)

Comment 59

10 months ago
It seems like this was fixed by bug 1216951
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago10 months ago
Flags: needinfo?(gds)
Resolution: --- → DUPLICATE
Duplicate of bug: 1216951

Updated

10 days ago
Depends on: 1494764
You need to log in before you can comment on or make changes to this bug.