Last Comment Bug 717129 - crash nsNNTPProtocol::Initialize
: crash nsNNTPProtocol::Initialize
: crash
Product: MailNews Core
Classification: Components
Component: Networking: NNTP (show other bugs)
: Trunk
: x86 Windows NT
-- critical (vote)
: Thunderbird 15.0
Assigned To: :aceman
Depends on:
  Show dependency treegraph
Reported: 2012-01-10 18:22 PST by Makoto Kato [:m_kato]
Modified: 2012-06-02 11:52 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (1.99 KB, patch)
2012-01-27 12:39 PST, :aceman
Pidgeot18: review-
Details | Diff | Splinter Review
patch v2 (2.55 KB, patch)
2012-05-30 14:12 PDT, :aceman
Pidgeot18: review+
Details | Diff | Splinter Review
patch v3 (2.57 KB, patch)
2012-05-31 11:10 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description User image Makoto Kato [:m_kato] 2012-01-10 18:22:51 PST
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 User image :aceman 2012-01-25 08:40:30 PST
Should I add the check?
Comment 2 User image Ludovic Hirlimann [:Usul] 2012-01-27 04:19:52 PST
(In reply to :aceman from comment #1)
> Should I add the check?

go for it
Comment 3 User image :aceman 2012-01-27 12:39:06 PST
Created attachment 592218 [details] [diff] [review]

Should the function exit like this, or can the server be initialized somehow?
Comment 4 User image David :Bienvenu 2012-01-27 12:40:07 PST
Comment on attachment 592218 [details] [diff] [review]

probably better to ask Joshua...
Comment 5 User image :aceman 2012-03-06 04:59:41 PST
Jcranmer, any idea?
Comment 6 User image Joshua Cranmer [:jcranmer] 2012-05-30 06:35:49 PDT
Comment on attachment 592218 [details] [diff] [review]

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 User image :aceman 2012-05-30 06:57:58 PDT
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 User image Joshua Cranmer [:jcranmer] 2012-05-30 07:02:57 PDT
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 User image :aceman 2012-05-30 07:08:46 PDT
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 User image Joshua Cranmer [:jcranmer] 2012-05-30 07:21:26 PDT
I'm also not thrilled with the use of NS_ERROR_UNITIALIZED; another one would be better.
Comment 11 User image :aceman 2012-05-30 14:12:02 PDT
Created attachment 628472 [details] [diff] [review]
patch v2
Comment 12 User image Joshua Cranmer [:jcranmer] 2012-05-31 06:09:06 PDT
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 User image :aceman 2012-05-31 11:10:26 PDT
Created attachment 628819 [details] [diff] [review]
patch v3

Thanks, done.
Comment 14 User image Mike Conley (:mconley) 2012-06-02 11:52:58 PDT

Note You need to log in before you can comment on or make changes to this bug.