Closed Bug 1563959 Opened 5 years ago Closed 5 years ago

Crash in [@ nsMsgFilterList::ApplyFiltersToHdr]

Categories

(MailNews Core :: Filters, defect)

x86
Windows 10
defect
Not set
critical

Tracking

(thunderbird_esr6069+ verified, thunderbird68 verified, thunderbird69 verified)

VERIFIED FIXED
Thunderbird 69.0
Tracking Status
thunderbird_esr60 69+ verified
thunderbird68 --- verified
thunderbird69 --- verified

People

(Reporter: wsmwk, Assigned: jorgk-bmo)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 2 obsolete files)

First appears in 68 beta 20190613171710, so presumably a regression

This bug is for crash report bp-2b38939a-f19a-46ce-ae58-e676a0190627.

Top 10 frames of crashing thread:

0 xul.dll nsMsgFilterList::ApplyFiltersToHdr comm/mailnews/base/search/src/nsMsgFilterList.cpp:289
1 xul.dll nsImapMailFolder::NormalEndMsgWriteStream comm/mailnews/imap/src/nsImapMailFolder.cpp:4291
2 xul.dll nsresult `anonymous namespace'::SyncRunnable4<nsIImapMessageSink, unsigned int, bool, nsIImapUrl*, int>::Run comm/mailnews/imap/src/nsSyncRunnableHelpers.cpp:184
3 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1175
4 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
5 xul.dll nsXULWindow::ShowModal xpfe/appshell/nsXULWindow.cpp:385
6 xul.dll nsContentTreeOwner::ShowAsModal xpfe/appshell/nsContentTreeOwner.cpp:450
7 xul.dll nsWindowWatcher::OpenWindowInternal toolkit/components/windowwatcher/nsWindowWatcher.cpp:1246
8 xul.dll nsWindowWatcher::OpenWindow toolkit/components/windowwatcher/nsWindowWatcher.cpp:290
9 xul.dll NS_InvokeByIndex 

There are a few options in the callee. We could also use an error check or something like this:
https://searchfox.org/comm-central/rev/30fb8479949b5246fda13398de98216dcb3ad346/mailnews/imap/src/nsImapMailFolder.cpp#3168
https://searchfox.org/comm-central/rev/30fb8479949b5246fda13398de98216dcb3ad346/mailnews/imap/src/nsImapMailFolder.cpp#4212

Look for GetMessageHeader in nsImapMailFolder.cpp to see all the variations.

Filter man, what do you say?

Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #9076401 - Flags: review?(acelists)
Comment on attachment 9076401 [details] [diff] [review]
1563959-check-header-for-null.patch

Review of attachment 9076401 [details] [diff] [review]:
-----------------------------------------------------------------

The crashing line 289 is msgHdr->GetMessageKey(&msgKey); which was added by the filter logging.
Yes, we do not check if msgHdr is null and can crash here. Before the filter logging we wer passing the null msgHdr to further functions and also there is a call to msgHdr->GetMessageId() but only if there are any filters defined and that null header matched a header, which hopefully isn't possible.

So it looks like the filter logging exposed a bug that we are getting a null header to apply filters to.

::: mailnews/base/search/src/nsMsgFilterList.cpp
@@ +272,5 @@
>                                     nsIMsgDatabase *db,
>                                     const nsACString &headers,
>                                     nsIMsgFilterHitNotify *listener,
>                                     nsIMsgWindow *msgWindow) {
> +  NS_ENSURE_ARG_POINTER(msgHdr);

OK.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +4280,5 @@
>        nsCOMPtr<nsIMsgDBHdr> newMsgHdr;
>        GetMessageHeader(uidOfMessage, getter_AddRefs(newMsgHdr));
>        GetMoveCoalescer();
>        nsCOMPtr<nsIMsgWindow> msgWindow;
> +      if (imapUrl && newMsgHdr) {

The header is not used in this block, why check it here?
Did you mean to skip calling m_filterList->ApplyFiltersToHdr() ?

Oops. Like this we skip running the filter since no one checks the return value. Or do you have a different suggestion?

Check the status of GetMessageHeader?

Attachment #9076404 - Flags: review?(acelists)
Attachment #9076401 - Attachment is obsolete: true
Attachment #9076401 - Flags: review?(acelists)

Crashing code added in bug 697522.

Regressed by: 697522

Discussed with Aceman on IRC. On a null header the function does nothing since filter->MatchHdr(msgHdr, returns an error on a null header.

We should also intialise the match result since the matching might have failed and then the result may be undefined and the logging will log garbage.

Attachment #9076404 - Attachment is obsolete: true
Attachment #9076404 - Flags: review?(acelists)
Attachment #9076407 - Flags: review?(acelists)
Comment on attachment 9076407 [details] [diff] [review]
1563959-check-header-for-null.patch (v3)

Review of attachment 9076407 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, thanks. We don't know if this is some error state, or proper operation, but now somebody can see it in the filter log and investigate further.
Attachment #9076407 - Flags: review?(acelists) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/604b7c9f720c
Check that we got a non-null header before running a filter on it (and crashing). r=aceman

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #9076407 - Flags: approval-comm-beta+

I think it's proper operation. IMAP messages can disappear for various reasons, so if there gone by the time we come to filter them, there's nothing to do. As you can see, the crash rate is low, so this is not common operation.

Target Milestone: --- → Thunderbird 69.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: