Closed Bug 615928 Opened 15 years ago Closed 5 years ago

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

Categories

(MailNews Core :: Database, defect)

1.9.2 Branch
x86
All
defect
Not set
critical

Tracking

(thunderbird_esr68 fixed)

RESOLVED FIXED
Thunderbird 74.0
Tracking Status
thunderbird_esr68 --- fixed

People

(Reporter: wsmwk, Assigned: benc)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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
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
OS: Windows Vista → All
Crash Signature: [@ nsMsgDBFolder::OnMessageClassified(char const*, unsigned int, unsigned int)]
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
Crash Signature: [@ nsMsgDBFolder::OnMessageClassified(char const*, unsigned int, unsigned int)] → [@ nsMsgDBFolder::OnMessageClassified(char const*, unsigned int, unsigned int)] [@ nsMsgDBFolder::OnMessageClassified ]
Depends on: 797710
No longer depends on: 797710
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
Summary: crash [@ nsMsgDBFolder::OnMessageClassified(char const*, unsigned int, unsigned int)] → crash [@ nsMsgDBFolder::OnMessageClassified via bayesian analysis, with aMsgURI null?

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

Crash Signature: [@ nsMsgDBFolder::OnMessageClassified(char const*, unsigned int, unsigned int)] [@ nsMsgDBFolder::OnMessageClassified ] → [@ nsMsgDBFolder::OnMessageClassified ]
Flags: needinfo?(benc)

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.

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...

Assignee: nobody → benc
Flags: needinfo?(benc)
Attachment #9119961 - Flags: review?(mkmelin+mozilla)

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 on attachment 9119961 [details] [diff] [review] 615928-add-getdatabase.patch Review of attachment 9119961 [details] [diff] [review]: ----------------------------------------------------------------- Seems alright to me, r=mkmelin
Attachment #9119961 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 74.0

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

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.

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

Flags: needinfo?(benc)

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

(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

Attachment #9119961 - Flags: approval-comm-esr68?
Attachment #9119961 - Flags: approval-comm-esr68? → approval-comm-esr68+
Flags: needinfo?(benc)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: