537026 - crash [@ @0x0 | nsMsgFilterService::OpenFilterList(nsILocalFile*, nsIMsgFolder*, nsIMsgWindow*, nsIMsgFilterList**)]

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsMsgFilterService::OpenFilterList(nsILocalFile*, nsIMsgFolder*, nsIMsgWindow*, nsIMsgFilterList**)]

startup crash?

one person (this crash, windows 7) is all i find in 6 months of crashes, no email address
bp-d9db2a70-2615-471d-8fbc-587782091213
0		@0x20646c4f	
1	thunderbird.exe	nsMsgFilterService::OpenFilterList	mailnews/base/search/src/nsMsgFilterService.cpp:138
2	thunderbird.exe	nsMsgIncomingServer::GetFilterList	mailnews/base/util/nsMsgIncomingServer.cpp:1112
3	thunderbird.exe	nsMsgIncomingServer::GetEditableFilterList	mailnews/base/util/nsMsgIncomingServer.cpp:1136
4	thunderbird.exe	nsMsgDBFolder::GetEditableFilterList	mailnews/base/util/nsMsgDBFolder.cpp:4971

Comment 1

[Wayne Mery (:wsmwk)]

comment 0 stack is no longer accessible.

v3.1.3 and 3.0 have @0x0 | nsMsgFilterService::OpenFilterList(nsILocalFile*, nsIMsgFolder*, nsIMsgWindow*, nsIMsgFilterList**) - not same stack but perhaps the same issue, so updating

bp-748da73b-4655-43e2-bbc6-064bf2100912
0		@0x0	
1	thunderbird.exe	nsMsgFilterService::OpenFilterList	mailnews/base/search/src/nsMsgFilterService.cpp:138
2	thunderbird.exe	nsMsgIncomingServer::GetFilterList	mailnews/base/util/nsMsgIncomingServer.cpp:1120
3	thunderbird.exe	nsMsgDBFolder::GetFilterList	mailnews/base/util/nsMsgDBFolder.cpp:4907
4	thunderbird.exe	nsImapMailFolder::UpdateFolderWithListener	mailnews/imap/src/nsImapMailFolder.cpp:697
5	thunderbird.exe	nsImapMailFolder::UpdateFolder	mailnews/imap/src/nsImapMailFolder.cpp:678
6	thunderbird.exe	nsImapMailFolder::GetNewMessages	mailnews/imap/src/nsImapMailFolder.cpp:2619
7	thunderbird.exe	nsImapIncomingServer::PerformBiff	mailnews/imap/src/nsImapIncomingServer.cpp:989
8	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
9	thunderbird.exe	XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2722
10	thunderbird.exe	XPC_WN_CallMethod	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1740
11	js3250.dll	js_Invoke	js/src/jsinterp.cpp:1360
12	js3250.dll	js_Interpret	js/src/jsops.cpp:2240
13	js3250.dll	js_Invoke	js/src/jsinterp.cpp:1368
14	js3250.dll	js_InternalInvoke	js/src/jsinterp.cpp:1423
15	js3250.dll	JS_CallFunction	js/src/jsapi.cpp:5114
16	thunderbird.exe	nsJSContext::CallEventHandler	dom/base/nsJSEnvironment.cpp:2177

Comment 2

[Wayne Mery (:wsmwk)]

~1 crash per month

v7 bp-c3624a55-d29a-409a-b55e-8a7172111005

bug 612063 probably the same

Comment 4

[:aceman]

Attachment: patch

Comment 5

[Kent James (:rkent)]

Comment on attachment 656911 [details] [diff] [review]
patch

I don't really understand the theory of the crash you have that this change is trying to address.

First, by adding a lot of cosmetic cleanups to code at the same time you are adding some subtle fixes to a crash, it makes it much more difficult to figure out what you are really doing. I would encourage you to not add all of the cosmetic cleanups in the same patch.

