View Issue Details

IDProjectCategoryView StatusLast Update
0002834ardourbugspublic2020-04-19 20:14
Reporterlincoln Assigned Topaul  
PrioritynormalSeveritycrashReproducibilityalways
Status closedResolutionfixed 
Target Version3.0-beta1 
Summary0002834: Crash when undo history limit reached
DescriptionI 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.
TagsNo tags attached.

Activities

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;

lincoln

2009-08-23 00:06

reporter   ~0006590

Noticed that a similar situation may exist in UndoTransaction::about_to_explicitly_delete ()

cth103

2009-08-24 18:28

administrator   ~0006591

Where else do commands get deleted about from UndoTransaction::clear ?

lincoln

2009-08-24 23:14

reporter   ~0006599

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.

cth103

2009-08-24 23:55

administrator   ~0006603

Sorry, "about" should have been "apart" in my question!

actions.clear() will not call destructors for the Commands in the list.

lincoln

2009-08-25 00:07

reporter   ~0006604

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.

cth103

2009-08-25 00:12

administrator   ~0006605

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().

lincoln

2009-08-25 00:20

reporter   ~0006606

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.

paul

2009-09-05 13:10

administrator   ~0006646

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.

cth103

2010-08-14 15:20

administrator   ~0008836

Anyone remember where we got to with this?

cth103

2011-01-19 17:40

administrator   ~0009947

Should be fixed in SVN with a patch designed by Lincoln which allows nested undo commands.

system

2020-04-19 20:14

developer   ~0021980

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.

Issue History

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