Gracefully shutdown tools when frames are closed

If the tools are not gracefully exited, then the stack variables are
never destroyed, so variable lifetime issues can occur.

Fixes https://gitlab.com/kicad/code/kicad/issues/1753

common/tool/tool_manager.cpp

@@ -55,17 +55,18 @@ struct TOOL_MANAGER::TOOL_STATE
 
     TOOL_STATE( const TOOL_STATE& aState )
     {
-        theTool = aState.theTool;
+        theTool            = aState.theTool;
-        idle = aState.idle;
+        idle               = aState.idle;
-        pendingWait = aState.pendingWait;
+        shutdown           = aState.shutdown;
+        pendingWait        = aState.pendingWait;
         pendingContextMenu = aState.pendingContextMenu;
-        contextMenu = aState.contextMenu;
+        contextMenu        = aState.contextMenu;
         contextMenuTrigger = aState.contextMenuTrigger;
-        cofunc = aState.cofunc;
+        cofunc             = aState.cofunc;
-        wakeupEvent = aState.wakeupEvent;
+        wakeupEvent        = aState.wakeupEvent;
-        waitEvents = aState.waitEvents;
+        waitEvents         = aState.waitEvents;
-        transitions = aState.transitions;
+        transitions        = aState.transitions;
-        vcSettings = aState.vcSettings;
+        vcSettings         = aState.vcSettings;
         // do not copy stateStack
     }
 
@@ -81,6 +82,9 @@ struct TOOL_MANAGER::TOOL_STATE
     /// Is the tool active (pending execution) or disabled at the moment
     bool idle;
 
+    /// Should the tool shutdown during next execution
+    bool shutdown;
+
     /// Flag defining if the tool is waiting for any event (i.e. if it
     /// issued a Wait() call).
     bool pendingWait;
@@ -112,17 +116,18 @@ struct TOOL_MANAGER::TOOL_STATE
 
     TOOL_STATE& operator=( const TOOL_STATE& aState )
     {
-        theTool = aState.theTool;
+        theTool            = aState.theTool;
-        idle = aState.idle;
+        idle               = aState.idle;
-        pendingWait = aState.pendingWait;
+        shutdown           = aState.shutdown;
+        pendingWait        = aState.pendingWait;
         pendingContextMenu = aState.pendingContextMenu;
-        contextMenu = aState.contextMenu;
+        contextMenu        = aState.contextMenu;
         contextMenuTrigger = aState.contextMenuTrigger;
-        cofunc = aState.cofunc;
+        cofunc             = aState.cofunc;
-        wakeupEvent = aState.wakeupEvent;
+        wakeupEvent        = aState.wakeupEvent;
-        waitEvents = aState.waitEvents;
+        waitEvents         = aState.waitEvents;
-        transitions = aState.transitions;
+        transitions        = aState.transitions;
-        vcSettings = aState.vcSettings;
+        vcSettings         = aState.vcSettings;
         // do not copy stateStack
         return *this;
     }
@@ -179,11 +184,12 @@ private:
     ///> Restores the initial state.
     void clear()
     {
-        idle = true;
+        idle               = true;
-        pendingWait = false;
+        shutdown           = false;
+        pendingWait        = false;
         pendingContextMenu = false;
-        cofunc = NULL;
+        cofunc             = NULL;
-        contextMenu = NULL;
+        contextMenu        = NULL;
         contextMenuTrigger = CMENU_OFF;
         vcSettings.Reset();
         transitions.clear();
@@ -429,6 +435,74 @@ bool TOOL_MANAGER::runTool( TOOL_BASE* aTool )
 }
 
 
