Last Comment Bug 537026 - crash [@ @0x0 | nsMsgFilterService::OpenFilterList(nsILocalFile*, nsIMsgFolder*, nsIMsgWindow*, nsIMsgFilterList**)]
: crash [@ @0x0 | nsMsgFilterService::OpenFilterList(nsILocalFile*, nsIMsgFolde...
Status: RESOLVED FIXED
[rare]
: crash
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: 1.9.1 Branch
: x86 Windows 7
: -- critical (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
:
Mentors:
: 612063 (view as bug list)
Depends on:
Blocks: 612063
  Show dependency treegraph
 
Reported: 2009-12-28 14:50 PST by Wayne Mery (:wsmwk, NI for questions)
Modified: 2012-09-17 18:04 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.89 KB, patch)
2012-08-30 09:29 PDT, :aceman
rkent: feedback-
Details | Diff | Splinter Review
patch v2 (3.38 KB, patch)
2012-08-30 14:07 PDT, :aceman
rkent: feedback+
Details | Diff | Splinter Review
patch v3 (3.70 KB, patch)
2012-09-15 07:35 PDT, :aceman
rkent: review+
Details | Diff | Splinter Review

Description Wayne Mery (:wsmwk, NI for questions) 2009-12-28 14:50:57 PST
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, NI for questions) 2010-09-15 15:47:01 PDT
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, NI for questions) 2011-10-25 10:20:26 PDT
~1 crash per month

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

bug 612063 probably the same
Comment 3 :aceman 2012-08-30 06:44:24 PDT
*** Bug 612063 has been marked as a duplicate of this bug. ***
Comment 4 :aceman 2012-08-30 09:29:31 PDT
Created attachment 656911 [details] [diff] [review]
patch
Comment 5 Kent James (:rkent) 2012-08-30 10:12:59 PDT
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 2012-08-30 10:28:52 PDT
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) 2012-08-30 13:18:14 PDT
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, afk until Dec) 2012-08-30 13:33:04 PDT
(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) 2012-08-30 13:37:52 PDT
OK you are right, I missed the *resultFilterList = filterList;   Sorry :(
Comment 10 :aceman 2012-08-30 14:07:12 PDT
Created attachment 657024 [details] [diff] [review]
patch v2

This seems to work now.
Comment 11 Kent James (:rkent) 2012-09-14 14:06:11 PDT
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) 2012-09-14 14:18:17 PDT
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 2012-09-15 07:35:28 PDT
Created attachment 661495 [details] [diff] [review]
patch v3
Comment 14 Kent James (:rkent) 2012-09-17 09:34:12 PDT
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 2012-09-17 11:00:24 PDT
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] 2012-09-17 18:04:28 PDT
https://hg.mozilla.org/comm-central/rev/c7cb323988b1

Note You need to log in before you can comment on or make changes to this bug.