541879 - crash [@ nsMsgDBFolder::CallFilterPlugins(nsIMsgWindow*, int*)]

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Wayne Mery (:wsmwk)]

+++ This bug was initially created as a clone of bug 472019 +++

it's unclear what impact bug bug 472019 had on crash-stats - #37 crash for 3.0.0
[@ nsMsgDBFolder::CallFilterPlugins(nsIMsgWindow*, int*)] #64 crash for 3.0.1  

bp-16971c9b-b851-4c59-b69e-ddc172100124
0	thunderbird.exe	nsMsgDBFolder::CallFilterPlugins	 mailnews/base/util/nsMsgDBFolder.cpp:2511
1	thunderbird.exe	nsMsgLocalMailFolder::UpdateFolder	mailnews/local/src/nsLocalMailFolder.cpp:630
2	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
3	thunderbird.exe	XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2456

Comment 1

[Wayne Mery (:wsmwk)]

unlike bug 472019, this crash is all address 0x0

Comment 2

[timeless]

448 nsresult nsMsgLocalMailFolder::GetDatabase()
450   nsCOMPtr <nsIMsgDatabase> msgDB;
451   return GetDatabaseWOReparse(getter_AddRefs(msgDB));

455 NS_IMETHODIMP nsMsgLocalMailFolder::GetDatabaseWOReparse(nsIMsgDatabase **aDatabase)
461   nsresult rv = NS_OK;
464     rv = OpenDatabase();
471   NS_IF_ADDREF(*aDatabase = mDatabase);
472   return rv;

So this is how we get a null mDatabase and an NS_OK from GetDatabase()

2419   if (!mDatabase)
2420   {
2421     rv = GetDatabase();
2422     NS_ENSURE_SUCCESS(rv, rv);
2423   }

This is the guard which doesn't do the job it wanted.

2506   rv = mDatabase->GetNewList(&numNewKeys, &newKeys);

is the crash point on trunk.

Comment 3

[timeless]

Attachment: proposal

Comment 4

[Wayne Mery (:wsmwk)]

after several days of 3.0.1 being out, it seems OK to claim that none of the crashes seem to involve calendar. (vs  Bug 472019)

eg bp-16971c9b-b851-4c59-b69e-ddc172100124 no extension installed

Comment 5

[David :Bienvenu]

Comment on attachment 423320 [details] [diff] [review]
proposal

even though the callers ignore the return value, I'd rather not swallow the errors here. So I'd keep the NS_ENSURE_SUCCESS(rv, rv), and return NS_ERROR_NOT_AVAILABLE when we can't get the DB...

Comment 6

[Kent James (:rkent)]

Looking at this at wsmwk's request, I did have one question. You should be able to rely on the rv for operations that initialize a pointer, otherwise we would need to check everywhere both the rv as well as the pointer, which is not really very pretty.

So don't you think that we should fix this at the upstream location nsMsgLocalMailFolder::GetDatabaseWOReparse as well? (I would still fix CallFilterPlugins since we can't be sure that we know the source of the null mDatabase).

Also, I've been using NS_ENSURE_STATE(mDatabase) in my own code for cases like this, which is a little shorter than NS_ENSURE_TRUE(mDatabase, NS_ERROR_NOT_AVAILABLE) which is what Bienvenu requested. Is there any issue with using that instead (which returns on error NS_ERROR_UNEXPECTED)?

Comment 7

[Wayne Mery (:wsmwk)]

bienvenu, see comment 6

timeless (or kent), pending bienvenu's response, can you do a revised patch?

#16 crash for v3.1.4 and ~#35 for v3.0.8, so => topcrash
(already has wanted for TB 3.1)

Comment 8

[David :Bienvenu]

Attachment: proposed fix

this is what I had in mind.

yes, we should try to figure out who's not returning an error on a null db, but it's not clear to me where the underlying problem is (e.g., is it in ::OpenDatabase, or below?)

Comment 9