+void TOOL_MANAGER::ShutdownAllTools()
+{
+    // Create a temporary list of tools to iterate over since when the tools shutdown
+    // they remove themselves from the list automatically (invalidating the iterator)
+    ID_LIST tmpList = m_activeTools;
+
+    for( auto id : tmpList )
+    {
+        ShutdownTool( id );
+    }
+}
+
+
+void TOOL_MANAGER::ShutdownTool( TOOL_ID aToolId )
+{
+    TOOL_BASE* tool = FindTool( aToolId );
+
+    if( tool && tool->GetType() == INTERACTIVE )
+        ShutdownTool( tool );
+}
+
+
+void TOOL_MANAGER::ShutdownTool( const std::string& aToolName )
+{
+    TOOL_BASE* tool = FindTool( aToolName );
+
+    if( tool && tool->GetType() == INTERACTIVE )
+        ShutdownTool( tool );
+}
+
+
+void TOOL_MANAGER::ShutdownTool( TOOL_BASE* aTool )
+{
+    wxASSERT( aTool != NULL );
+
+    TOOL_ID id = aTool->GetId();
+
+    if( isActive( aTool ) )
+    {
+        auto it = std::find( m_activeTools.begin(), m_activeTools.end(), id );
+
+        TOOL_STATE* st = m_toolIdIndex[*it];
+
+        // the tool state handler is waiting for events (i.e. called Wait() method)
+        if( st && st->pendingWait )
+        {
+            // Wake up the tool and tell it to shutdown
+            st->shutdown = true;
+            st->pendingWait = false;
+            st->waitEvents.clear();
+
+            if( st->cofunc )
+            {
+                wxLogTrace( kicadTraceToolStack,
+                        "TOOL_MANAGER::ShutdownTool is shutting down tool %s",
+                        st->theTool->GetName() );
+
+                setActiveState( st );
+                bool end = !st->cofunc->Resume();
+
+                if( end )
+                    it = finishTool( st );
+            }
+        }
+    }
+}
+
+
 TOOL_BASE* TOOL_MANAGER::FindTool( int aId ) const
 {
     std::map<TOOL_ID, TOOL_STATE*>::const_iterator it = m_toolIdIndex.find( aId );
@@ -560,7 +634,11 @@ TOOL_EVENT* TOOL_MANAGER::ScheduleWait( TOOL_BASE* aTool, const TOOL_EVENT_LIST&
     // switch context back to event dispatcher loop
     st->cofunc->KiYield();
 
-    return &st->wakeupEvent;
+    // If the tool should shutdown, it gets a null event to break the loop
+    if( st->shutdown )
+        return nullptr;
+    else
+        return &st->wakeupEvent;
 }
 
 

cvpcb/cvpcb_mainframe.cpp

@@ -185,9 +185,9 @@ CVPCB_MAINFRAME::CVPCB_MAINFRAME( KIWAY* aKiway, wxWindow* aParent ) :
 
 CVPCB_MAINFRAME::~CVPCB_MAINFRAME()
 {
-    // Be sure a active tool (if exists) is deactivated:
+    // Shutdown all running tools
     if( m_toolManager )
-        m_toolManager->DeactivateTool();
+        m_toolManager->ShutdownAllTools();
 
     // Clean up the tool infrastructure
     delete m_actions;

cvpcb/display_footprints_frame.cpp

@@ -152,16 +152,16 @@ DISPLAY_FOOTPRINTS_FRAME::DISPLAY_FOOTPRINTS_FRAME( KIWAY* aKiway, wxWindow* aPa
 
 DISPLAY_FOOTPRINTS_FRAME::~DISPLAY_FOOTPRINTS_FRAME()
 {
+    // Shutdown all running tools
+    if( m_toolManager )
+        m_toolManager->ShutdownAllTools();
+
     GetBoard()->DeleteAllModules();
     GetCanvas()->StopDrawing();
     GetCanvas()->GetView()->Clear();
     // Be sure any event cannot be fired after frame deletion:
     GetCanvas()->SetEvtHandlerEnabled( false );
 
-    // Be sure a active tool (if exists) is deactivated:
-    if( m_toolManager )
-        m_toolManager->DeactivateTool();
-
     delete GetScreen();
     SetScreen( NULL );      // Be sure there is no double deletion
 }

cvpcb/tools/cvpcb_control.cpp

@@ -89,9 +89,6 @@ int CVPCB_CONTROL::Main( const TOOL_EVENT& aEvent )
             evt->SetPassEvent();
     }
 
-    // This tool is supposed to be active forever
-    wxASSERT( false );
-
     return 0;
 }
 

cvpcb/tools/cvpcb_fpviewer_selection_tool.cpp

@@ -92,9 +92,6 @@ int CVPCB_FOOTPRINT_VIEWER_SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
             evt->SetPassEvent();
     }
 
