617740 - crash @ nsPop3Sink::IncorporateBegin. prefs.js is invalid or broken?

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsPop3Sink::IncorporateBegin(char const*, nsIURI*, unsigned int, void**)]

bp-cb373b9b-50c5-4a9b-8ee4-966282101205
EXCEPTION_ACCESS_VIOLATION_READ
0x0
0	thunderbird.exe	nsPop3Sink::IncorporateBegin	mailnews/local/src/nsPop3Sink.cpp:592
1	thunderbird.exe	nsPop3Protocol::RetrResponse	mailnews/local/src/nsPop3Protocol.cpp:3304
2	thunderbird.exe	nsPop3Protocol::ProcessProtocolState	mailnews/local/src/nsPop3Protocol.cpp:3954
3	thunderbird.exe	nsMsgProtocol::OnDataAvailable	mailnews/base/util/nsMsgProtocol.cpp:359
4	thunderbird.exe	nsInputStreamPump::OnStateTransfer	netwerk/base/src/nsInputStreamPump.cpp:510
5	thunderbird.exe	nsInputStreamPump::OnInputStreamReady	netwerk/base/src/nsInputStreamPump.cpp:400
6	xpcom_core.dll	nsOutputStreamReadyEvent::Run	xpcom/io/nsStreamUtils.cpp:112
7	xpcom_core.dll	nsThread::ProcessNextEvent	xpcom/threads/nsThread.cpp:527
8	xpcom_core.dll	NS_ProcessNextEvent_P	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:250
9	thunderbird.exe	nsXULWindow::ShowModal	xpfe/appshell/src/nsXULWindow.cpp:416
10	thunderbird.exe	nsContentTreeOwner::ShowAsModal	xpfe/appshell/src/nsContentTreeOwner.cpp:528
11	thunderbird.exe	nsWindowWatcher::OpenWindowJSInternal	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:1010
12	thunderbird.exe	nsWindowWatcher::OpenWindow	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:425
13	thunderbird.exe	nsPromptService::DoDialog	embedding/components/windowwatcher/src/nsPromptService.cpp:795
14	thunderbird.exe	nsPromptService::ConfirmEx	embedding/components/windowwatcher/src/nsPromptService.cpp:402

Comment 1

[timeless]

m_outFileStream is null

Comment 2

[Wayne Mery (:wsmwk)]

m_kato, is this easily sorted out?

(In reply to timeless from comment #1)
> m_outFileStream is null


7 of 10 crashes with signature have nsPop3Protocol::ProcessProtocolStat on stack.  example of current stack ...

bp-420d6ece-1f17-4625-b07c-6fd302140901
0 	xul.dll	nsPop3Sink::IncorporateBegin(char const*, nsIURI*, unsigned int, void**)	mailnews/local/src/nsPop3Sink.cpp
1 	xul.dll	nsPop3Protocol::RetrResponse(nsIInputStream*, unsigned int)	mailnews/local/src/nsPop3Protocol.cpp
2 	xul.dll	nsPop3Protocol::ProcessProtocolState(nsIURI*, nsIInputStream*, unsigned __int64, unsigned int)	mailnews/local/src/nsPop3Protocol.cpp
3 	xul.dll	nsMsgProtocol::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned __int64, unsigned int)	mailnews/base/util/nsMsgProtocol.cpp
4 	xul.dll	nsInputStreamPump::OnStateTransfer()	netwerk/base/src/nsInputStreamPump.cpp
5 	xul.dll	nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)	netwerk/base/src/nsInputStreamPump.cpp

Comment 3

[Makoto Kato [:m_kato]]

