833399 - crash in nsImapOfflineTxn::UndoTransaction

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

This bug was filed from the Socorro interface and is 
report bp-8ff7273d-2c34-43a8-97b9-e42fd2130110 .
============================================================= 
0	xul.dll	nsImapOfflineTxn::UndoTransaction	mailnews/imap/src/nsImapUndoTxn.cpp:551
1	xul.dll	nsTransactionItem::UndoTransaction	editor/txmgr/src/nsTransactionItem.cpp:181
2	xul.dll	nsTransactionItem::UndoChildren	editor/txmgr/src/nsTransactionItem.cpp:227
3	xul.dll	nsTransactionItem::UndoTransaction	editor/txmgr/src/nsTransactionItem.cpp:171
4	xul.dll	nsTransactionManager::UndoTransaction	editor/txmgr/src/nsTransactionManager.cpp:134
5	xul.dll	nsMessenger::Undo	mailnews/base/src/nsMessenger.cpp:1468 

crash first appears in Thunderbird 14

Comment 1

[:aceman]

The code where it crashe looks like this:
switch (m_opType)
 case nsIMsgOfflineImapOperation::kMsgMoved:
 case nsIMsgOfflineImapOperation::kMsgCopy:
 case nsIMsgOfflineImapOperation::kAddedHeader:
 case nsIMsgOfflineImapOperation::kFlagsChanged:
 case nsIMsgOfflineImapOperation::kDeletedMsg:
 {
   nsCOMPtr<nsIMsgDBHdr> firstHdr = m_srcHdrs[0]; <---

I wonder if we are sure at least one element exists in the array m_srcHdrs.
Many (all?) functions in the file either append elements, or loop over them as 'for (i=0;i<m_srcHdrs.Count();i++)' so they cope with 0 elements fine. Shouldn't we do that here too? If we really don't want to undo all (the for loop) but only one, then at least check if it exists?

Comment 2

[[e-mail]]

I don't really know the offline imap code, sorry.
I wouldn't object to checking the length of the array, though I don't quite see the point of creating a transaction with nothing to do.

Comment 3

[:aceman]

Maybe David Bienvenu could know.

Comment 4

[David :Bienvenu]

bulletproofing this seems like the right thing to do, perhaps with an assertion, since as Neil says, it shouldn't happen.

Comment 5

[:aceman]

Attachment: patch

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 709520 [details] [diff] [review]
patch

Ok, this looks fine to me, we'll let it ride the trains.

One nit:

m_srcHdrs.Count() < 1 could be m_srcHdrs.IsEmpty().

Comment 7

[:aceman]

Attachment: patch v2

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/69b6b0d94e73

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 730305 [details] [diff] [review]
patch v2

[Triage Comment]
Taking for next esr as this seems like a stable fix.

Comment 10

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr17/rev/39d8c27a94da