Last Comment Bug 833399 - crash in nsImapOfflineTxn::UndoTransaction
: crash in nsImapOfflineTxn::UndoTransaction
: crash, regression
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: 14
: x86 All
-- critical (vote)
: Thunderbird 22.0
Assigned To: :aceman
Depends on:
  Show dependency treegraph
Reported: 2013-01-22 09:06 PST by Wayne Mery (:wsmwk, NI for questions)
Modified: 2013-06-19 02:44 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (1.16 KB, patch)
2013-02-03 13:00 PST, :aceman
standard8: review+
Details | Diff | Splinter Review
patch v2 (1.16 KB, patch)
2013-03-27 13:05 PDT, :aceman
acelists: review+
standard8: approval‑comm‑esr17+
Details | Diff | Splinter Review

Description User image Wayne Mery (:wsmwk, NI for questions) 2013-01-22 09:06:48 PST
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 User image :aceman 2013-01-28 06:14:28 PST
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 User image 2013-01-31 17:21:56 PST
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 User image :aceman 2013-02-01 00:03:20 PST
Maybe David Bienvenu could know.
Comment 4 User image David :Bienvenu 2013-02-01 09:47:37 PST
bulletproofing this seems like the right thing to do, perhaps with an assertion, since as Neil says, it shouldn't happen.
Comment 5 User image :aceman 2013-02-03 13:00:25 PST
Created attachment 709520 [details] [diff] [review]
Comment 6 User image Mark Banner (:standard8) 2013-03-27 04:30:34 PDT
Comment on attachment 709520 [details] [diff] [review]

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 User image :aceman 2013-03-27 13:05:44 PDT
Created attachment 730305 [details] [diff] [review]
patch v2
Comment 8 User image Ryan VanderMeulen [:RyanVM] 2013-03-27 13:16:30 PDT
Comment 9 User image Mark Banner (:standard8) 2013-06-19 02:40:00 PDT
Comment on attachment 730305 [details] [diff] [review]
patch v2

[Triage Comment]
Taking for next esr as this seems like a stable fix.
Comment 10 User image Mark Banner (:standard8) 2013-06-19 02:44:13 PDT

Note You need to log in before you can comment on or make changes to this bug.