[gparted] Change OperationDetail to not store complex objects in STL containers (#729139)



commit 947cd028575ceb9eee0e7facc2c1749397e50aa9
Author: Phillip Susi <psusi ubuntu com>
Date:   Tue Apr 1 22:04:01 2014 -0400

    Change OperationDetail to not store complex objects in STL containers (#729139)
    
    OperationDetail was storing its children in a std::vector.  This means they
    can be moved around in memory arbitrarily, going through indeterminate
    lifetimes.  This is generally a bad thing for any non trivial object and
    in the case of OperationDetail, it created havoc with the way it maintains
    pointers between parent/child objects for signal connections.  It will now
    keep only pointers to children in a std::vector instead, so their lifetime
    can be controlled, fixing various crashes.
    
    Bug 729139 - Refactor OperationDetail to address random behavior

 include/Dialog_Progress.h |    2 +-
 include/OperationDetail.h |    6 +++---
 src/Dialog_Progress.cc    |    6 +++---
 src/FileSystem.cc         |    6 +++---
 src/OperationDetail.cc    |   28 +++++++++++++++++-----------
 5 files changed, 27 insertions(+), 21 deletions(-)
---
diff --git a/include/Dialog_Progress.h b/include/Dialog_Progress.h
index d680ced..a3c88e7 100644
--- a/include/Dialog_Progress.h
+++ b/include/Dialog_Progress.h
@@ -89,7 +89,7 @@ private:
        treeview_operations_Columns treeview_operations_columns;
        
        std::vector<Operation *> operations ;
-       OperationDetail operationdetail ;
+       Glib::ustring progress_text;
        bool succes, cancel;
        double fraction ;
        unsigned int t, warnings ;
diff --git a/include/OperationDetail.h b/include/OperationDetail.h
index 290f17d..e8fb802 100644
--- a/include/OperationDetail.h
+++ b/include/OperationDetail.h
@@ -60,8 +60,8 @@ public:
        Glib::ustring get_elapsed_time() const ;
        
        void add_child( const OperationDetail & operationdetail ) ;
-       std::vector<OperationDetail> & get_childs() ;
-       const std::vector<OperationDetail> & get_childs() const ;
+       std::vector<OperationDetail*> & get_childs() ;
+       const std::vector<OperationDetail*> & get_childs() const ;
        OperationDetail & get_last_child() ;
 
        double fraction ;
@@ -78,7 +78,7 @@ private:
 
        Glib::ustring treepath ;
        
-       std::vector<OperationDetail> sub_details ;      
+       std::vector<OperationDetail*> sub_details;
        std::time_t time_start, time_elapsed ;
        sigc::connection cancelconnection;
 };
diff --git a/src/Dialog_Progress.cc b/src/Dialog_Progress.cc
index f1ee542..9526e64 100644
--- a/src/Dialog_Progress.cc
+++ b/src/Dialog_Progress.cc
@@ -148,7 +148,7 @@ void Dialog_Progress::on_signal_update( const OperationDetail & operationdetail
                }
 
                //update the gui elements..
-               this ->operationdetail = operationdetail ;
+               progress_text = operationdetail.progress_text;
 
                if ( operationdetail .get_status() == STATUS_EXECUTE )
                        label_current_sub_text = operationdetail .get_description() ;
@@ -182,7 +182,7 @@ void Dialog_Progress::update_gui_elements()
        label_current_sub .set_markup( "<i>" + label_current_sub_text + "</i>\n" ) ;
        
        //To ensure progress bar height remains the same, add a space in case message is empty
-       progressbar_current .set_text( operationdetail .progress_text + " " ) ;
+       progressbar_current.set_text( progress_text + " " );
 }
 
 bool Dialog_Progress::pulsebar_pulse()
@@ -458,7 +458,7 @@ void Dialog_Progress::echo_operation_details( const OperationDetail & operationd
                << "<td>" << std::endl ;
 
                for ( unsigned int t = 0 ; t <  operationdetail .get_childs() .size() ; t++ )
-                       echo_operation_details( operationdetail .get_childs()[ t ], out ) ;
+                       echo_operation_details( *(operationdetail.get_childs()[ t ]), out );
 
                out << "</td>" << std::endl << "</tr>" << std::endl ;
        }
