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...
Status: RESOLVED FIXED
: crash
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- critical (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks:
  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: ---


Attachments
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 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.
bp-a9579808-8656-4dc4-b2d9-808d02101111
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 timeless 2010-11-15 01:25:02 PST
Crash Reason	EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE
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;
288    NS_ENSURE_TRUE(term, NS_ERROR_OUT_OF_MEMORY);
289
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 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 :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 :aceman 2012-03-17 05:58:08 PDT
Created attachment 606860 [details] [diff] [review]
null-args and cleanup
Comment 5 Mark Banner (:standard8, afk until Dec) 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 :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 :aceman 2012-04-03 13:44:48 PDT
Created attachment 611962 [details] [diff] [review]
patch v2

Removes comment lines. r=standard8.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-04-03 16:33:07 PDT
http://hg.mozilla.org/comm-central/rev/54480c6bb1f8

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.