1683549 - Crash in [@ InvalidArrayIndex_CRASH | nsImapMailFolder::CopyNextStreamMessage]

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Wayne Mery (:wsmwk)]

regression?

Crash report: https://crash-stats.mozilla.org/report/index/d51cfd8a-e831-4a77-91a5-6fb9b0201210 - 85.0a1 buildid 20201209104855 is the first crash. Also crashing in beta bp-87f4ec6c-b365-48e8-8f05-617310201220

MOZ_CRASH Reason: ElementAt(aIndex = 0, aLength = 0)

Top 10 frames of crashing thread:

0 xul.dll InvalidArrayIndex_CRASH xpcom/ds/nsTArray.cpp:28
1 xul.dll nsImapMailFolder::CopyNextStreamMessage comm/mailnews/imap/src/nsImapMailFolder.cpp:6214
2 xul.dll `anonymous namespace'::SyncRunnable2<nsIImapMailFolderSink, bool, nsISupports*>::Run comm/mailnews/imap/src/nsSyncRunnableHelpers.cpp:121
3 xul.dll mozilla::RunnableTask::Run xpcom/threads/TaskController.cpp:452
4 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:732
5 xul.dll mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:591
6 xul.dll mozilla::TaskController::ProcessPendingMTTask xpcom/threads/TaskController.cpp:375
7 xul.dll mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:122:7'>::Run xpcom/threads/nsThreadUtils.h:534
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1200
9 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:548

Comment 1

[Wayne Mery (:wsmwk)]

#8 crash for beta 85

Comment 2

[Magnus Melin [:mkmelin]]

Crash at https://searchfox.org/comm-central/rev/290abc2f4584c108051881abef139fcc243b92ae/mailnews/imap/src/nsImapMailFolder.cpp#6214
Yes, regression from bug 1612239 - https://hg.mozilla.org/comm-central/rev/c2ddbcc4f197#l28.175. Some logic error. Ben, please have a look.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1683549_cpmsg_crash.patch

Comment 4

[Ben Campbell]

Comment on attachment 9194919 [details] [diff] [review]
bug1683549_cpmsg_crash.patch

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

Actually, I think this patch would probably fix the crash, but it's a bit of a band-aid and I think we're better to try and address the root cause.

I _thought_ it'd be an easy fix to remove the redundant `nsImapMailCopyState::m_totalCount`.  It always just holds the size of m_messages array. Well.... almost always.
Turns out that the various different copy paths (inter-server, local, from file etc etc) all use nsImapMailCopyState in subtly different and incompatible ways.
Trying to figure out how it works led me down a pretty deep rabbit hole. I've pulled together some patches which I think should fix this bug, perform a little cleanup and also remove some more possible future footguns. The IMAP folder class and nsImapMailCopyState could do with a serious refactoring of the copy code, but that'd be a big job.

Heres a try run for the patches to follow:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=25352c760cb35aaaf11e3f4015bd0101ae8e8009

Comment 5

[Ben Campbell]

Attachment: 1683549-part-one-qi-cleanup-1.patch

This one removes a bunch of redundant QIs in the copy code. Not strictly required, but less code is always good :-)

Comment 6

[Ben Campbell]

Attachment: 1683549-part-two-remove-m_totalCount-1.patch

This one should provide the actual bugfix. Sidesteps the issue of m_totalCount getting out of sync by just removing m_totalCount altogether.

Comment 7

[Ben Campbell]

Attachment: 1683549-part-three-remove-m_message-1.patch

And I noticed that the currently-being-processed message had it's own pointer (m_message), which really isn't required, so this removes it.

This set of patches passes the unit tests locally just fine, so I'm pretty confident of them. But will be nice to see how the mochitests fare in the try run...

Comment 8

[Ben Campbell]

Attachment: 1683549-part-two-remove-m_totalCount-2.patch

How odd. The mac build required a little (uint32_t) cast that windows and linux did not...
Seems OK now:

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ef1ac4557e1e3d7b25ea50c4e9dba66c9edaeeb0

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9196729 [details] [diff] [review]
1683549-part-one-qi-cleanup-1.patch

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

LGTM, r=mkmelin

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9196731 [details] [diff] [review]
1683549-part-three-remove-m_message-1.patch

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

::: mailnews/imap/src/nsImapMailFolder.h
@@ +53,5 @@
>  
>    nsCOMPtr<nsISupports> m_srcSupport;        // source file spec or folder
>    nsTArray<RefPtr<nsIMsgDBHdr>> m_messages;  // array of source messages
>    RefPtr<nsImapMoveCopyMsgTxn>
> +      m_undoMsgTxn;  // undo object with this copy operation

funny indention+wrapping. If clang-format wants it this way, maybe we should just put the comments one line before

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/097774940250
Tidy away some unnecessary QI casting in nsImapMailFolder message copying. r=mkmelin
https://hg.mozilla.org/comm-central/rev/eaf78ae76ba0
Remove redundant m_totalCount member from nsImapMailCopyState. r=mkmelin
https://hg.mozilla.org/comm-central/rev/ecee47190351
Remove redundant m_message member from nsImapMailCopyState. r=mkmelin