847356 - crash in PR_EnterMonitor via nsImapProtocol::GetPseudoInterrupted | nsIMAPBodypartMultipart::Generate. m_protocolConnection of nsIMAPBodyShell is invalid/destroyed use after free

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

not a new crash

This bug was filed from the Socorro interface and is 
report bp-9d4152d9-b35b-4538-8bc9-cbe212121129 .
============================================================= 

0	nspr4.dll	PR_EnterMonitor	nsprpub/pr/src/threads/prmon.c:64
1	xul.dll	nsImapProtocol::GetPseudoInterrupted	mailnews/imap/src/nsImapProtocol.cpp:4584
2	xul.dll	nsIMAPBodypartMultipart::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:1015
3	xul.dll	nsIMAPBodypartMessage::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:867
4	xul.dll	nsIMAPBodyShell::Generate	mailnews/imap/src/nsIMAPBodyShell.cpp:265
5	xul.dll	nsImapProtocol::ProcessSelectedStateURL	mailnews/imap/src/nsImapProtocol.cpp:2636
6	xul.dll	nsImapProtocol::ProcessCurrentURL	mailnews/imap/src/nsImapProtocol.cpp:1717
7	xul.dll	nsImapProtocol::ImapThreadMainLoop	mailnews/imap/src/nsImapProtocol.cpp:1338
8	xul.dll	nsImapProtocol::Run	mailnews/imap/src/nsImapProtocol.cpp:1016 


is bug 632011 comment 6 relevant?

Comment 2

[Wayne Mery (:wsmwk)]

Definitely still here in version 60 bp-ca9f728c-df58-4b12-a28b-8dadf0181002 and nightly bp-6a6877a4-ff71-4220-b975-f0d190180524  TB62.0a1 - but rash rate over past 3 months seems to have dropped by 30-40%

0	ntdll.dll	RtlEnterCriticalSection	
1	nss3.dll	PR_Lock	nsprpub/pr/src/threads/combined/prulock.c:213
2	nss3.dll	PR_EnterMonitor	nsprpub/pr/src/threads/prmon.c:131
3	xul.dll	nsImapProtocol::GetPseudoInterrupted()	comm/mailnews/imap/src/nsImapProtocol.cpp:4686
4	xul.dll	nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, bool, bool)	comm/mailnews/imap/src/nsIMAPBodyShell.cpp:977
5	xul.dll	nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, bool, bool)	comm/mailnews/imap/src/nsIMAPBodyShell.cpp:980
6	xul.dll	nsIMAPBodypartMultipart::Generate(nsIMAPBodyShell*, bool, bool)	comm/mailnews/imap/src/nsIMAPBodyShell.cpp:980
7	xul.dll	nsIMAPBodypartMessage::Generate(nsIMAPBodyShell*, bool, bool)	comm/mailnews/imap/src/nsIMAPBodyShell.cpp:837
8	xul.dll	nsIMAPBodyShell::Generate(char*)	comm/mailnews/imap/src/nsIMAPBodyShell.cpp:258
9	xul.dll	nsImapProtocol::ProcessSelectedStateURL()	comm/mailnews/imap/src/nsImapProtocol.cpp:2755
10	xul.dll	nsImapProtocol::ProcessCurrentURL()	comm/mailnews/imap/src/nsImapProtocol.cpp:1782


Mac signature pthread_mutex_lock | PR_EnterMonitor | nsImapProtocol::GetPseudoInterrupted
bp-86af9e65-be15-4eba-9354-39eec0180926

Comment 3

[Makoto Kato [:m_kato]]

I guess that m_protocolConnection of nsIMAPBodyShell is invalid.  Although nsIMAPBodyShell will be got from cache (via FindShellInCacheForHost), m_protocolConnection of nsIMAPBodyShell doesn't use strong reference.  So nsIMAPBodyShell doens't know that m_protocolConnection is destroyed.

Comment 4

[Makoto Kato [:m_kato]]

