Last Comment Bug 612069 - add null-arg checks in nsMsgSearchSession.cpp (due to crash [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)])
: add null-arg checks in nsMsgSearchSession.cpp (due to crash [@ nsMsgFilter::C...
: crash
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
-- critical (vote)
: Thunderbird 14.0
Assigned To: :aceman
Depends on:
  Show dependency treegraph
Reported: 2010-11-13 20:17 PST by Wayne Mery (:wsmwk, NI for questions)
Modified: 2012-04-03 16:33 PDT (History)
4 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

null-args and cleanup (24.00 KB, patch)
2012-03-17 05:58 PDT, :aceman
standard8: review+
Details | Diff | Splinter Review
patch v2 (22.92 KB, patch)
2012-04-03 13:44 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description User image Wayne Mery (:wsmwk, NI for questions) 2010-11-13 20:17:51 PST
crash [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)]
3 crashes in last few months. only v3.1.6. (looks like) only one person.
0	thunderbird-bin	nsMsgFilter::CreateTerm	mailnews/base/search/src/nsMsgFilter.cpp:290
1	thunderbird-bin	VirtualFolderChangeListener::Init	mailnews/base/src/nsMsgAccountManager.cpp:2622
2	thunderbird-bin	nsMsgAccountManager::AddVFListenersForVF	mailnews/base/src/nsMsgAccountManager.cpp:3267
3	thunderbird-bin	nsMsgAccountManager::LoadVirtualFolders	mailnews/base/src/nsMsgAccountManager.cpp:3088
4	libxpcom_core.dylib	libxpcom_core.dylib@0x6e120	
5	thunderbird-bin	XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2722
6	thunderbird-bin	XPC_WN_CallMethod	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1740
Comment 1 User image timeless 2010-11-15 01:25:02 PST
Crash Address	0x0

Wayne: can you try to include the crash reason+crash address when you file bugs like this? In theory it should help people recognize whether they're null pointer references (like this one) or unhappy pointer references (more work)

285 nsMsgFilter::CreateTerm(nsIMsgSearchTerm **aResult)
286 {
287    nsMsgSearchTerm *term = new nsMsgSearchTerm;
290    *aResult = static_cast<nsIMsgSearchTerm*>(term); 

caller lines:
2620   filterList->CreateFilter(NS_LITERAL_STRING("temp"), getter_AddRefs(tempFilter));
2621   NS_ENSURE_SUCCESS(rv, rv);
2622   filterList->ParseCondition(tempFilter, searchTermString.get()); 

Unfortunately, this seems like a case of pointer confusion, I can't quickly figure out how line 2622 could reach nsMsgFilter::CreateTerm
Comment 2 User image Wayne Mery (:wsmwk, NI for questions) 2012-03-15 15:31:45 PDT
one crash in 8 months and the stack looks pretty worthless, so incomplete
bp-3dbaf602-f651-4a9b-8dfe-5d95a2111221 version 8

but if someone gets bored ...
Comment 3 User image :aceman 2012-03-15 16:16:46 PDT
I guess we could at least add NS_ENSURE_ARG_POINTER(aResult); into nsMsgSearchSession::CreateTerm(nsIMsgSearchTerm **aResult) like it is now in nsMsgFilter::CreateTerm . (It wasn't there at the time of comment 1, it was added in bug 707306).

I have not yet looked into nsMsgSearchSession.cpp file for possible missing null-arg checks so this could be a good place to do it.
Comment 4 User image :aceman 2012-03-17 05:58:08 PDT
Created attachment 606860 [details] [diff] [review]
null-args and cleanup
Comment 5 User image Mark Banner (:standard8) 2012-03-29 04:31:39 PDT
Comment on attachment 606860 [details] [diff] [review]
null-args and cleanup

Review of attachment 606860 [details] [diff] [review]:

::: mailnews/base/search/src/nsMsgSearchSession.cpp
@@ +78,3 @@
>  }
> +/* [noscript] void AddSearchTerm(in nsMsgSearchAttribute attrib, in nsMsgSearchOperator op, in nsMsgSearchValue value, in boolean BooleanAND, in string customString); */

I think we can just drop these type of lines - they don't add any real value to the code.
Comment 6 User image :aceman 2012-03-29 10:04:31 PDT
Maybe the at least note which arguments are input and which output and readonly.

Should I remove them before each function?
Comment 7 User image :aceman 2012-04-03 13:44:48 PDT
Created attachment 611962 [details] [diff] [review]
patch v2

Removes comment lines. r=standard8.
Comment 8 User image Ryan VanderMeulen [:RyanVM] 2012-04-03 16:33:07 PDT

Any chance of getting a test for this?

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