-    // This tool is supposed to be active forever
-    wxASSERT( false );
-
     return 0;
 }
 

eeschema/lib_view_frame.cpp

@@ -211,6 +211,10 @@ LIB_VIEW_FRAME::LIB_VIEW_FRAME( KIWAY* aKiway, wxWindow* aParent, FRAME_T aFrame
 
 LIB_VIEW_FRAME::~LIB_VIEW_FRAME()
 {
+    // Shutdown all running tools
+    if( m_toolManager )
+        m_toolManager->ShutdownAllTools();
+
     if( m_previewItem )
         GetCanvas()->GetView()->Remove( m_previewItem );
 }

eeschema/libedit/lib_edit_frame.cpp

@@ -196,6 +196,10 @@ LIB_EDIT_FRAME::LIB_EDIT_FRAME( KIWAY* aKiway, wxWindow* aParent ) :
 
 LIB_EDIT_FRAME::~LIB_EDIT_FRAME()
 {
+    // Shutdown all running tools
+    if( m_toolManager )
+        m_toolManager->ShutdownAllTools();
+
     // current screen is destroyed in EDA_DRAW_FRAME
     SetScreen( m_dummyScreen );
 

eeschema/sch_edit_frame.cpp

@@ -311,6 +311,10 @@ SCH_EDIT_FRAME::SCH_EDIT_FRAME( KIWAY* aKiway, wxWindow* aParent ):
 
 SCH_EDIT_FRAME::~SCH_EDIT_FRAME()
 {
+    // Shutdown all running tools
+    if( m_toolManager )
+        m_toolManager->ShutdownAllTools();
+
     delete m_item_to_repeat;        // we own the cloned object, see this->SaveCopyForRepeatItem()
 
     SetScreen( NULL );

eeschema/tools/ee_selection_tool.cpp

@@ -403,9 +403,6 @@ int EE_SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
             evt->SetPassEvent();
     }
 
-    // This tool is supposed to be active forever
-    assert( false );
-
     return 0;
 }
 

gerbview/gerbview_frame.cpp

@@ -222,6 +222,10 @@ GERBVIEW_FRAME::GERBVIEW_FRAME( KIWAY* aKiway, wxWindow* aParent ):
 
 GERBVIEW_FRAME::~GERBVIEW_FRAME()
 {
+    // Shutdown all running tools
+    if( m_toolManager )
+        m_toolManager->ShutdownAllTools();
+
     GetCanvas()->GetView()->Clear();
 
     GetGerberLayout()->GetImagesList()->DeleteAllImages();

gerbview/tools/gerbview_selection_tool.cpp

@@ -236,9 +236,6 @@ int GERBVIEW_SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
             evt->SetPassEvent();
     }
 
-    // This tool is supposed to be active forever
-    assert( false );
-
     return 0;
 }
 

include/tool/tool_manager.h

@@ -94,6 +94,36 @@ public:
      */
     bool InvokeTool( const std::string& aToolName );
 
