615928 - crash [@ nsMsgDBFolder::OnMessageClassified via bayesian analysis, with aMsgURI null?

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsMsgDBFolder::OnMessageClassified(char const*, unsigned int, unsigned int)]

bp-801df54b-0f78-4738-aee0-73bb02101126
EXCEPTION_ACCESS_VIOLATION_READ
0x0
0	thunderbird.exe	nsMsgDBFolder::OnMessageClassified	mailnews/base/util/nsMsgDBFolder.cpp:2247
1	thunderbird.exe	nsMsgLocalMailFolder::OnMessageClassified	mailnews/local/src/nsLocalMailFolder.cpp:3669
2	thunderbird.exe	nsBayesianFilter::classifyMessage	mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:1787
3	thunderbird.exe	MessageClassifier::analyzeTokens	mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:1367
4	thunderbird.exe	TokenStreamListener::OnStopRequest	mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:1208
5	thunderbird.exe	nsStreamConverter::OnStopRequest	mailnews/mime/src/nsStreamConverter.cpp:1116
6	thunderbird.exe	nsMsgProtocol::OnStopRequest	mailnews/base/util/nsMsgProtocol.cpp:401
7	thunderbird.exe	nsMailboxProtocol::OnStopRequest	mailnews/local/src/nsMailboxProtocol.cpp:381
8	thunderbird.exe	nsInputStreamPump::OnStateStop	netwerk/base/src/nsInputStreamPump.cpp:578
9	thunderbird.exe	nsInputStreamPump::OnInputStreamReady	netwerk/base/src/nsInputStreamPump.cpp:403
10	xpcom_core.dll	nsOutputStreamReadyEvent::Run	xpcom/io/nsStreamUtils.cpp:112

Comment 1

[timeless]

a bunch of the methods here check if (mDatabase), this one doesn't. it's likely that aMsgURI is null which is why we wouldn't crash earlier

Comment 2

[Wayne Mery (:wsmwk)]

these crashes might all be coming through nsBayesianFilter::classifyMessage

(In reply to timeless from comment #1)
> a bunch of the methods here check if (mDatabase), this one doesn't. it's
> likely that aMsgURI is null which is why we wouldn't crash earlier

recent tb16 crashes bp-4cb09a4f-f746-4dd7-ae73-3dd6c2121102 bp-4f2b6036-b8aa-4bff-84cb-17d072121018

Comment 3

[Wayne Mery (:wsmwk)]

Still a solid crash in TB60, in fact some 60.3.1 which was only recently released.

bp-c957c658-708b-4e36-a2f0-6c59a0181123 60.3.1
 0 	xul.dll	nsMsgDBFolder::OnMessageClassified(char const*, unsigned int, unsigned int)	comm/mailnews/base/util/nsMsgDBFolder.cpp:2372
1 	xul.dll	nsImapMailFolder::OnMessageClassified(char const*, unsigned int, unsigned int)	comm/mailnews/imap/src/nsImapMailFolder.cpp:9199
2 	xul.dll	MessageClassifier::classifyNextMessage()	comm/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:1327
3 	xul.dll	MessageClassifier::analyzeTokens(Tokenizer&)	comm/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:1313
4 	xul.dll	TokenStreamListener::OnStopRequest(nsIRequest*, nsISupports*, nsresult)	comm/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:1145
5 	xul.dll	nsStreamConverter::OnStopRequest(nsIRequest*, nsISupports*, nsresult)	comm/mailnews/mime/src/nsStreamConverter.cpp:1079

Comment 4

[Wayne Mery (:wsmwk)]

Interesting that many users only crash once (per crash stats).

bp-432a3657-15a4-41f8-916b-006480200105 is a current example that appears to have no additional addons. Can you take a crack using timeless' analysis

Comment 5

[Ben Campbell]

Offending line:
https://hg.mozilla.org/releases/comm-esr68/annotate/dfeab0e6d5a43c6bcb1575bbdc5c0d018b42df9b/mailnews/base/util/nsMsgDBFolder.cpp#l2215

as per comment #1, it looks like it's occurring when MessageClassifier::classifyNextMessage is trying to signal the end of a batch. It passes in a null aMsgURI to OnClassifyMessage() to indicate it's done.
Then as part of the end-of-batch processing, nsMsgDBFolder::OnClassifyMessage() is dying because it's mDatabase is null (I think)...

Stack traces show it happens via both nsMsgLocalMailFolder::OnClassifyMessage() and nsImapMailFolder::OnClassifiyMessage()... so doesn't look protocol-specific.

Comment 6

[Ben Campbell]

Attachment: 615928-add-getdatabase.patch

OK, one idea. mDatabase is (kind of) lazy-loaded. So this patch adds a GetDatabase() call to the start of OnMessageClassified(), to ensure mDatabase is set.

No way to test it without a test case, but it seems like a plausible fix...

Comment 7

[Ben Campbell]

Another thought:
There are a few operations in nsMsgDBFolder that open the database if it's not already open and close it again afterwards (nulling mDatabase). If it was already open they just leave it as is.
So it could be that the database was closed during message classification, but that wouldn't necessarily stop these kinds of operations from succeeding. And then the end-of-batch handling would fail because the db was still closed. Either way, my patch should handle that. But maybe my patch should be more aggressive about closing the database afterwards? I really don't know...

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9119961 [details] [diff] [review]
615928-add-getdatabase.patch

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

Seems alright to me, r=mkmelin

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2b1818522523
Ensure msg db is available in nsMsgDBFolder::OnMessageClassified(). r=mkmelin

Comment 10

[Wayne Mery (:wsmwk)]

The crash rate is low enough we can just let this ride until it hits beta 74 which is a couple weeks away. Then uplift to 68.

Comment 11

[Wayne Mery (:wsmwk)]

(amended comment)
One beta 73 crash bp-68776f29-a366-4ca0-a40c-6bcd10200228 in the past month.

Comment 12

[Magnus Melin [:mkmelin]]

That crash is 73.0b0 so wouldn't have this fix so I'd say it's gone.

Comment 13

[Wayne Mery (:wsmwk)]

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

That crash is 73.0b0 so wouldn't have this fix so I'd say it's gone.

Agreed - thanks for the correction. However, statistically the crash rate is too low in beta to declare it is gone.

That said, I think we should take the patch on esr

Comment 14

[Rob Lemley [:rjl]]

Thunderbird 68.6.0:
https://hg.mozilla.org/releases/comm-esr68/rev/4547876868f2