#553: MP-model-loading related segfault

MP aircraft are loaded by a separate OSG thread (introduced after FG2.4.0).
The OSG thread also calls the "modelLoaded" callback. However, we mustn't
allow the OSG thread to call FGNasalModelData::modelLoaded directly:
FGNasalModelData isn't thread-safe. There are obvious issues (_callCount++;),
tricky issues like calling the Nasal _parser_ or creating hashes and
modifying the global Nasal namespace. It doesn't use locks to protect
against another thread executing a Nasal context or running garbage
collection. It also executes Nasal code itself (the model's "load" hook),
which we also cannot allow in a separate thread...
This patch returns all Nasal parts of MP-aircraft loading (parsing,
module creation, execution) to the main thread, while keeping the
multi-threaded OSG part (loading of MP-aircraft model files itself).
The same issue exists with scenery models (see other commit).

To summarize with 2 words: It s*cks... ;-)

diff --git a/src/AIModel/AIBase.cxx b/src/AIModel/AIBase.cxx
index 07fc6c31a49c38f86883ec0ee03ffd0da94efb70..a9d64e341683d619fa4cc78132faf1cb9c0720c3 100644 (file)
--- a/src/AIModel/AIBase.cxx
+++ b/src/AIModel/AIBase.cxx
@@ -75,8 +75,7 @@ FGAIBase::FGAIBase(object_type ot, bool enableHot) :
     _refID( _newAIModelID() ),
     _otype(ot),
     _initialized(false),
-    _aimodel(0),
-    _fxpath(""),
+    _modeldata(0),
     _fx(0)
 
 {
@@ -232,21 +231,28 @@ void FGAIBase::update(double dt) {
                             pitch*speed );
         _fx->set_velocity( velocity );
     }
-    else if ((_aimodel)&&(fgGetBool("/sim/sound/aimodels/enabled",false)))
+    else if ((_modeldata)&&(_modeldata->needInitilization()))
     {
-        string fxpath = _aimodel->get_sound_path();
-        if (fxpath != "")
+        // process deferred nasal initialization,
+        // which must be done in main thread
+        _modeldata->init();
+
+        // sound initialization
+        if (fgGetBool("/sim/sound/aimodels/enabled",false))
         {
-            _fxpath = fxpath;
-            props->setStringValue("sim/sound/path", _fxpath.c_str());
-
-            // initialize the sound configuration
-            SGSoundMgr *smgr = globals->get_soundmgr();
-            stringstream name;
-            name <<  "aifx:";
-            name << _refID;
-            _fx = new FGFX(smgr, name.str(), props);
-            _fx->init();
+            string fxpath = _modeldata->get_sound_path();
+            if (fxpath != "")
+            {
+                props->setStringValue("sim/sound/path", fxpath.c_str());
+
+                // initialize the sound configuration
+                SGSoundMgr *smgr = globals->get_soundmgr();
+                stringstream name;
+                name <<  "aifx:";
+                name << _refID;
+                _fx = new FGFX(smgr, name.str(), props);
+                _fx->init();
+            }
         }
     }
 }
@@ -324,8 +330,8 @@ bool FGAIBase::init(bool search_in_AI_path)
     else
         _installed = true;
 
-    _aimodel = new FGAIModelData(props);
-    osg::Node * mdl = SGModelLib::loadDeferredModel(f, props, _aimodel);
+    _modeldata = new FGAIModelData(props);
+    osg::Node * mdl = SGModelLib::loadDeferredModel(f, props, _modeldata);
 
     _model = new osg::LOD;
     _model->setName("AI-model range animation node");
@@ -912,7 +918,8 @@ int FGAIBase::_newAIModelID() {
 
 FGAIModelData::FGAIModelData(SGPropertyNode *root)
   : _nasal( new FGNasalModelData(root) ),
-    _path("")
+    _ready(false),
+    _initialized(false)
 {
 }
 
@@ -923,9 +930,21 @@ FGAIModelData::~FGAIModelData()
 
 void FGAIModelData::modelLoaded(const string& path, SGPropertyNode *prop, osg::Node *n)
 {
-    const char* fxpath = prop->getStringValue("sound/path");
-    if (fxpath) {
-        _path = string(fxpath);
-    }
-    _nasal->modelLoaded(path, prop, n);
+    // WARNING: All this is called in a separate OSG thread! Only use thread-safe stuff
+    // here that is fine to be run concurrently with stuff in the main loop!
+    if (_ready)
+        return;
+    _fxpath = _prop->getStringValue("sound/path");
+    _prop = prop;
+    _path = path;
+    _ready = true;
+}
+
+// do Nasal initialization (must be called in the main loop)
+void FGAIModelData::init()
+{
+    // call FGNasalSys to create context and run hooks (not-thread safe!)
+    _nasal->modelLoaded(_path, _prop, 0);
+    _prop = 0;
+    _initialized = true;
 }

diff --git a/src/AIModel/AIBase.hxx b/src/AIModel/AIBase.hxx
index b4815c41ad3b57dc5b4d9c2eca01bb99f336a79b..b4782eb4374cc6fdc8b34d062227d35f3b9141cd 100644 (file)
--- a/src/AIModel/AIBase.hxx
+++ b/src/AIModel/AIBase.hxx
@@ -21,7 +21,6 @@
 #define _FG_AIBASE_HXX
 
 #include <string>
-#include <list>
 
 #include <simgear/constants.h>
 #include <simgear/math/SGMath.hxx>
@@ -38,7 +37,6 @@
 
 
 using std::string;
-using std::list;
 
 class SGMaterial;
 class FGAIManager;
@@ -230,9 +228,8 @@ private:
     bool _initialized;
     osg::ref_ptr<osg::LOD> _model; //The 3D model LOD object
 
-    osg::ref_ptr<FGAIModelData> _aimodel;
+    osg::ref_ptr<FGAIModelData> _modeldata;
 
-    string _fxpath;
     SGSharedPtr<FGFX>  _fx;
 
 public:
@@ -453,12 +450,24 @@ class FGAIModelData : public simgear::SGModelData {
 public:
     FGAIModelData(SGPropertyNode *root = 0);
     ~FGAIModelData();
+
+    /** osg callback, thread-safe */
     void modelLoaded(const string& path, SGPropertyNode *prop, osg::Node *n);
-    inline string& get_sound_path() { return _path; };
+
+    /** init hook to be called after model is loaded.
+     * Not thread-safe. Call from main thread only. */
+    void init(void);
+
+    bool needInitilization(void) { return _ready && !_initialized;}
+    bool isInitialized(void) { return _initialized;}
+    inline std::string& get_sound_path() { return _fxpath;}
 
 private:
     FGNasalModelData *_nasal;
-    string _path;
+    SGPropertyNode_ptr _prop;
+    std::string _path, _fxpath;
+    bool _ready;
+    bool _initialized;
 };
 
 #endif // _FG_AIBASE_HXX