717129 - crash nsNNTPProtocol::Initialize

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[Makoto Kato [:m_kato]]

This bug was filed from the Socorro interface and is 
report bp-b037d74c-94b5-46d9-883c-c4be62120108 .
============================================================= 
0 	xul.dll 	nsNNTPProtocol::Initialize 	mailnews/news/src/nsNNTPProtocol.cpp:381
1 	xul.dll 	nsNntpIncomingServer::GetNntpConnection 	mailnews/news/src/nsNntpIncomingServer.cpp:592
2 	xul.dll 	nsNntpIncomingServer::LoadNewsUrl 	mailnews/news/src/nsNntpIncomingServer.cpp:623
3 	xul.dll 	nsNntpService::RunNewsUrl 	mailnews/news/src/nsNntpService.cpp:1195
4 	xul.dll 	nsNntpService::GetNewNews 	mailnews/news/src/nsNntpService.cpp:1226
5 	xul.dll 	nsMsgNewsFolder::GetNewsMessages 	mailnews/news/src/nsNewsFolder.cpp:923
6 	xul.dll 	nsMsgNewsFolder::GetNewMessages 	mailnews/news/src/nsNewsFolder.cpp:894
7 	xul.dll 	nsNntpIncomingServer::DownloadMail 	mailnews/news/src/nsNntpIncomingServer.cpp:705
8 	xul.dll 	nsNntpIncomingServer::PerformExpand 	mailnews/news/src/nsNntpIncomingServer.cpp:682
9 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
10 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2334
11 	xul.dll 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1629
12 	mozjs.dll 	CallCompiler::generateNativeStub 	js/src/methodjit/MonoIC.cpp:937
13 	mozjs.dll 	js::mjit::ic::NativeCall 	js/src/methodjit/MonoIC.cpp:1162
14 	mozjs.dll 	js::mjit::EnterMethodJIT 	js/src/methodjit/MethodJIT.cpp:884
15 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:962
16 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:4075
17 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:614
18 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:678
19 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:710
20 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5039
21 	xul.dll 	nsXPCWrappedJSClass::CallMethod 	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1660
22 	xul.dll 	nsXPCWrappedJS::CallMethod 	js/src/xpconnect/src/xpcwrappedjs.cpp:585
23 	xul.dll 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp:114
24 	xul.dll 	SharedStub 	xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp:141
25 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102


m_nntpServer seems to be null.  connection pool has closed connection?

Comment 1

[:aceman]

Should I add the check?

Comment 2

[Ludovic Hirlimann [:Usul]]

(In reply to :aceman from comment #1)
> Should I add the check?

go for it

Comment 3

[:aceman]

Attachment: patch

Should the function exit like this, or can the server be initialized somehow?

Comment 4

[David :Bienvenu]

Comment on attachment 592218 [details] [diff] [review]
patch

probably better to ask Joshua...

Comment 5

[:aceman]

Jcranmer, any idea?

Comment 6

[Joshua Cranmer [:jcranmer]]

Comment on attachment 592218 [details] [diff] [review]
patch

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

The problem is not the server isn't initialized (that has to happen when it's created), but that it's apparently being used after the server is dead. I'd rather see an assertion on m_nntpServer not being present and using some other error (NS_ERROR_UNEXPECTED perhaps?).

Comment 7

[:aceman]

But why would that happen? You don't know but you want to see the message in error console so that the user can report it and we can ask him what he does?

Is NS_ENSURE_TRUE an assertion? Or what exactly should I make it?

Comment 8

[Joshua Cranmer [:jcranmer]]

NS_ASSERTION is how you get an assertion (it won't fail in release builds, so you still need a check to prevent a crash).

As for how it happens, we end up a lot of time in NNTP having values which can't be null being null somewhow, so I'm going to chalk it up to the same effect.

Comment 9

[:aceman]

OK, so the only thing I have to do in the patch is just add an NS_ASSERTION line next to the NS_ENSURE_TRUE line?

Comment 10

[Joshua Cranmer [:jcranmer]]

I'm also not thrilled with the use of NS_ERROR_UNITIALIZED; another one would be better.

Comment 11

[:aceman]

Attachment: patch v2

Comment 12

[Joshua Cranmer [:jcranmer]]

Comment on attachment 628472 [details] [diff] [review]
patch v2

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

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +329,5 @@
>    }
>    nsMsgProtocol::InitFromURI(aURL);
>  
>    nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_nntpServer);
> +  NS_ASSERTION(m_nntpServer, "m_nntpServer is null!");

Change this message to "nsNNTPProtocol needs an m_nntpServer"

Comment 13

[:aceman]

Attachment: patch v3

Thanks, done.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/comm-central/rev/62b83330b36c