Partial fix for crash in SGPropertyNode::fireValueChanged

The effect system used Listeners on property nodes to get the values
for shader uniforms. These listeners get deleted by an osg thread
causing access to freed memory when this happens while the main thread
calls fireValueChanged.

This patch changes the update method to polling for scalar properties.
This isn't 100% threadsafe, too. But at least it does not crash anymore.

diff --git a/simgear/scene/material/Effect.cxx b/simgear/scene/material/Effect.cxx
index 7d961af47f51d3ed14e2b3332fd902aa5b0f9c32..f94be67ac615270af87030291aa0640f98f9a1e7 100644 (file)
--- a/simgear/scene/material/Effect.cxx
+++ b/simgear/scene/material/Effect.cxx
@@ -1356,6 +1356,28 @@ void Effect::InitializeCallback::doUpdate(osg::Node* node, osg::NodeVisitor* nv)
     }
 }
 
+void Effect::UpdateCallback::operator()(osg::Node* node, osg::NodeVisitor* nv)
+{
+    EffectGeode* eg = dynamic_cast<EffectGeode*>(node);
+    if (!eg)
+        return;
+    Effect* effect = eg->getEffect();
+    if (!effect)
+        return;
+
+    for (vector<SGSharedPtr<Updater> >::iterator itr = effect->_extraData.begin(),
+             end = effect->_extraData.end();
+         itr != end;
+         ++itr) {
+        PropertyPoller * poller
+            = dynamic_cast<PropertyPoller*>(itr->ptr());
+        if (poller)
+            poller->pollProperties(effect);
+    }
+
+    traverse(node, nv);
+}
+
 bool Effect::Key::EqualTo::operator()(const Effect::Key& lhs,
                                       const Effect::Key& rhs) const
 {

diff --git a/simgear/scene/material/Effect.hxx b/simgear/scene/material/Effect.hxx
index 0966dc1aa63e8b7d9bcba911b90e44af12cf4f64..346c52c2c39aa1674ac34f2800fee870725c282a 100644 (file)
--- a/simgear/scene/material/Effect.hxx
+++ b/simgear/scene/material/Effect.hxx
@@ -73,6 +73,17 @@ private:
     bool _initialized;
 };
 
+class PropertyPoller
+{
+public:
+    PropertyPoller()  {};
+    virtual ~PropertyPoller() {};
+    virtual void pollProperties(Effect* effect)
+    {
+    }
+private:
+};
+
 class Effect : public osg::Object
 {
 public:
@@ -121,6 +132,17 @@ public:
     {
         void doUpdate(osg::Node* node, osg::NodeVisitor* nv);
     };
+    friend struct UpdateCallback;
+    struct UpdateCallback : public osg::NodeCallback
+    {
+        UpdateCallback() {}
+        UpdateCallback(const UpdateCallback& nc, const osg::CopyOp& copyop)
+        : osg::NodeCallback(nc, copyop)
+        {
+        }
+
+        virtual void operator()(osg::Node* node, osg::NodeVisitor* nv);
+    };
 protected:
     std::vector<SGSharedPtr<Updater> > _extraData;
     ~Effect();

diff --git a/simgear/scene/material/EffectBuilder.hxx b/simgear/scene/material/EffectBuilder.hxx
index a919f10b3398d5380e28197e40d0b0e80777f3a3..52e2cf1c98614a2e0c4dee3ea65ff3bf679d7584 100644 (file)
--- a/simgear/scene/material/EffectBuilder.hxx
+++ b/simgear/scene/material/EffectBuilder.hxx
@@ -496,19 +496,17 @@ make_OSGFunctor(Obj* obj, void (Obj::*const func)(const OSGParam&))
 template<typename OSGParamType, typename ObjType, typename F>
 class ScalarChangeListener
     : public SGPropertyChangeListener, public InitializeWhenAdded,
+      public PropertyPoller,
       public Effect::Updater
 {
 public:
     ScalarChangeListener(ObjType* obj, const F& setter,
                          const std::string& propName)
-        : _obj(obj), _setter(setter)
+        : _obj(obj), _setter(setter), _propName(propName)
     {
-        _propName = new std::string(propName);
     }
     virtual ~ScalarChangeListener()
     {
-        delete _propName;
-        _propName = 0;
     }
     void valueChanged(SGPropertyNode* node)
     {
@@ -516,16 +514,20 @@ public:
     }
     void initOnAddImpl(Effect* effect, SGPropertyNode* propRoot)
     {
-        SGPropertyNode* listenProp = makeNode(propRoot, *_propName);
-        delete _propName;
-        _propName = 0;
-        if (listenProp)
-            listenProp->addChangeListener(this, true);
+        _listenProp = makeNode(propRoot, _propName);
+//        if ( _listenProp.valid() )
+//            _listenProp->addChangeListener(this, true);
+    }
+    void pollProperties(Effect* effect)
+    {
+        if( false == _listenProp.valid() ) return;
+        valueChanged(_listenProp);
     }
 private:
+    SGPropertyNode_ptr _listenProp;
     osg::ref_ptr<ObjType> _obj;
     F _setter;
-    std::string* _propName;
+    std::string _propName;
 };
 
 template<typename T, typename Func>

diff --git a/simgear/scene/material/EffectGeode.cxx b/simgear/scene/material/EffectGeode.cxx
index c296763928ae90d712183ed06820e07d12f1691e..2355bb67f78750ad5acd79139b1769073bca7303 100644 (file)
--- a/simgear/scene/material/EffectGeode.cxx
+++ b/simgear/scene/material/EffectGeode.cxx
@@ -52,7 +52,9 @@ void EffectGeode::setEffect(Effect* effect)
     _effect = effect;
     if (!_effect)
         return;
+    //TODO: do we leak the callbacks or does the geode own pointer afterwards?
     addUpdateCallback(new Effect::InitializeCallback);
+    addUpdateCallback(new Effect::UpdateCallback);
 }
 
 void EffectGeode::resizeGLObjectBuffers(unsigned int maxSize)