The only functional change that I can see in this patch is that you are adding a check that aFilterFile->Exists fails. Do you have some theory for this crash that would be solved by this? Your check itself is fine, and is the correct thing to do, but I don't see what that has to do with the crash  - but if you have a theory please tell me.

The more substantial issue in this routine is that it leaks a filter list, because memory is allocated in:

nsMsgFilterList *filterList = new nsMsgFilterList();

but never released. I don't see how that can cause the crash either, but if you were doing code cleanup converting that to a nsRefPtr to stop the leak would be a valuable contribution.

Comment 6

[:aceman]

Sorry, I have no theory so I just patch what I can. It may not be the complete patch but we can land it and leave the bug open to watch the crashes.

In addition to the Exists check I add NS_ENSURE_ARG_POINTER(resultFilterList);

Sorry, I do not know how to use nsRefPtr.

Comment 7

[Kent James (:rkent)]

This would be a very easy place to learn to use nsRefPtr, as its use here would be confined to just a few lines of code. Have you used nsCOMPtr? nsRefPtr is virtually identical, but is used in cases where the reference is to a C++ object and not a XPCOM interface. This code was probably written before nsRefPtr existed.

The basic usage is just (from http://mxr.mozilla.org/comm-aurora/source/mailnews/addrbook/src/nsAddbookUrl.cpp#185):

nsRefPtr<nsAddbookUrl> clone = new nsAddbookUrl();

instead of

nsAddbookUrl* clone = new nsAddbookUrl();

and from then on you just treat it like a regular pointer, but without the addref/release stuff

Comment 8

[Mark Banner (:standard8)]

(In reply to Kent James (:rkent) from comment #5)
> First, by adding a lot of cosmetic cleanups to code at the same time you are
> adding some subtle fixes to a crash, it makes it much more difficult to
> figure out what you are really doing. I would encourage you to not add all
> of the cosmetic cleanups in the same patch.

Agreed, I think if we're doing cosmetic cleanups, they should be on a separate bug.

> The more substantial issue in this routine is that it leaks a filter list,
> because memory is allocated in:
> 
> nsMsgFilterList *filterList = new nsMsgFilterList();
> 
> but never released.

Err, it is managed in two places, there's one NS_RELEASE, and before that there's an assignment to the out parameter of the function, so I don't think this is leaking (although it could benefit from a ref ptr).

Comment 9

[Kent James (:rkent)]

OK you are right, I missed the *resultFilterList = filterList;   Sorry :(

Comment 10

[:aceman]

Attachment: patch v2

This seems to work now.

Comment 11

[Kent James (:rkent)]

Comment on attachment 657024 [details] [diff] [review]
patch v2

This generally looks good, but just a small issue:

   if (NS_SUCCEEDED(rv))
   {
-    *resultFilterList = filterList;
+    NS_ADDREF(*resultFilterList = filterList);
     ...
   }
   return rv;

Instead of doing an NS_ADDREF at that point, delay this until right before the "return rv", and instead do:

  filterList.swap(*resultFilterList);
  return rv;

The swap is more efficient, but more importantly IMHO I like to avoid manual addref operations whenever possible.

Comment 12

[Kent James (:rkent)]

Actually I had forgotten the discussion that the .swap was going to have issues with the conversion, sorry. The addref is OK then, but do it at the end right before the return.

Comment 13

[:aceman]

Attachment: patch v3

Comment 14

[Kent James (:rkent)]

Comment on attachment 661495 [details] [diff] [review]
patch v3

Thanks for the patch!

The work done here is really just some code cleanup. We really don't have a good theory of this rare crash, so we don't know if this patch will fix it. Still we should land this cleanup and close the bug, as if the crash occurs again it likely will have a different signature after this patch.

Comment 15

[:aceman]

I think the crashing function may be the same so the signature put into the Crash Signature field may be the same. And as the crash-stats server links bugs according to the signature field, if the crash happens again, this bug may be offered in the list. So hopefully Wayne can tell us.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/c7cb323988b1