963362 - crash in nsMsgFilterList::GetFilterAt(unsigned int, nsIMsgFilter**) filtering incoming mail

Status: RESOLVED WORKSFORME

(MailNews Core :: Filters, defect)

Description

[Wayne Mery (:wsmwk)]

This bug was filed from the Socorro interface and is 
report bp-b7b3f985-b67f-4cba-8441-b78f32131205.
=============================================================
0	xul.dll	nsMsgFilterList::GetFilterAt(unsigned int,nsIMsgFilter * *)	mailnews/base/search/src/nsMsgFilterList.cpp
1	xul.dll	nsMsgFilterList::GetFilterNamed(nsAString_internal const &,nsIMsgFilter * *)	mailnews/base/search/src/nsMsgFilterList.cpp
2	xul.dll	nsMsgIncomingServer::ConfigureTemporaryReturnReceiptsFilter(nsIMsgFilterList *)	mailnews/base/util/nsMsgIncomingServer.cpp
3	xul.dll	nsMsgIncomingServer::ConfigureTemporaryFilters(nsIMsgFilterList *)	mailnews/base/util/nsMsgIncomingServer.cpp
4	xul.dll	nsImapMailFolder::UpdateFolderWithListener(nsIMsgWindow *,nsIUrlListener *)	mailnews/imap/src/nsImapMailFolder.cpp
5	xul.dll	nsImapMailFolder::UpdateFolder(nsIMsgWindow *)	mailnews/imap/src/nsImapMailFolder.cpp
6	xul.dll	nsImapMailFolder::GetNewMessages(nsIMsgWindow *,nsIUrlListener *)	mailnews/imap/src/nsImapMailFolder.cpp
7	xul.dll	nsImapIncomingServer::PerformBiff(nsIMsgWindow *)	mailnews/imap/src/nsImapIncomingServer.cpp
8	xul.dll	nsMsgBiffManager::PerformBiff()	mailnews/base/src/nsMsgBiffManager.cpp 

Crash is uncommon enough that no beta users crash in last 6 months.
Not new in version 24, but appears to crash more often than version 17
I think the stack above is most common, with ConfigureTemporaryReturnReceiptsFilter on stack.  

One example differing is:
bp-2e93832f-62be-4c3c-a63d-325a82131209
0	xul.dll	nsMsgFilterList::GetFilterAt(unsigned int,nsIMsgFilter * *)	mailnews/base/search/src/nsMsgFilterList.cpp
1	xul.dll	nsMsgFilterList::ComputeArbitraryHeaders()	mailnews/base/search/src/nsMsgFilterList.cpp
2	xul.dll	nsMsgFilterList::GetArbitraryHeaders(nsACString_internal &)	mailnews/base/search/src/nsMsgFilterList.cpp
3	xul.dll	nsImapIncomingServer::GetArbitraryHeaders(nsACString_internal &)	mailnews/imap/src/nsImapIncomingServer.cpp
4	xul.dll	`anonymous namespace'::SyncRunnable1<nsIImapServerSink,nsACString_internal const &>::Run()	mailnews/imap/src/nsSyncRunnableHelpers.cpp
5	xul.dll	nsThread::ProcessNextEvent(bool,bool *)	xpcom/threads/nsThread.cpp

Comment 1

[:aceman]

The offending line was changed by me in bug 180312:
acelists@12735
   928  NS_IF_ADDREF(*filter = m_filters[filterIndex]);

Per comment https://bugzilla.mozilla.org/show_bug.cgi?id=180312#c20 .

Can you see if there are any crashes at this point before TB16 where that patch landed?

Comment 2

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #1)
> The offending line was changed by me in bug 180312:
> acelists@12735
>    928  NS_IF_ADDREF(*filter = m_filters[filterIndex]);
> 
> Per comment https://bugzilla.mozilla.org/show_bug.cgi?id=180312#c20 .
> 
> Can you see if there are any crashes at this point before TB16 where that
> patch landed?

Crash rate is low and I can only go back as far as sept 2012, so it's hard to definitely say no crashes before TB16. But since Sept 2012 I found nothing older than crash date 2012-10-19 16.0.1 build 20121010150919. So best guess is regression. (actually found one version 15 crash but it was startup and I can't see the stack, so I'm considering it to be a freak)

Comment 3

[Wayne Mery (:wsmwk)]

David from Bug 915107 - crash in nsMsgSearchSession::~nsMsgSearchSession() via nsMsgFilterAfterTheFact::~nsMsgFilterAfterTheFact - had this crash sig some months ago. bp-5b7ce080-745a-480a-a2fb-f63822130301

Comment 4

[Wayne Mery (:wsmwk)]

answer to your questions in comment 2.

according to user of https://crash-stats.mozilla.com/report/index/d583ac36-90b6-4824-89b4-ee7832131224 this happens while incoming mail is filtered

Comment 5

[:aceman]

If we do not know if it started only after the patch, then we have no answer. This would need the calibres of rkent, standard8 or Neil to see if that line is correct. I do not yet understand these NS_IF_ADDREF() games.

Comment 6

[:aceman]

Would something.swap(*filter) work better here?

Comment 7

[[e-mail]]

(In reply to aceman from comment #6)
> Would something.swap(*filter) work better here?

No, you're trying to get an additional reference to the indexed filter. (The filter should still exist, because the nsCOMArray should have an owning reference to it.)

Comment 8

[:aceman]

m_filters is a nsTArray. Does it change anything?

Comment 9

[[e-mail]]

Well, it's an nsTArray<nsCOMPtr<...> > so it *should* be OK...

Comment 10

[:aceman]

There is a new bug 984810 similar to this.
In all stacks I could see there is nsMsgIncomingServer::ConfigureTemporaryReturnReceiptsFilter before the spot where it crashes.
Does anything bad happen in "rv = GetFilterAt(i, getter_AddRefs(filter));" if filter is null?

Comment 11

[:aceman]

Is it OK if we NS_IF_ADDREF() the same filter twice?

Comment 12

[Kent James (:rkent)]

"Is it OK if we NS_IF_ADDREF() the same filter twice?" In general, no. That would result in a memory leak because the reference count is off.

Comment 13

[:aceman]

I think we do it in GetFilterAt and  the GetFilterNamed. But would it cause a crash?

Comment 14

[Wayne Mery (:wsmwk)]

Aceman, crash rate for this and bug 984810 is near zero for a 3 month period, so I'm not sure a bug exist anymore.  
So closing both WFM. If you disagree, please reopen.