diff --git a/src/FileSystem.cc b/src/FileSystem.cc
index 0a092c1..71bb246 100644
--- a/src/FileSystem.cc
+++ b/src/FileSystem.cc
@@ -118,12 +118,12 @@ int FileSystem::execute_command( const Glib::ustring & command, OperationDetail
                OperationDetail( output, STATUS_NONE, FONT_ITALIC ) );
        operationdetail.get_last_child().add_child(
                OperationDetail( error, STATUS_NONE, FONT_ITALIC ) );
-       std::vector<OperationDetail> &children = operationdetail.get_last_child().get_childs();
+       std::vector<OperationDetail*> &children = operationdetail.get_last_child().get_childs();
        outputcapture.signal_update.connect( sigc::bind( sigc::ptr_fun( update_command_output ),
-                                                        &(children[children.size() - 2]),
+                                                        children[children.size() - 2],
                                                         &output ) );
        errorcapture.signal_update.connect( sigc::bind( sigc::ptr_fun( update_command_output ),
-                                                       &(children[children.size() - 1]),
+                                                       children[children.size() - 1],
                                                        &error ) );
        outputcapture.connect_signal();
        errorcapture.connect_signal();
diff --git a/src/OperationDetail.cc b/src/OperationDetail.cc
index d08cba2..34d5a1e 100644
--- a/src/OperationDetail.cc
+++ b/src/OperationDetail.cc
@@ -36,6 +36,11 @@ OperationDetail::OperationDetail( const Glib::ustring & description, OperationDe
 OperationDetail::~OperationDetail()
 {
        cancelconnection.disconnect();
+       while (sub_details.size())
+       {
+               delete sub_details.back();
+               sub_details.pop_back();
+       }
 }
 
 void OperationDetail::set_description( const Glib::ustring & description, Font font )
@@ -121,23 +126,23 @@ Glib::ustring OperationDetail::get_elapsed_time() const
 
 void OperationDetail::add_child( const OperationDetail & operationdetail ) 
 {
-       sub_details .push_back( operationdetail ) ;
+       sub_details .push_back( new OperationDetail(operationdetail) );
 
-       sub_details .back() .set_treepath( treepath + ":" + Utils::num_to_str( sub_details .size() -1 ) ) ;
-       sub_details .back() .signal_update .connect( sigc::mem_fun( this, &OperationDetail::on_update ) ) ;
-       sub_details.back().cancelconnection = signal_cancel.connect(
-                               sigc::mem_fun( &sub_details.back(), &OperationDetail::cancel ) );
+       sub_details.back()->set_treepath( treepath + ":" + Utils::num_to_str( sub_details .size() -1 ) );
+       sub_details.back()->signal_update.connect( sigc::mem_fun( this, &OperationDetail::on_update ) );
+       sub_details.back()->cancelconnection = signal_cancel.connect(
+                               sigc::mem_fun( sub_details.back(), &OperationDetail::cancel ) );
        if ( cancelflag )
-               sub_details.back().cancel( cancelflag == 2 );
-       on_update( sub_details .back() ) ;
+               sub_details.back()->cancel( cancelflag == 2 );
+       on_update( *sub_details.back() );
 }
        
-std::vector<OperationDetail> & OperationDetail::get_childs() 
+std::vector<OperationDetail*> & OperationDetail::get_childs()
 {
        return sub_details ;
 }
 
-const std::vector<OperationDetail> & OperationDetail::get_childs() const
+const std::vector<OperationDetail*> & OperationDetail::get_childs() const
 {
        return sub_details ;
 }
@@ -148,7 +153,7 @@ OperationDetail & OperationDetail::get_last_child()
        if ( sub_details .size() == 0 )
                add_child( OperationDetail( "---", STATUS_ERROR ) ) ;
 
-       return sub_details[sub_details.size() - 1];
+       return *sub_details[sub_details.size() - 1];
 }
 
 void OperationDetail::on_update( const OperationDetail & operationdetail ) 
@@ -161,7 +166,8 @@ void OperationDetail::cancel( bool force )
 {
        if ( force )
                cancelflag = 2;
-       else cancelflag = 1;
+       else
+               cancelflag = 1;
        signal_cancel(force);
 }
 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]