Last Comment Bug 833399 - crash in nsImapOfflineTxn::UndoTransaction
: crash in nsImapOfflineTxn::UndoTransaction
Status: RESOLVED FIXED
: crash, regression
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: 14
: x86 All
: -- critical (vote)
: Thunderbird 22.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks:
  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: ---
22+
fixed


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

Description 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 :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 neil@parkwaycc.co.uk 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 :aceman 2013-02-01 00:03:20 PST
Maybe David Bienvenu could know.
Comment 4 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 :aceman 2013-02-03 13:00:25 PST
Created attachment 709520 [details] [diff] [review]
patch
Comment 6 Mark Banner (:standard8) 2013-03-27 04:30:34 PDT
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 2013-03-27 13:05:44 PDT
Created attachment 730305 [details] [diff] [review]
patch v2
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-03-27 13:16:30 PDT
https://hg.mozilla.org/comm-central/rev/69b6b0d94e73
Comment 9 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 Mark Banner (:standard8) 2013-06-19 02:44:13 PDT
https://hg.mozilla.org/releases/comm-esr17/rev/39d8c27a94da

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