Closed
Bug 537026
Opened 15 years ago
Closed 12 years ago
crash [@ @0x0 | nsMsgFilterService::OpenFilterList(nsILocalFile*, nsIMsgFolder*, nsIMsgWindow*, nsIMsgFilterList**)]
Categories
(MailNews Core :: Filters, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 18.0
People
(Reporter: wsmwk, Assigned: aceman)
References
Details
(Keywords: crash, Whiteboard: [rare])
Crash Data
Attachments
(1 file, 2 obsolete files)
3.70 KB,
patch
|
rkent
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•14 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
Summary: crash [@ nsMsgFilterService::OpenFilterList(nsILocalFile*, nsIMsgFolder*, nsIMsgWindow*, nsIMsgFilterList**)] → crash [@ @0x0 | nsMsgFilterService::OpenFilterList(nsILocalFile*, nsIMsgFolder*, nsIMsgWindow*, nsIMsgFilterList**)]
Reporter | ||
Updated•14 years ago
|
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Updated•13 years ago
|
Crash Signature: [@ @0x0 | nsMsgFilterService::OpenFilterList(nsILocalFile*, nsIMsgFolder*, nsIMsgWindow*, nsIMsgFilterList**)]
Reporter | ||
Comment 2•13 years ago
|
||
~1 crash per month v7 bp-c3624a55-d29a-409a-b55e-8a7172111005 bug 612063 probably the same
Blocks: 612063
Whiteboard: [rare]
Attachment #656911 -
Flags: review?(mbanner)
Attachment #656911 -
Flags: feedback?(kent)
Comment 5•12 years ago
|
||
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.
Attachment #656911 -
Flags: feedback?(kent) → feedback-
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•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
OK you are right, I missed the *resultFilterList = filterList; Sorry :(
Assignee | ||
Comment 10•12 years ago
|
||
This seems to work now.
Attachment #656911 -
Attachment is obsolete: true
Attachment #656911 -
Flags: review?(mbanner)
Attachment #657024 -
Flags: review?(mbanner)
Attachment #657024 -
Flags: feedback?(kent)
Comment 11•12 years ago
|
||
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.
Attachment #657024 -
Flags: feedback?(kent) → feedback+
Comment 12•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #657024 -
Attachment is obsolete: true
Attachment #657024 -
Flags: review?(mbanner)
Attachment #661495 -
Flags: review?(kent)
Comment 14•12 years ago
|
||
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.
Attachment #661495 -
Flags: review?(kent) → review+
Assignee | ||
Comment 15•12 years ago
|
||
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.
Keywords: checkin-needed
Comment 16•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/c7cb323988b1
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in
before you can comment on or make changes to this bug.
Description
•