related to bug 825513 (this might be same root cause of bug 825513)

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to Makoto Kato [:m_kato] from comment #4)

related to bug 825513 (this might be same root cause of bug 825513)

That bug, and followup bug 1507718 have patches landed several months ago, which had some impact on crash rate but their signatures still continue, so bug 1565517 has been filed.

This signature continues to be very strong in current versions - #16 in beta and #24 for 60.7.2
https://crash-stats.mozilla.org/signature/?signature=RtlEnterCriticalSection%20%7C%20PR_Lock%20%7C%20PR_EnterMonitor%20%7C%20nsImapProtocol%3A%3AGetPseudoInterrupted&date=>%3D2019-01-12T11%3A01%3A00.000Z&date=<2019-07-12T11%3A01%3A00.000Z#graphs

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Wayne Mery (:wsmwk) from comment #5)

That bug, and followup bug 1507718 have patches landed several months ago, which had some impact on crash rate but their signatures still continue, so bug 1565517 has been filed.

This signature continues to be very strong in current versions - #16 in beta and #24 for 60.7.2
https://crash-stats.mozilla.org/signature/?signature=RtlEnterCriticalSection%20%7C%20PR_Lock%20%7C%20PR_EnterMonitor%20%7C%20nsImapProtocol%3A%3AGetPseudoInterrupted&date=>%3D2019-01-12T11%3A01%3A00.000Z&date=<2019-07-12T11%3A01%3A00.000Z#graphs

I looked the latest crashes such as bp-7b87c963-cb2c-4b9f-8a48-159610190709. I guess as first impression, m_protocolConnection isn't holded by reference count, so nsIMAPBodyShell doesn't know whether it is still alive.

Comment 7

[Wayne Mery (:wsmwk)]

Signature changed In early November
from
RtlEnterCriticalSection | PR_Lock | PR_EnterMonitor | nsImapProtocol::GetPseudoInterrupted for version 68.x
to
RtlEnterCriticalSection | PR_EnterMonitor | nsImapProtocol::GetPseudoInterrupted [1] for version 78.x

But at a much lower rate in version 78, 30/day compared to 180/day. So something got better in the imap world.

[1] https://crash-stats.mozilla.org/signature/?signature=RtlEnterCriticalSection%20%7C%20PR_EnterMonitor%20%7C%20nsImapProtocol%3A%3AGetPseudoInterrupted&date=%3E%3D2020-07-01T01%3A34%3A00.000Z&date=%3C2021-01-01T01%3A34%3A00.000Z&_sort=-date#graphs

Comment 8

[Wayne Mery (:wsmwk)]

#100 crash for 78.14.0, #144 for 91.2.1 - so similar crash rate

see comment 6
bp-27e2ead6-48df-441f-9f50-c5ea70211031
0 ntdll.dll RtlEnterCriticalSection
1 nss3.dll PR_EnterMonitor(PRMonitor*) nsprpub/pr/src/threads/prmon.c:139
2 xul.dll nsImapProtocol::GetPseudoInterrupted() comm/mailnews/imap/src/nsImapProtocol.cpp:4725
3 xul.dll nsImapBodyShell::Generate(char*) comm/mailnews/imap/src/nsImapBodyShell.cpp:197
4 xul.dll nsImapServerResponseParser::ProcessOkCommand(char const*) comm/mailnews/imap/src/nsImapServerResponseParser.cpp:406
5 xul.dll nsImapServerResponseParser::ParseIMAPServerResponse(char const*, bool, char*) comm/mailnews/imap/src/nsImapServerResponseParser.cpp:264
6 xul.dll nsImapProtocol::ParseIMAPandCheckForNewMail(char const*, bool) comm/mailnews/imap/src/nsImapProtocol.cpp:2031
7 xul.dll nsImapProtocol::FetchMessage(nsTString<char> const&, <unnamed-tag>, char const*, unsigned int, unsigned int, char*) comm/mailnews/imap/src/nsImapProtocol.cpp:3715
8 xul.dll nsImapProtocol::FetchTryChunking(nsTString<char> const&, <unnamed-tag>, bool, char*, unsigned int, bool) comm/mailnews/imap/src/nsImapProtocol.cpp:3765
9 xul.dll nsIMAPBodypart::GeneratePart(nsImapBodyShell*, bool, bool) comm/mailnews/imap/src/nsImapBodyShell.cpp:409

