View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002834 | ardour | bugs | public | 2009-08-22 23:50 | 2020-04-19 20:14 |
| Reporter | lincoln | Assigned To | paul | ||
| Priority | normal | Severity | crash | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Target Version | 3.0-beta1 | ||||
| Summary | 0002834: Crash when undo history limit reached | ||||
| Description | I have been noticing that under some circumstances Ardour 3 will crash unexpectedly after some operation. I tested A3 with a history depth of 1 and this accelerated the problem and made some patterns repeatable. After much digging around it came down to a double delete of actions in UndoTransactio::clear. Not sure if this is also present in Ardour 2 I am attaching a patch to clear this crash. The patch also includes a fix to ensure that nested undo transactions are handled correctly in session::begin_reversable_command and session::commit_reversable_command. This will will hopefully make these type of nested undo commands a bit more robust. Any feedback welcome. | ||||
| Tags | No tags attached. | ||||
|
2009-08-22 23:50
|
nested-undo-and-undo-double-free-fix.patch (2,886 bytes)
Index: libs/ardour/session_state.cc
===================================================================
--- libs/ardour/session_state.cc (revision 5564)
+++ libs/ardour/session_state.cc (working copy)
@@ -2151,10 +2151,12 @@
{
UndoTransaction* trans = new UndoTransaction();
trans->set_name(name);
-
+
if (!_current_trans.empty()) {
+ _current_trans.top()->inc_count();
_current_trans.top()->add_command (trans);
} else {
+ trans->inc_count();
_current_trans.push(trans);
}
}
@@ -2164,12 +2166,13 @@
{
assert(!_current_trans.empty());
struct timeval now;
-
+
if (cmd) {
+ _current_trans.top()->dec_count();
_current_trans.top()->add_command(cmd);
}
- if (_current_trans.top()->empty()) {
+ if (_current_trans.top()->empty()) {
_current_trans.pop();
return;
}
@@ -2177,8 +2180,10 @@
gettimeofday(&now, 0);
_current_trans.top()->set_timestamp(now);
- _history.add(_current_trans.top());
- _current_trans.pop();
+ if(_current_trans.top()->count() == 0){
+ _history.add(_current_trans.top());
+ _current_trans.pop();
+ }
}
Session::GlobalRouteBooleanState
Index: libs/pbd/undo.cc
===================================================================
--- libs/pbd/undo.cc (revision 5564)
+++ libs/pbd/undo.cc (working copy)
@@ -34,6 +34,7 @@
: _clearing(false)
{
gettimeofday (&_timestamp, 0);
+ _count = 0;
}
UndoTransaction::UndoTransaction (const UndoTransaction& rhs)
@@ -42,6 +43,7 @@
{
clear ();
actions.insert(actions.end(),rhs.actions.begin(),rhs.actions.end());
+ _count = 0;
}
UndoTransaction::~UndoTransaction ()
@@ -74,6 +76,30 @@
return *this;
}
+int
+UndoTransaction::get_action_count ()
+{
+ return actions.size();
+}
+
+int
+UndoTransaction::count ()
+{
+ return _count;
+}
+
+int
+UndoTransaction::inc_count ()
+{
+ return _count++;
+}
+
+int
+UndoTransaction::dec_count ()
+{
+ return _count--;
+}
+
void
UndoTransaction::add_command (Command *const action)
{
@@ -115,9 +141,6 @@
UndoTransaction::clear ()
{
_clearing = true;
- for (list<Command*>::iterator i = actions.begin(); i != actions.end(); ++i) {
- delete *i;
- }
actions.clear ();
_clearing = false;
}
Index: libs/pbd/pbd/undo.h
===================================================================
--- libs/pbd/pbd/undo.h (revision 5564)
+++ libs/pbd/pbd/undo.h (working copy)
@@ -41,7 +41,12 @@
void clear ();
bool empty() const;
bool clearing () const { return _clearing; }
-
+
+ int inc_count ();
+ int dec_count ();
+ int count ();
+
+ int get_action_count ();
void add_command (Command* const);
void remove_command (Command* const);
@@ -64,7 +69,8 @@
std::list<PBD::ProxyShiva<Command,UndoTransaction>*> shivas;
struct timeval _timestamp;
bool _clearing;
-
+ int _count;
+
friend void command_death (UndoTransaction*, Command *);
friend class UndoHistory;
|
|
|
Noticed that a similar situation may exist in UndoTransaction::about_to_explicitly_delete () |
|
|
Where else do commands get deleted about from UndoTransaction::clear ? |
|
|
If I understand your question correctly please look at the relevant part of the patch: UndoTransaction::clear () { _clearing = true; - for (list<Command*>::iterator i = actions.begin(); i != actions.end(); ++i) { - delete *i; - } actions.clear (); _clearing = false; } The for loop is deleting the Commands, then actions.clear is called which calls the destructors a second time. |
|
|
Sorry, "about" should have been "apart" in my question! actions.clear() will not call destructors for the Commands in the list. |
|
|
C++ references state otherwise. void clear ( ); Clear content All the elements in the list container are dropped: their destructors are called, and then they are removed from the list container, leaving it with a size of 0. |
|
|
Yes, but the list contains pointers, which don't have destructors. If it were a list<Command> then the Command destructors would get deleted. In the case of list<Command*> no Command destructors are called on ::clear(). |
|
|
I see that. However I am also seeing memory corruption and crashes when removing commands from the undo history list. I guess this needs more investigation. |
|
|
the nested command part is nice though. we've started to run into this issue for a very simple reason. until dave started work on MIDI editing, all reversible commands originated in the Editor class, and always serially. at this point in time, MidiRegionView initiates some reversible commands, which may or may not be nested inside a "wider" transaction at the Editor level. i'm not sure that this is a good design, but given that fixing it is a lot of work, i'm tempted to use the nested part of this patch for the time being. |
|
|
Anyone remember where we got to with this? |
|
|
Should be fixed in SVN with a patch designed by Lincoln which allows nested undo commands. |
|
|
Issue has been closed automatically, by Trigger Close Plugin. Feel free to re-open with additional information if you think the issue is not resolved. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2009-08-22 23:50 | lincoln | New Issue | |
| 2009-08-22 23:50 | lincoln | File Added: nested-undo-and-undo-double-free-fix.patch | |
| 2009-08-23 00:06 | lincoln | Note Added: 0006590 | |
| 2009-08-24 18:28 | cth103 | Note Added: 0006591 | |
| 2009-08-24 23:14 | lincoln | Note Added: 0006599 | |
| 2009-08-24 23:55 | cth103 | Note Added: 0006603 | |
| 2009-08-25 00:07 | lincoln | Note Added: 0006604 | |
| 2009-08-25 00:12 | cth103 | Note Added: 0006605 | |
| 2009-08-25 00:20 | lincoln | Note Added: 0006606 | |
| 2009-09-05 13:10 | paul | Note Added: 0006646 | |
| 2009-09-05 13:11 | paul | Status | new => assigned |
| 2009-09-05 13:11 | paul | Assigned To | => paul |
| 2010-04-24 10:28 | cth103 | Category | bugs => bugs2 |
| 2010-04-24 10:31 | cth103 | Category | bugs2 => bugs |
| 2010-07-21 15:33 | cth103 | cost | => 0.00 |
| 2010-07-21 15:33 | cth103 | Target Version | => 3.0-beta1 |
| 2010-08-14 15:20 | cth103 | Note Added: 0008836 | |
| 2010-08-19 15:02 | cth103 | Status | assigned => feedback |
| 2011-01-19 17:40 | cth103 | Note Added: 0009947 | |
| 2011-01-19 17:40 | cth103 | Status | feedback => resolved |
| 2011-01-19 17:40 | cth103 | Resolution | open => fixed |
| 2020-04-19 20:14 | system | Note Added: 0021980 | |
| 2020-04-19 20:14 | system | Status | resolved => closed |