481866 - crash [@ nsMsgDatabase::GetUint32Property(nsIMdbRow*, char const*, unsigned int*, unsigned int)] - [@ nsMsgHdr::GetUint32Property] emptying trash

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsMsgDatabase::GetUint32Property - nsMsgHdr::GetUint32Property] emptying trash

I had done alt,f,y to empty trash
then alt,f,f to compress folders
crash
upon restart, much or all of same mail is still in trash

3.0b3pre 20090304031214

bp-90091c3a-c846-499a-a680-94d722090306
nsMsgDatabase::GetUint32Property	 mailnews/db/msgdb/src/nsMsgDatabase.cpp:3486
nsMsgHdr::GetUint32Property	mailnews/db/msgdb/src/nsMsgHdr.cpp:308
NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2265
XPC_WN_CallMethod	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1587
js_Invoke	js/src/jsinterp.cpp:1313

Comment 1

[Wayne Mery (:wsmwk)]

attempted to reproduce. no crash. no problem - trash is gone

Comment 2

[Kent James (:rkent)]

Just from your description, I thought to myself "probably a null m_mdbStore" and sure enough, looking at the line in question that's what it is.

I really think that we should add a null check to all of the database operations for m_mdbStore. Ultimately we would like to be able to have the database object exist but not be open - which means null m_mdbStore - so I think that would be the correct long-term decision as well.

Comment 3

[David :Bienvenu]

Attachment: proposed fix

If the db is closed, the outstanding msg hdrs will have their mdb rows cleared - if code (especially js code) doesn't pay attention to the db closed notification, and tries to use the msg hdr, we can crash. This should fix the common cases of this crash. I'd like to do a unit test for this, and add more bulletproofing, but this should fix the existing crash.

Comment 4

[David :Bienvenu]

let's put this in b3 - an other way of encountering this is to compaction fire, and close the db for an open folder, or have some js code holding onto headers for the folder that's compacted.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 385654 [details] [diff] [review]
proposed fix

> NS_IMETHODIMP nsMsgHdr::GetProperty(const char *propertyName, nsAString &resultProperty)
> {
>   NS_ENSURE_ARG_POINTER(propertyName);
>+  if (!m_mdb || !m_mdbRow)
>+    return NS_ERROR_NULL_POINTER;
>   return m_mdb->GetPropertyAsNSString(GetMDBRow(), propertyName, resultProperty);

I think we should just use m_mdbRow/GetMDBRow() consistently in these functions - I'd prefer just using m_mdbRow. r/sr=Standard8 with those fixed.

Comment 6

[Wayne Mery (:wsmwk)]

fix signature for crash-stats

Comment 7

[Wayne Mery (:wsmwk)]

apparently landed on 2009-06-30. last 3.0b3pre crash in past 3 weeks is 20090627 - so I'd say this is gone. but 3.0b3 crash-stats should be reviewed to verify.

currently a top 20 crash for 3.0b2

Comment 8

[David :Bienvenu]

yes, this was checked in...