bp-5eb60ce8-dcf6-4cc8-8db2-48bc80211027

Comment 9

[Magnus Melin [:mkmelin]]

Maybe we should make m_protocolConnection a RefPtr and see if it helps?

Comment 10

[Ben Campbell]

(In reply to Magnus Melin [:mkmelin] from comment #9)

Maybe we should make m_protocolConnection a RefPtr and see if it helps?

A disappearing connection definitely seems to be the most likely cause of the crash. I'm a little hesitant to just blindly make m_protocolConnection a RefPtr though... there's some caching of nsImapBodyShell objects (in nsImapHostSessionList), and I'm worried it'd keep connections alive past their natural lifespan. Maybe a weak ref instead? Anyway, I'll investigate and figure something out.

Comment 11

[Ben Campbell]

Attachment: Bug 847356 - Ensure nsImapProtocol is kept in existence for duration of nsImapBodyShell::Generate(). r=mkmelin

Additionally, this changes nsImapBodyShell to no longer retain nsImapProtocol
as a member variable. nsImapBodyShells are cached, so it seems irresponsible
for them to hold a raw pointer to the connection. Instead, the connection is
passed in as an extra parameter to nsImapBodyShell::Generate().
There's also a little light tidying and cruft removal.

Comment 12

[Ben Campbell]

Not 100% sure this patch will fix it, but I think it's an improvement in any case.

xpcshell unit tests seem to run fine for me locally, and there's a try build here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f193e2f5626c4c4905a94a2e442e62c033e3d252

Comment 13

[Wayne Mery (:wsmwk)]

Richard, perhaps you want to try this? https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/UlAJoxyuTnSlGh2FlFZ7tw/runs/0/artifacts/public/build/install/sea/target.installer.exe

Comment 14

[Wayne Mery (:wsmwk)]

Ben, checkin now will cause this to immediately hit beta which is probably not a good thing, because beta merge is tomorrow. So I hope it's OK I've removed checkin-needed

Comment 15

[Ben Campbell]

Oops! No rush on this. Definitely be nice to have the tires kicked on this a bit more first!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d352bc331cec
Ensure nsImapProtocol is kept in existence for duration of nsImapBodyShell::Generate(). r=mkmelin

Comment 17

[Richard Leger]

(In reply to Wayne Mery (:wsmwk) from comment #13)

Richard, perhaps you want to try this? https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/UlAJoxyuTnSlGh2FlFZ7tw/runs/0/artifacts/public/build/install/sea/target.installer.exe

Has it landed in any beta yet? E.g 97.0b3?

Comment 18

[Wayne Mery (:wsmwk)]

(In reply to Richard Leger from comment #17)

(In reply to Wayne Mery (:wsmwk) from comment #13)

Richard, perhaps you want to try this? https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/UlAJoxyuTnSlGh2FlFZ7tw/runs/0/artifacts/public/build/install/sea/target.installer.exe

Has it landed in any beta yet? E.g 97.0b3?

No. I think at this point we would just let it "ride the train" to the next beta version 98, Approx Feb 7.

Comment 19

[Wayne Mery (:wsmwk)]

Richard, 98 beta is available.

Comment 20

[Richard Leger]

(In reply to Wayne Mery (:wsmwk) from comment #19)

Richard, 98 beta is available.

I have been on 98 beta for a couple of days now and I have not encountered major issues with IMAP so far...
Will keep you posted if any.

Only that crash upon closing TB which is likely unrelated to IMAP:
bp-8819ff0b-c658-4c2d-a0fb-976721220302 11 hours ago
bp-d4f589cc-1596-40f3-bebd-fd2af1220302 16 hours ago