+    /**
+     * Shutdown all tools with a currently registered event loop in this tool manager
+     * by waking them up with a null event.
+     */
+    void ShutdownAllTools();
+
+    /**
+     * Shutdown the specified tool by waking it up with a null event to terminate
+     * the processing loop.
+     *
+     * @param aTool is the tool to shutdown
+     */
+    void ShutdownTool( TOOL_BASE* aTool );
+
+    /**
+     * Shutdown the specified tool by waking it up with a null event to terminate
+     * the processing loop.
+     *
+     * @param aToolId is the ID of the tool to shutdown
+     */
+    void ShutdownTool( TOOL_ID aToolId );
+
+    /**
+     * Shutdown the specified tool by waking it up with a null event to terminate
+     * the processing loop.
+     *
+     * @param aToolName is name of the tool to shutdown
+     */
+    void ShutdownTool( const std::string& aToolName );
+
     /**
      * Function RunAction()
      * Runs the specified action. The common format for action names is "application.ToolName.Action".

kicad/kicad_manager_frame.cpp

@@ -166,9 +166,9 @@ KICAD_MANAGER_FRAME::KICAD_MANAGER_FRAME( wxWindow* parent, const wxString& titl
 
 KICAD_MANAGER_FRAME::~KICAD_MANAGER_FRAME()
 {
-    // Ensure there are no active tools
+    // Shutdown all running tools
     if( m_toolManager )
-        m_toolManager->DeactivateTool();
+        m_toolManager->ShutdownAllTools();
 
     delete m_actions;
     delete m_toolManager;

pagelayout_editor/pl_editor_frame.cpp

@@ -200,6 +200,10 @@ PL_EDITOR_FRAME::PL_EDITOR_FRAME( KIWAY* aKiway, wxWindow* aParent ) :
 
 PL_EDITOR_FRAME::~PL_EDITOR_FRAME()
 {
+    // Shutdown all running tools
+    if( m_toolManager )
+        m_toolManager->ShutdownAllTools();
+
     // Since the file menu contains file history menus, we must ensure that the menu
     // destructor is called before the file history objects are deleted since their destructor
     // unregisters the menu from the history.

pagelayout_editor/tools/pl_selection_tool.cpp

@@ -193,9 +193,6 @@ int PL_SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
             evt->SetPassEvent();
     }
 
-    // This tool is supposed to be active forever
-    assert( false );
-
     return 0;
 }
 

pcbnew/footprint_edit_frame.cpp

@@ -233,6 +233,10 @@ FOOTPRINT_EDIT_FRAME::FOOTPRINT_EDIT_FRAME( KIWAY* aKiway, wxWindow* aParent,
 
 FOOTPRINT_EDIT_FRAME::~FOOTPRINT_EDIT_FRAME()
 {
+    // Shutdown all running tools
+    if( m_toolManager )
+        m_toolManager->ShutdownAllTools();
+
     // save the footprint in the PROJECT
     retainLastFootprint();
 

pcbnew/footprint_viewer_frame.cpp

@@ -271,6 +271,10 @@ FOOTPRINT_VIEWER_FRAME::FOOTPRINT_VIEWER_FRAME( KIWAY* aKiway, wxWindow* aParent
 
 FOOTPRINT_VIEWER_FRAME::~FOOTPRINT_VIEWER_FRAME()
 {
+    // Shutdown all running tools
+    if( m_toolManager )
+        m_toolManager->ShutdownAllTools();
+
     GetCanvas()->StopDrawing();
     GetCanvas()->GetView()->Clear();
     // Be sure any event cannot be fired after frame deletion:

pcbnew/pcb_edit_frame.cpp

@@ -339,6 +339,10 @@ PCB_EDIT_FRAME::PCB_EDIT_FRAME( KIWAY* aKiway, wxWindow* aParent ) :
 
 PCB_EDIT_FRAME::~PCB_EDIT_FRAME()
 {
+    // Shutdown all running tools
+    if( m_toolManager )
+        m_toolManager->ShutdownAllTools();
+
     // Since the file menu contains file history menus, we must ensure that the menu
     // destructor is called before the file history objects are deleted since their destructor
     // unregisters the menu from the history.

pcbnew/tools/selection_tool.cpp

@@ -287,9 +287,6 @@ int SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
             evt->SetPassEvent();
     }
 
-    // This tool is supposed to be active forever
-    assert( false );
-
     return 0;
 }