COMMIT lifetime safety.

If we pass a COMMIT to a posted action, the COMMIT
may no longer exist when the action is run.
Equally problematic, if another COMMIT is pushed
in between we'd probably also run into trouble.

We still allow the API to do this because we don't
have a better solution at present.  But we need
one.

common/tool/tool_manager.cpp

@@ -328,7 +328,7 @@ VECTOR2D TOOL_MANAGER::GetCursorPosition() const
 
 
 bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki::any& aParam,
-                                COMMIT* aCommit )
+                                COMMIT* aCommit, bool aFromAPI )
 {
     if( m_shuttingDown )
         return true;
@@ -336,10 +336,6 @@ bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki:
     bool       retVal = false;
     TOOL_EVENT event = aAction.MakeEvent();
 
-    // We initialize the SYNCHRONOUS state to finished so that tools that don't have an event
-    // loop won't hang if someone forgets to set the state.
-    std::atomic<SYNCRONOUS_TOOL_STATE> synchronousControl = STS_FINISHED;
-
     if( event.Category() == TC_COMMAND )
         event.SetMousePosition( GetCursorPosition() );
 
@@ -347,20 +343,20 @@ bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki:
     if( aParam.has_value() )
         event.SetParameter( aParam );
 
-    // Pass the commit (if any)
-    if( aCommit )
-    {
-        event.SetSynchronous( &synchronousControl );
-        event.SetCommit( aCommit );
-    }
-
     if( aNow )
     {
         TOOL_STATE* current = m_activeState;
 
+        // An event with a commit must be run synchronously
         if( aCommit )
         {
-            // An event with a commit must be run synchronously
+            // We initialize the SYNCHRONOUS state to finished so that tools that don't have an
+            // event loop won't hang if someone forgets to set the state.
+            std::atomic<SYNCRONOUS_TOOL_STATE> synchronousControl = STS_FINISHED;
+
+            event.SetSynchronous( &synchronousControl );
+            event.SetCommit( aCommit );
+
             processEvent( event );
 
             while( synchronousControl == STS_RUNNING )
@@ -384,6 +380,17 @@ bool TOOL_MANAGER::doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki:
     }
     else
     {
+        // It is really dangerous to pass a commit (whose lifetime we can't guarantee) to
+        // deferred event processing.  There is a possibility that user actions will get run
+        // in between, which might either affect the lifetime of the commit or push or pop
+        // other commits.  However, we don't currently have a better solution for the API.
+        if( aCommit )
+        {
+            wxASSERT_MSG( aFromAPI, wxT( "Deferred actions have no way of guaranteeing the "
+                                         "lifetime of the COMMIT object" ) );
+            event.SetCommit( aCommit );
+        }
+
         PostEvent( event );
     }
 

include/tool/tool_manager.h

@@ -256,9 +256,10 @@ public:
      *
      * @param aAction is the action to be invoked.
      * @param aParam is an optional parameter that might be used by the invoked action. Its meaning
-     *               depends on the action.
+     *               depends on the action.  The parameter is not allowed to be a COMMIT as we have
+     *               no way of guaranteeing the lifetime of the COMMIT object.
      */
-    template<typename T>
+    template<typename T, std::enable_if_t<!std::is_convertible_v<T, COMMIT*>>* = nullptr>
     bool PostAction( const TOOL_ACTION& aAction, T aParam )
     {
         // Use a cast to ensure the proper type is stored inside the parameter
@@ -275,12 +276,15 @@ public:
         doRunAction( aAction, false, a, nullptr );
     }
 
-    bool PostAction( const TOOL_ACTION& aAction, COMMIT* aCommit )
+    // TODO: we need a way to guarantee that user events can't be processed in
+    // between API actions.  Otherwise the COMMITs will get tangled up and we'll
+    // crash on bad pointers.
+    bool PostAPIAction( const TOOL_ACTION& aAction, COMMIT* aCommit )
     {
         // Default initialize the parameter argument to an empty ki_any
         ki::any a;
 
-        return doRunAction( aAction, false, a, aCommit );
+        return doRunAction( aAction, false, a, aCommit, true );
     }
 
     /**
@@ -538,7 +542,7 @@ private:
      * Helper function to actually run an action.
      */
     bool doRunAction( const TOOL_ACTION& aAction, bool aNow, const ki::any& aParam,
-                      COMMIT* aCommit );
+                      COMMIT* aCommit, bool aFromAPI = false );
     bool doRunAction( const std::string& aActionName, bool aNow, const ki::any& aParam,
                       COMMIT* aCommit );
 

pcbnew/api/api_handler_pcb.cpp

@@ -1138,7 +1138,7 @@ HANDLER_RESULT<Empty> API_HANDLER_PCB::handleInteractiveMoveItems(
     mgr->RunAction<EDA_ITEMS*>( PCB_ACTIONS::selectItems, &toSelect );
 
     COMMIT* commit = getCurrentCommit( aCtx.ClientName );
-    mgr->PostAction( PCB_ACTIONS::move, commit );
+    mgr->PostAPIAction( PCB_ACTIONS::move, commit );
 
     return Empty();
 }