From 6ab7f68f4b81884eaa1e568c20d5c4f216e85c68 Mon Sep 17 00:00:00 2001
From: James Turner <zakalawe@mac.com>
Date: Fri, 4 Sep 2020 10:51:32 +0100
Subject: [PATCH] =?UTF-8?q?Bindings:=20don=E2=80=99t=20cache=20the=20comma?=
 =?UTF-8?q?nd=20pointer?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Caching it complicates add/remove command logic, so making a simple
fix for now, which can be back-ported to 2020.2; ideally we would
cache the pointer but have an invalidation scheme, but that’s
considerably more work and risk.

Relates to Sentry crash:
https://sentry.io/organizations/flightgear/issues/1858764364
---
 simgear/structure/SGBinding.cxx | 56 ++++++++++++++-------------------
 simgear/structure/SGBinding.hxx | 11 -------
 2 files changed, 23 insertions(+), 44 deletions(-)

diff --git a/simgear/structure/SGBinding.cxx b/simgear/structure/SGBinding.cxx
index d8992e9a..0a917122 100644
--- a/simgear/structure/SGBinding.cxx
+++ b/simgear/structure/SGBinding.cxx
@@ -18,24 +18,16 @@
 #include <simgear/structure/exception.hxx>
 
 SGBinding::SGBinding()
-  : _command(0),
-    _arg(new SGPropertyNode),
-    _setting(0)
+    : _arg(new SGPropertyNode)
 {
 }
 
 SGBinding::SGBinding(const std::string& commandName)
-    : _command(0),
-    _arg(0),
-    _setting(0)
 {
     _command_name = commandName;
 }
 
 SGBinding::SGBinding(const SGPropertyNode* node, SGPropertyNode* root)
-  : _command(0),
-    _arg(0),
-    _setting(0)
 {
   read(node, root);
 }
@@ -43,8 +35,8 @@ SGBinding::SGBinding(const SGPropertyNode* node, SGPropertyNode* root)
 void
 SGBinding::clear()
 {
-    _command = NULL;
     _arg.clear();
+    _root.clear();
     _setting.clear();
 }
 
@@ -57,14 +49,12 @@ SGBinding::read(const SGPropertyNode* node, SGPropertyNode* root)
 
   _command_name = node->getStringValue("command", "");
   if (_command_name.empty()) {
-    SG_LOG(SG_INPUT, SG_WARN, "No command supplied for binding.");
-    _command = 0;
+      SG_LOG(SG_INPUT, SG_DEV_ALERT, "No command supplied for binding.");
   }
 
   _arg = const_cast<SGPropertyNode*>(node);
   _root = const_cast<SGPropertyNode*>(root);
-  _setting = 0;
-
+  _setting.clear();
 }
 
 void
@@ -78,30 +68,29 @@ SGBinding::fire() const
 void
 SGBinding::innerFire () const
 {
-  if (_command == 0)
-    _command = SGCommandMgr::instance()->getCommand(_command_name);
-  if (_command == 0) {
-    SG_LOG(SG_INPUT, SG_WARN, "No command attached to binding:" << _command_name);
-  } else {
-      try {
-          if (!(*_command)(_arg, _root)) {
-                SG_LOG(SG_INPUT, SG_ALERT, "Failed to execute command "
-                       << _command_name);
-          }
-      } catch (sg_exception& e) {
+    auto cmd = SGCommandMgr::instance()->getCommand(_command_name);
+    if (!cmd) {
+        SG_LOG(SG_INPUT, SG_WARN, "No command found for binding:" << _command_name);
+        return;
+    }
+
+    try {
+        if (!(*cmd)(_arg, _root)) {
+            SG_LOG(SG_INPUT, SG_ALERT, "Failed to execute command " << _command_name);
+        }
+    } catch (sg_exception& e) {
         SG_LOG(SG_GENERAL, SG_ALERT, "command '" << _command_name << "' failed with exception\n"
-          << "\tmessage:" << e.getMessage() << " (from " << e.getOrigin() << ")");
-      }
-  }
+                                                 << "\tmessage:" << e.getMessage() << " (from " << e.getOrigin() << ")");
+    }
 }
 
 void
 SGBinding::fire (SGPropertyNode* params) const
 {
   if (test()) {
-    if (params != NULL) {
-      copyProperties(params, _arg);
-    }
+      if (params != nullptr) {
+          copyProperties(params, _arg);
+      }
 
     innerFire();
   }
@@ -122,8 +111,9 @@ SGBinding::fire (double setting) const
   if (test()) {
                                 // A value is automatically added to
                                 // the args
-    if (_setting == 0)          // save the setting node for efficiency
-      _setting = _arg->getChild("setting", 0, true);
+                                if (!_setting) { // save the setting node for efficiency
+                                    _setting = _arg->getChild("setting", 0, true);
+                                }
     _setting->setDoubleValue(setting);
     innerFire();
   }
diff --git a/simgear/structure/SGBinding.hxx b/simgear/structure/SGBinding.hxx
index ed3a951c..054566a6 100644
--- a/simgear/structure/SGBinding.hxx
+++ b/simgear/structure/SGBinding.hxx
@@ -77,16 +77,6 @@ public:
    */
   const std::string &getCommandName () const { return _command_name; }
 
-
-  /**
-   * Get the command itself.
-   *
-   * @return The command associated with this binding, or 0 if none
-   * is present.
-   */
-  SGCommandMgr::Command* getCommand () const { return _command; }
-
-
   /**
    * Get the argument that will be passed to the command.
    *
@@ -140,7 +130,6 @@ private:
   SGBinding (const SGBinding &binding);
 
   std::string _command_name;
-  mutable SGCommandMgr::Command* _command;
   mutable SGPropertyNode_ptr _arg;
   mutable SGPropertyNode_ptr _setting;
   mutable SGPropertyNode_ptr _root;