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

Status: RESOLVED WORKSFORME

(MailNews Core :: Filters, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsMsgFilterService::BackUpFilterFile(nsILocalFile*, nsIMsgWindow*)]
bp-a92775cc-20d2-409a-9705-f55642101101
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

Comment 1

[:aceman]

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

Comment 2

[:aceman]

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

[Wayne Mery (:wsmwk)]

(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
bp-ad2a1223-8f86-45f8-8aeb-562ef2120103
bp-53797f74-e4bb-4d41-9149-172012120102


(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

[:aceman]

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

Comment 5

[:aceman]

The reports still continue (like 6af35d1b-2943-4a50-b07e-3a87a2120829) so let's try some patch.

Comment 6

[:aceman]

bp-6af35d1b-2943-4a50-b07e-3a87a2120829

Comment 7

[:aceman]

Attachment: patch

Comment 8

[Kent James (:rkent)]

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

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:

NS_ENSURE_ARG_POINTER(aFilterFile);

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.

Comment 9

[Mark Banner (:standard8)]

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

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.

Comment 10

[:aceman]

Thanks, no problem.

Comment 11

[Wayne Mery (:wsmwk)]

signature has changed to nsMsgFilterService::BackUpFilterFile(nsIFile*, nsIMsgWindow*)

Comment 12

[Wayne Mery (:wsmwk)]

and fwiw, it's rare so no great need to pursue this as far as I can tell.

Comment 13

[Wayne Mery (:wsmwk)]

this year no crashes newer than version 15.  So either gone, or not actionable without steps
https://crash-stats.mozilla.com/signature/?date=%3E2016-02-01&signature=nsMsgFilterService%3A%3ABackUpFilterFile&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1