[David :Bienvenu]

fixed on trunk.

Comment 10

[Mark Banner (:standard8)]

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

a=Standard8. I'll land this with some others in a few mins.

Comment 11

[Mark Banner (:standard8)]

Checked in to 1.9.2 (sorry for forgetting to set the username):

http://hg.mozilla.org/releases/comm-1.9.2/rev/b8bf5511c02a

Comment 12

[Wayne Mery (:wsmwk)]

I found only one report in getsatisfaction at http://gsfn.us/t/1g57q

Comment 13

[Wayne Mery (:wsmwk)]

posted followup qn to http://gsfn.us/t/1g57q

Comment 14

[Wayne Mery (:wsmwk)]

in http://getsatisfaction.com/mozilla_messaging/topics/message_filters-ixvhj#reply_3777630 user has a testcase of sorts to go with recent bp-43bef6d6-dba0-4ad2-a720-d809c2101021

"I still get the filter warning message every time I open one of the local folders that has new filtered messages, and have also had another crash on OKing the message. So apparently not resolved. 

Crash ID: bp-43bef6d6-dba0-4ad2-a720-d809c2101021 

Error mesasge: 
"One of your filters uses a custom header that contains an invalid character, such as ':', a non-printable character, a non-ascii character, or an eight-bit ascii character. Please edit the msgFilterRules.dat file, which contains your filters, to remove invalid characters from your custom headers." 

I've copied my msgFilterRules.dat below in case it gives any clues. 

version="9" 
logging="no" 
name="speleonics" 
enabled="yes" 
type="17" 
action="Move to folder" 
actionValue="mailbox://nobody@Local%20Folders/speleonics" 
condition="AND (all addresses,contains,speleonics)" 
name="Mathcad" 
enabled="yes" 
type="17" 
action="Move to folder" 
actionValue="mailbox://nobody@Local%20Folders/Mathcad" 
condition="AND (all addresses,contains,mathcad)" 
name="Magazine stuff" 
enabled="yes" 
type="17" 
action="Move to folder" 
actionValue="mailbox://nobody@Local%20Folders/Magazine%20renewals" 
condition="OR (from,contains,cieonline.co.uk) OR (from,contains,datateam.co.uk)" 
name="SPAM" 
enabled="yes" 
type="17" 
action="Move to folder" 
actionValue="mailbox://nobody@Local%20Folders/SPAM" 
condition="OR (subject,contains,-----SPAM-----) OR (subject,contains,***[SPAM]***)" 
name="MatlockFreeCycle" 
enabled="yes" 
type="17" 
action="Move to folder" 
actionValue="mailbox://nobody@Local%20Folders/Freecycle" 
condition="AND (all addresses,contains,MatlockFreeCycle)"

Comment 15

[David :Bienvenu]

It looks like somewhere between the check for the null database and the use of it, someone is closing the db. Reopening.

Comment 16

[Wayne Mery (:wsmwk)]

(In reply to comment #15)
> It looks like somewhere between the check for the null database and the use of
> it, someone is closing the db. Reopening.

should thunderbird-3.1 .5-fixed be reset?

Comment 17

[Mark Banner (:standard8)]

Really we should probably be filing a follow-up bug.

Comment 18

[Wayne Mery (:wsmwk)]

(In reply to comment #17)
> Really we should probably be filing a follow-up bug.

Bug 629497

Comment 19

[You can set nickname by putting [:nickname] in the field doesn't work? See Bug 1146127]

Sorry, no, I can't. This crash seems to be 'non-repeatable'.

On 4/20/2011 15:01, Wayne Mery (vn) wrote:
> we could use your help.
> do you have a way to make this crash?
>
> On 1/27/2011 3:04 PM, Wayne Mery (vn) wrote:
>> see https://bugzilla.mozilla.org/show_bug.cgi?id=541879
>>
>> https://crash-stats.mozilla.com/report/index/bc671c31-b89c-4a33-8f5c-58aeb2101231