crash [@ nsMsgFilterService::BackUpFilterFile(nsILocalFile*, nsIMsgWindow*)]



MailNews Core
7 years ago
a year ago


(Reporter: wsmwk, Unassigned)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: [rare], crash signature)


(1 attachment)



7 years ago
crash [@ nsMsgFilterService::BackUpFilterFile(nsILocalFile*, nsIMsgWindow*)]
Frame	Module	Signature [Expand]	Source
0	thunderbird-bin	nsMsgFilterService::BackUpFilterFile	mailnews/base/search/src/nsMsgFilterService.cpp:195
1	thunderbird-bin	nsMsgFilterService::OpenFilterList	mailnews/base/search/src/nsMsgFilterService.cpp:138
2	thunderbird-bin	nsMsgIncomingServer::GetFilterList	mailnews/base/util/nsMsgIncomingServer.cpp:1120
3	thunderbird-bin	nsMsgDBFolder::GetFilterList	mailnews/base/util/nsMsgDBFolder.cpp:4908
4	thunderbird-bin	nsImapMailFolder::UpdateFolderWithListener	mailnews/imap/src/nsImapMailFolder.cpp:697
5	thunderbird-bin	nsImapMailFolder::UpdateFolder	mailnews/imap/src/nsImapMailFolder.cpp:678
6	thunderbird-bin	nsImapMailFolder::GetNewMessages	mailnews/imap/src/nsImapMailFolder.cpp:2619


7 years ago
Crash Signature: [@ nsMsgFilterService::BackUpFilterFile(nsILocalFile*, nsIMsgWindow*)]

Comment 1

6 years ago
The crash is no longer accessible?
Is it your crash? Can you reproduce it?

Comment 2

6 years ago
May be related to bug 537026 and bug 612063. The line nsMsgFilterService::OpenFilterList	mailnews/base/search/src/nsMsgFilterService.cpp:138 is also there. May be a problem in the BackUpFilterFile function, in all three bugs.

Comment 3

6 years ago
(In reply to :aceman from comment #1)
> The crash is no longer accessible?
apparently yes.

> Is it your crash? Can you reproduce it?

not my crash - mine almost always have my email address and/or comment.

it is a rare crash, but still exists. here are two recents with comments

(In reply to :aceman from comment #2)
> May be related to bug 537026 and bug 612063. The line
> nsMsgFilterService::OpenFilterList
> mailnews/base/search/src/nsMsgFilterService.cpp:138 is also there. May be a
> problem in the BackUpFilterFile function, in all three bugs.

indeed - pick one and stab it inthe heart! :)

Comment 4

6 years ago
I could probably add some null checks at least, but how to test if it helps? :)

Comment 5

5 years ago
The reports still continue (like 6af35d1b-2943-4a50-b07e-3a87a2120829) so let's try some patch.
Assignee: nobody → acelists

Comment 6

5 years ago

Comment 7

5 years ago
Created attachment 656910 [details] [diff] [review]
Attachment #656910 - Flags: review?(mbanner)
Attachment #656910 - Flags: feedback?(kent)


5 years ago

Comment 8

5 years ago
Comment on attachment 656910 [details] [diff] [review]

In general, since my feedback has been requested, let me give the feedback that I do not agree that it is worthwhile to go to existing code and make cosmetic changes simply to bring the code up to current standards. It makes work for everyone, introduces the possibility of errors, plus it complicates the code history when you are tracing back problems. So I would not normally approve such changes, unless they are in the context of an extensive rework of existing code. But my opinions may not reflect standard policy.

Back to the crash itself, the only substantial change here is the error check:


That is a correct thing to do, and is a non-cosmetic change that I would typically approve. But as in my comments on the patch for bug 537026, I cannot see a theory of the crash that this would solve. aFilterFile has already been confirmed as non-null in the parent to this call. AFAIK, this type of pointer does not suddenly get reset to null (unlike the message summary database pointer, which is a major design flaw in our code). Still, this crash is occurring on that exact line that would hint at a null aFilterFile, and this change would not hurt.

So what I would approve of is a single line change to add this check, but I do not expect it will solve the crash. I agree with earlier comments that the root cause if probably the same here as in bug 537026, but I have no theories what that root cause might be.
Attachment #656910 - Flags: feedback?(kent) → feedback-
Comment on attachment 656910 [details] [diff] [review]

I think I'm with Kent, the NS_ENSURE_ARG_POINTER isn't really necessary as the caller has already used this object. The only thing I can think is that something is slightly messed somewhere, as I can't see anything wrong with the way aFilterFile is used prior to being passed to this function. I'm pretty sure this wouldn't fix the crash though.

The other changes, I'd perhaps accept if we were doing stuff in the file or cleanup or something, but not on their own.
Attachment #656910 - Flags: review?(mbanner) → review-

Comment 10

5 years ago
Thanks, no problem.
Assignee: acelists → nobody

Comment 11

5 years ago
signature has changed to nsMsgFilterService::BackUpFilterFile(nsIFile*, nsIMsgWindow*)
Crash Signature: [@ nsMsgFilterService::BackUpFilterFile(nsILocalFile*, nsIMsgWindow*)] → [@ nsMsgFilterService::BackUpFilterFile(nsILocalFile*, nsIMsgWindow*)] [@ nsMsgFilterService::BackUpFilterFile(nsIFile*, nsIMsgWindow*) ] [@ nsMsgFilterService::BackUpFilterFile ]

Comment 12

5 years ago
and fwiw, it's rare so no great need to pursue this as far as I can tell.
Whiteboard: [rare]


4 years ago
Blocks: 820981

Comment 13

a year ago
this year no crashes newer than version 15.  So either gone, or not actionable without steps
Last Resolved: a year ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.