(In reply to Wayne Mery (:wsmwk) from comment #2)
> m_kato, is this easily sorted out?

This issue isn't same.  This signature is because server is null (we should check error code of m_folder->GetServer()).

I doesn't know why this returns error.  But I think user configuration / prefs.js is invalid or broken?

Comment 4

[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 5

[Wayne Mery (:wsmwk)]

This is the only nsPop3 crash bug we have open. 

Any obvious cause in the stack?

bp-568fd845-f5dc-4ac7-b89d-585460181114
 0 	xul.dll	nsPop3Sink::IncorporateBegin(char const*, nsIURI*, unsigned int, void**)	comm/mailnews/local/src/nsPop3Sink.cpp:453
1 	xul.dll	nsPop3Protocol::RetrResponse(nsIInputStream*, unsigned int)	comm/mailnews/local/src/nsPop3Protocol.cpp:3340
2 	xul.dll	nsPop3Protocol::ProcessProtocolState(nsIURI*, nsIInputStream*, unsigned __int64, unsigned int)	comm/mailnews/local/src/nsPop3Protocol.cpp:4001
3 	xul.dll	nsMsgProtocol::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned __int64, unsigned int)	comm/mailnews/base/util/nsMsgProtocol.cpp:301
4 	xul.dll	nsInputStreamPump::OnStateTransfer()	netwerk/base/nsInputStreamPump.cpp:597
5 	xul.dll	nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)	netwerk/base/nsInputStreamPump.cpp:432

Comment 6

[ISHIKAWA, Chiaki]

I don't see any obvious cause for this right now. (edited for relevance)

Comment 7

[Wayne Mery (:wsmwk)]

At 1-2 crashes per week, this doesn't seem worthy of much attention.

This is the only nsPop3 crash bug we have open.

And also virtually the only pop signature in existence

Comment 8

[Wayne Mery (:wsmwk)]

In the last 6 months there are only 4 version 78 crashes - almost nothing compared to version 60 and 68 crashes.
bp-c33625f3-7bed-49ed-a200-baa3a0201030 is one, for 78.4.0

Comment 9

[Wayne Mery (:wsmwk)]

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

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

m_kato, is this easily sorted out?

This issue isn't same. This signature is because server is null (we should
check error code of m_folder->GetServer()).

I doesn't know why this returns error. But I think user configuration / prefs.js is invalid or broken?

Patchable? bp-b893d482-37c8-481e-b3f9-212400211027 91.2.1 is a recent example

If not, then we can close this.

Comment 10

[Magnus Melin [:mkmelin]]

Crashes at null server https://hg.mozilla.org/releases/comm-esr91/file/tip/mailnews/local/src/nsPop3Sink.cpp#l393 but this has since been reworked/removed in https://hg.mozilla.org/comm-central/rev/35e064ada8debd021ce507fd0e8b20be363a9046.
We could fix it for 91 I guess.

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: bug617740_crash@incorporatebegin_esr91.patch

Null check server, for 91 only.

Comment 12

[Ben Campbell]

Comment on attachment 9248626 [details] [diff] [review]
bug617740_crash@incorporatebegin_esr91.patch">bug617740_crash@incorporatebegin_esr91.patch

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

r+ and it might prevent a few people experiencing crashes!

But really, something is seriously wrong if the `.server` attr is null. It's a read-only attribute and should be set during creation of the folder and unchanging throughout it's lifetime.
The root folder is created by the server itself , and child folders are created by their parent. So there's never a case where we don't know what the server is. But folder discovery/creation is fantastically convoluted at the moment (hangover from folders-as-RDF-resource days), so I wouldn't be at all surprised to find code paths that leave folders in odd states like that...  There are plans afoot to clean up and simplify it all (see Bug 1679333 and see-alsos).

Eventually I'd like to see a hard MOZ_ASSERT() inside GetServer() instead :-)

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9248626 [details] [diff] [review]
bug617740_crash@incorporatebegin_esr91.patch

[Approval Request Comment]
Safe crash fix just adding a null check for rare crash. 91 only since the code has since been reworked.

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9248626 [details] [diff] [review]
bug617740_crash@incorporatebegin_esr91.patch

[Triage Comment]
approved for esr91

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 91.3.1:
https://hg.mozilla.org/releases/comm-esr91/rev/53076449f953