1658797 - Crash in [@ InvalidArrayIndex_CRASH | nsMsgFolderNotificationService::NotifyMsgsMoveCopyCompleted]

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

Just another imap issue?

new signature for 78. Earliest build is 20200616 bp-a9ae379f-335e-4dad-902a-cdb4f0200710

This bug is for crash report bp-7a5d01c5-7b10-41a2-bbae-5e5190200812.

Top 10 frames of crashing thread:

0 xul.dll InvalidArrayIndex_CRASH xpcom/ds/nsTArray.cpp:27
1 xul.dll nsMsgFolderNotificationService::NotifyMsgsMoveCopyCompleted comm/mailnews/base/src/nsMsgFolderNotificationService.cpp:111
2 xul.dll nsImapMailFolder::OnStopRunningUrl comm/mailnews/imap/src/nsImapMailFolder.cpp
3 xul.dll nsMsgMailNewsUrl::SetUrlState comm/mailnews/base/util/nsMsgMailNewsUrl.cpp:231
4 xul.dll nsImapMailFolder::SetUrlState comm/mailnews/imap/src/nsImapMailFolder.cpp:6337
5 xul.dll `anonymous namespace'::SyncRunnable5<nsIImapMailFolderSink, nsIImapProtocol*, nsIMsgMailNewsUrl*, bool, bool, nsresult>::Run comm/mailnews/imap/src/nsSyncRunnableHelpers.cpp:221
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1211
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:501
8 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308

Comment 1

[Magnus Melin [:mkmelin]]

Maybe we need to check that we have > 0 srcMsg instead of checking the count of the listeners?
https://searchfox.org/comm-central/rev/78a165ed474d0278d4c23c947929d4bf17d6c593/mailnews/base/src/nsMsgFolderNotificationService.cpp#83,92

Comment 2

[Ben Campbell]

Attachment: 1658797-dont-crash-with-empty-srcmsgs-1.patch

Yup... the line numbers in the stack trace seem a little tricky, but I think the likely culprit is an notifying with an empty aSrcMsgs array.
This patch early-outs if aSrcMsgs is empty or if there are no listeners to notify.

I'd much prefer tightening up the (implied) interface contract and making an empty aSrcMsgs trigger an assert, but there'd be a bunch of work going through all the places where it is called and figuring out exactly what they're intending. So for now, this.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9173800 [details] [diff] [review]
1658797-dont-crash-with-empty-srcmsgs-1.patch

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

Thanks, r=mkmelin

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/aa87011ee2d9
Don't crash in nsMsgFolderNotificationService::NotifyMsgsMoveCopyCompleted() if no messages in notification. r=mkmelin DONTBUILD

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1658797_folderlistener_crash_followup.patch

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/131ea62c0db9
correct fix for Crash in [@ InvalidArrayIndex_CRASH | nsMsgFolderNotificationService::NotifyMsgsMoveCopyCompleted]. rs=bustage-fix

Comment 7

[Magnus Melin [:mkmelin]]

The fix wasn't quite right - showing some X3 errors for comm/mailnews/base/test/unit/test_nsIMsgFolderListener.js: https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedTaskRun=Ichx1ygMQ_OcyJj_Rprr1g.0

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9173852 [details] [diff] [review]
bug1658797_folderlistener_crash_followup.patch

[Approval Request Comment]
Crash fix. Both changesets:
https://hg.mozilla.org/comm-central/rev/aa87011ee2d9
https://hg.mozilla.org/comm-central/rev/131ea62c0db9

Comment 9

[Wayne Mery (:wsmwk)]

Comment on attachment 9173852 [details] [diff] [review]
bug1658797_folderlistener_crash_followup.patch

[Triage Comment]
Approved for esr78.

Comment 10

[Rob Lemley [:rjl]]

Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/31046f44c40c
https://hg.mozilla.org/releases/comm-esr78/rev/25d00387e071