Closed Bug 612069 Opened 11 years ago Closed 10 years ago

add null-arg checks in nsMsgSearchSession.cpp (due to crash [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)])

Categories

(MailNews Core :: Filters, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 14.0

People

(Reporter: wsmwk, Assigned: aceman)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
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
Crash Signature: [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)]
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 ...
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INCOMPLETE
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.
Assignee: nobody → acelists
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Status: REOPENED → ASSIGNED
Summary: crash [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)] → add null-arg checks in nsMsgSearchSession.cpp (due to crash [@ nsMsgFilter::CreateTerm(nsIMsgSearchTerm**)])
Attached patch null-args and cleanup (obsolete) — Splinter Review
Attachment #606860 - Flags: review?(mbanner)
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.
Attachment #606860 - Flags: review?(mbanner) → review+
Maybe the at least note which arguments are input and which output and readonly.

Should I remove them before each function?
OS: Mac OS X → All
Hardware: x86 → All
Attached patch patch v2Splinter Review
Removes comment lines. r=standard8.
Attachment #606860 - Attachment is obsolete: true
Attachment #611962 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/54480c6bb1f8

Any chance of getting a test for this?
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
You need to log in before you can comment on or make changes to this bug.