Closed Bug 1666115 Opened 9 months ago Closed 9 months ago

crash [@ nsMsgFilterAfterTheFact::ApplyFilter(int*)]

Categories

(MailNews Core :: Filters, defect)

1.9.1 Branch
x86
All
defect
Not set
critical

Tracking

(thunderbird_esr78+ verified, thunderbird82 fixed)

VERIFIED FIXED
83 Branch
Tracking Status
thunderbird_esr78 + verified
thunderbird82 --- fixed

People

(Reporter: wsmwk, Assigned: mkmelin)

References

()

Details

(Keywords: crash, regression, regressionwindow-wanted)

Crash Data

Attachments

(2 files, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #537017 +++

Earliest crash is buildid 20200511021338 77 beta bp-7010429d-6c3d-4295-8f7b-3d1f90200919
Looks like we shipped 77.0b1 https://hg.mozilla.org/releases/comm-beta/pushloghtml?startdate=2020-05-01&enddate=2020-05-11

bp-b8d38796-b190-4c00-9fec-03c482091224 from 20200511021338
0 xul.dll nsMsgFilterAfterTheFact::ApplyFilter() comm/mailnews/search/src/nsMsgFilterService.cpp:1053
1 xul.dll nsMsgApplyFiltersToMessages::RunNextFilter() comm/mailnews/search/src/nsMsgFilterService.cpp:1318
2 xul.dll nsMsgFilterAfterTheFact::AdvanceToNextFolder() comm/mailnews/search/src/nsMsgFilterService.cpp:552
3 xul.dll nsMsgFilterService::ApplyFilters(int, nsIArray*, nsIMsgFolder*, nsIMsgWindow*, nsIMsgOperationListener*) comm/mailnews/search/src/nsMsgFilterService.cpp:1389
4 xul.dll nsMsgDBFolder::OnMessageClassified(char const*, unsigned int, unsigned int) comm/mailnews/base/src/nsMsgDBFolder.cpp:2117
5 xul.dll nsMsgLocalMailFolder::OnMessageClassified(char const*, unsigned int, unsigned int) comm/mailnews/local/src/nsLocalMailFolder.cpp:3243
6 xul.dll nsMsgDBFolder::CallFilterPlugins(nsIMsgWindow*, bool*) comm/mailnews/base/src/nsMsgDBFolder.cpp:2559
7 xul.dll nsParseNewMailState::EndMsgDownload() comm/mailnews/local/src/nsParseMailbox.cpp:2308
8 xul.dll nsPop3Sink::EndMailDelivery(nsIPop3Protocol*) comm/mailnews/local/src/nsPop3Sink.cpp:231
9 xul.dll nsPop3Protocol::GetMsg() comm/mailnews/local/src/nsPop3Protocol.cpp:2738
10 xul.dll nsPop3Protocol::ProcessProtocolState(nsIURI*, nsIInputStream*, unsigned long long, unsigned int) comm/mailnews/local/src/nsPop3Protocol.cpp:3723
11 xul.dll nsMsgProtocol::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long long, unsigned int) comm/mailnews/base/src/nsMsgProtocol.cpp:298

Flags: needinfo?(mkmelin+mozilla)

On trunk the corresponding crash line would be https://searchfox.org/comm-central/rev/472225cef15a3c0c18ff2f14b893fafc1cb61c55/mailnews/search/src/nsMsgFilterService.cpp#1020 - but it's not something that can crash, so lines are not correct here.

Flags: needinfo?(mkmelin+mozilla)

https://hg.mozilla.org/releases/comm-esr78/file/tip/mailnews/search/src/nsMsgFilterService.cpp#l1053 which is

      (void)m_curFilter->LogRuleHitFail(

But strangely that whole NS_FAILED(finalResult) section looks like dead code, since finalResult is never assigned outside of it, except for the first assignment to NS_OK! I must be missing something...

(In reply to Magnus Melin [:mkmelin] from comment #2)

I must be missing something...

Took me a bit of head scratching too, but turns out finalResult is set in the BREAK_ACTION_IF_FALSE / BREAK_ACTION_IF_FAILURE (source code) macros.

Notes after a quick poke about:

m_curFilter looks like it's checked for null up near the top of that block before we hit the crash.
https://hg.mozilla.org/releases/comm-esr78/file/85fc86689124860ac5637083c75f60cb72b1c7dc/mailnews/search/src/nsMsgFilterService.cpp#l645

I don't see anywhere obvious(!) where m_curFilter might be reset during that big switch statement. However, there's a note just above the check:

// 'm_curFolder' can be reset asynchronously by the copy service
// calling OnStopCopy(). So take a local copy here and use it throughout the
// function.

Maybe m_curFilter is also being asynchronously reset by something? I don't think there's any multithreading involved in this code (right?), so async would have to mean something called from nsMsgFilterAfterTheFact::ApplyFilter(), between the nullptr check at the top and the logging line at the bottom which crashes
...
regarding a possible fix: I don't know the code well enough to judge if keeping a local copy of m_curFilter to hold it in scope is a safe thing to do or not. Would have to dig about and figure out the bigger picture...

Yeah, iirc filter is mainthread only

Conceivable this could fix it, and at least wouldn't make things worse.

Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #9180256 - Flags: review?(benc)
Attachment #9180256 - Attachment is obsolete: true
Attachment #9180256 - Flags: review?(benc)
Attachment #9180262 - Flags: review?(benc)
Comment on attachment 9180262 [details] [diff] [review]
bug1666115_ApplyFilter_crash.patch

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

Would be nice to find the underlying cause, but yes - can't hurt, so r+ LGTM here!

::: mailnews/search/src/nsMsgFilterService.cpp
@@ +650,4 @@
>      if (!curFolder)
>        break;  // Maybe not an error, we just need to call AdvanceToNextFolder();
>  
> +    nsCOMPtr<nsIMsgFilter> curFilter = m_curFilter;

I'd probably shuffle the ordering about a little here, just to make it _really_ obvious that the same trick is being used on both m_curFilter and m_curFolder.  (i.e. make both the checks use the m_ member vars, or take both local copies first before the checks).
Attachment #9180262 - Flags: review?(benc) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/5bccf9f7afae
fix crash [@ nsMsgFilterAfterTheFact::ApplyFilter(int*)]. r=benc

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch

Comment on attachment 9180262 [details] [diff] [review]
bug1666115_ApplyFilter_crash.patch

[Approval Request Comment]
Very safe potential crash fix.

Attachment #9180262 - Flags: approval-comm-esr78?
Attachment #9180262 - Flags: approval-comm-beta?

Comment on attachment 9180262 [details] [diff] [review]
bug1666115_ApplyFilter_crash.patch

[Triage Comment]
Approved for beta

Attachment #9180262 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9180262 [details] [diff] [review]
bug1666115_ApplyFilter_crash.patch

[Triage Comment]
Approved for esr78

Attachment #9180262 - Flags: approval-comm-esr78? → approval-comm-esr78+

Seems to be missing a dependency on esr78, the code that declares hitHdrs in nsMsgFilterAfterTheFact::ApplyFilter() is not there, so I would expect that this would not compile unless that bug is identified and uplifted as well.

Flags: needinfo?(rob)
Flags: needinfo?(mkmelin+mozilla)

That would be from bug 1612239 which we don't want to uplift.

Flags: needinfo?(mkmelin+mozilla)

ESR 78 version. For some reason my local 78 builds don't work atm, so untested.

Attachment #9182190 - Flags: review+

Ran a local build successfully with the revised patch.

Flags: needinfo?(rob)

Does appear to be gone from beta and esr

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.