Closed
Bug 833399
Opened 11 years ago
Closed 11 years ago
crash in nsImapOfflineTxn::UndoTransaction
Categories
(MailNews Core :: Networking: IMAP, defect)
Tracking
(thunderbird-esr1722+ fixed)
RESOLVED
FIXED
Thunderbird 22.0
People
(Reporter: wsmwk, Assigned: aceman)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file, 1 obsolete file)
1.16 KB,
patch
|
aceman
:
review+
standard8
:
approval-comm-esr17+
|
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
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•11 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)
Comment 4•11 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: nobody → acelists
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #709520 -
Flags: review?(mozilla)
Comment 6•11 years ago
|
||
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+
Attachment #709520 -
Attachment is obsolete: true
Attachment #730305 -
Flags: review+
Keywords: checkin-needed
Comment 8•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/69b6b0d94e73
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
Updated•11 years ago
|
tracking-thunderbird-esr17:
--- → 21+
Updated•11 years ago
|
Comment 9•11 years ago
|
||
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+
Comment 10•11 years ago
|
||
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.
Description
•