The default bug view has changed. See this FAQ.

crash in nsImapOfflineTxn::UndoTransaction

RESOLVED FIXED in Thunderbird 22.0

Status

MailNews Core
Networking: IMAP
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wsmwk, Assigned: aceman)

Tracking

({crash, regression})

Thunderbird 22.0
x86
All
crash, regression

Thunderbird Tracking Flags

(thunderbird-esr1722+ fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

1.16 KB, patch
aceman
: review+
Details | Diff | Splinter Review
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
(Assignee)

Comment 1

4 years ago
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?
Status: NEW → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(neil)

Comment 2

4 years ago
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.
Flags: needinfo?(neil)
(Assignee)

Comment 3

4 years ago
Maybe David Bienvenu could know.
Flags: needinfo?(mozilla)

Comment 4

4 years ago
bulletproofing this seems like the right thing to do, perhaps with an assertion, since as Neil says, it shouldn't happen.
Flags: needinfo?(mozilla)
(Assignee)

Comment 5

4 years ago
Created attachment 709520 [details] [diff] [review]
patch
Assignee: nobody → acelists
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #709520 - Flags: review?(mozilla)
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().
Attachment #709520 - Flags: review?(mozilla) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 730305 [details] [diff] [review]
patch v2
Attachment #709520 - Attachment is obsolete: true
Attachment #730305 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/69b6b0d94e73
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
tracking-thunderbird-esr17: --- → 21+
tracking-thunderbird-esr17: 21+ → 22+
Comment on attachment 730305 [details] [diff] [review]
patch v2

[Triage Comment]
Taking for next esr as this seems like a stable fix.
Attachment #730305 - Flags: approval-comm-esr17+
https://hg.mozilla.org/releases/comm-esr17/rev/39d8c27a94da
status-thunderbird-esr17: --- → fixed
You need to log in before you can comment on or make changes to this bug.