fix a few crashes and deadlocks

panda/src/pgraph/billboardEffect.cxx

@@ -234,10 +234,10 @@ compare_to_impl(const RenderEffect *other) const {
   DCAST_INTO_R(ta, other, 0);
 
   if (_axial_rotate != ta->_axial_rotate) {
-    return _axial_rotate - ta->_axial_rotate;
+    return (int)_axial_rotate - (int)ta->_axial_rotate;
   }
   if (_eye_relative != ta->_eye_relative) {
-    return _eye_relative - ta->_eye_relative;
+    return (int)_eye_relative - (int)ta->_eye_relative;
   }
   if (_offset != ta->_offset) {
     return _offset < ta->_offset ? -1 : 1;

panda/src/pgraph/clipPlaneAttrib.cxx

@@ -970,8 +970,17 @@ complete_pointers(TypedWritable **p_list, BamReader *manager) {
   while (ci != _off_planes.end()) {
     PandaNode *node;
     DCAST_INTO_R(node, p_list[pi++], pi);
-    NodePath np(node);
-    (*ci) = areg->lookup_node(np);
+    
+    // We go through some effort to look up the node in the registry
+    // without creating a NodePath around it first (which would up,
+    // and then down, the reference count, possibly deleting the
+    // node).
+    int ni = areg->find_node(node->get_type(), node->get_name());
+    if (ni != -1) {
+      (*ci) = areg->get_node(ni);
+    } else {
+      (*ci) = NodePath(node);
+    }
     ++ci;
   }
   _off_planes.sort();
@@ -980,8 +989,13 @@ complete_pointers(TypedWritable **p_list, BamReader *manager) {
   while (ci != _on_planes.end()) {
     PandaNode *node;
     DCAST_INTO_R(node, p_list[pi++], pi);
-    NodePath np(node);
-    (*ci) = areg->lookup_node(np);
+
+    int ni = areg->find_node(node->get_type(), node->get_name());
+    if (ni != -1) {
+      (*ci) = areg->get_node(ni);
+    } else {
+      (*ci) = NodePath(node);
+    }
     ++ci;
   }
   _on_planes.sort();

panda/src/pgraph/pandaNode.cxx

@@ -30,6 +30,7 @@
 #include "boundingBox.h"
 #include "pStatTimer.h"
 #include "config_mathutil.h"
+#include "reMutexHolder.h"
 
 // This category is just temporary for debugging convenience.
 NotifyCategoryDecl(drawmask, EXPCL_PANDA, EXPTP_PANDA);
@@ -1768,8 +1769,8 @@ replace_node(PandaNode *other) {
 
   // Fix up the NodePaths.
   {
-    MutexHolder holder1(other->_paths_lock);
-    MutexHolder holder2(_paths_lock);
+    ReMutexHolder holder1(other->_paths_lock);
+    ReMutexHolder holder2(_paths_lock);
     Paths::iterator pi;
     for (pi = other->_paths.begin(); pi != other->_paths.end(); ++pi) {
       (*pi)->_node = this;
@@ -2792,7 +2793,7 @@ attach(NodePathComponent *parent, PandaNode *child_node, int sort,
     PT(NodePathComponent) child = 
       new NodePathComponent(child_node, (NodePathComponent *)NULL,
                             pipeline_stage, current_thread);
-    MutexHolder holder(child_node->_paths_lock);
+    ReMutexHolder holder(child_node->_paths_lock);
     child_node->_paths.insert(child);
     return child;
   }
@@ -2997,7 +2998,7 @@ reparent_one_stage(NodePathComponent *new_parent, NodePathComponent *child,
 #ifndef NDEBUG
       // The NodePathComponent should already be in the set.
       {
-        MutexHolder holder(child_node->_paths_lock);
+        ReMutexHolder holder(child_node->_paths_lock);
         nassertr(child_node->_paths.find(child) != child_node->_paths.end(), false);
       }
 #endif // NDEBUG
@@ -3023,7 +3024,7 @@ get_component(NodePathComponent *parent, PandaNode *child_node,
   nassertr(parent != (NodePathComponent *)NULL, (NodePathComponent *)NULL);
   PandaNode *parent_node = parent->get_node();
 
-  MutexHolder holder(child_node->_paths_lock);
+  ReMutexHolder holder(child_node->_paths_lock);
 
   // First, walk through the list of NodePathComponents we already
   // have on the child, looking for one that already exists,
@@ -3070,7 +3071,7 @@ get_component(NodePathComponent *parent, PandaNode *child_node,
 PT(NodePathComponent) PandaNode::
 get_top_component(PandaNode *child_node, bool force, int pipeline_stage, 
                   Thread *current_thread) {
-  MutexHolder holder(child_node->_paths_lock);
+  ReMutexHolder holder(child_node->_paths_lock);
 
   // Walk through the list of NodePathComponents we already have on
   // the child, looking for one that already exists as a top node.
@@ -3182,7 +3183,7 @@ r_get_generic_component(bool accept_ambiguity, bool &ambiguity_detected,
 ////////////////////////////////////////////////////////////////////
 void PandaNode::
 delete_component(NodePathComponent *component) {
-  MutexHolder holder(_paths_lock);
+  ReMutexHolder holder(_paths_lock);
   int num_erased = _paths.erase(component);
   nassertv(num_erased == 1);
 }
@@ -3209,7 +3210,7 @@ void PandaNode::
 sever_connection(PandaNode *parent_node, PandaNode *child_node,
                  int pipeline_stage, Thread *current_thread) {
   {
-    MutexHolder holder(child_node->_paths_lock);
+    ReMutexHolder holder(child_node->_paths_lock);
     Paths::iterator pi;
     for (pi = child_node->_paths.begin(); pi != child_node->_paths.end(); ++pi) {
       if (!(*pi)->is_top_node(pipeline_stage, current_thread) && 
@@ -3242,7 +3243,7 @@ void PandaNode::
 new_connection(PandaNode *parent_node, PandaNode *child_node,
                int pipeline_stage, Thread *current_thread) {
   {
-    MutexHolder holder(child_node->_paths_lock);
+    ReMutexHolder holder(child_node->_paths_lock);
     Paths::iterator pi;
     for (pi = child_node->_paths.begin(); pi != child_node->_paths.end(); ++pi) {
       if ((*pi)->is_top_node(pipeline_stage, current_thread)) {
@@ -3267,7 +3268,7 @@ new_connection(PandaNode *parent_node, PandaNode *child_node,
 ////////////////////////////////////////////////////////////////////
 void PandaNode::
 fix_path_lengths(int pipeline_stage, Thread *current_thread) {
-  MutexHolder holder(_paths_lock);
+  ReMutexHolder holder(_paths_lock);
 
   bool any_wrong = false;
 

panda/src/pgraph/pandaNode.h

@@ -48,6 +48,7 @@
 #include "pStatCollector.h"
 #include "copyOnWriteObject.h"
 #include "copyOnWritePointer.h"
+#include "reMutex.h"
 
 #ifdef HAVE_PYTHON
 
@@ -419,7 +420,7 @@ private:
   // threads.  A NodePathComponent, once created, is always associated
   // with the same node.  We do, however, protect the Paths under a mutex.
   Paths _paths;
-  Mutex _paths_lock;
+  ReMutex _paths_lock;
 
   bool _dirty_prev_transform;
   static PandaNodeChain _dirty_prev_transforms;

panda/src/pgraph/renderEffects.I

@@ -96,7 +96,7 @@ operator < (const Effect &other) const {
 //       Access: Public
 //  Description: Provides an indication of whether a particular
 //               effect is equivalent to another one, for purposes
-//               of generating unique RenderEffectss.  This should
+//               of generating unique RenderEffects.  This should
 //               compare all properties of the Effect, but it is
 //               important that the type is compared first, to be
 //               consistent with the ordering defined by operator <.

panda/src/pgraph/renderEffects.cxx

@@ -56,7 +56,7 @@ RenderEffects() : _lock("RenderEffects") {
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderEffects::Copy Constructor
 //       Access: Private
-//  Description: RenderEffectss are not meant to be copied.
+//  Description: RenderEffects are not meant to be copied.
 ////////////////////////////////////////////////////////////////////
 RenderEffects::
 RenderEffects(const RenderEffects &) {
@@ -66,7 +66,7 @@ RenderEffects(const RenderEffects &) {
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderEffects::Copy Assignment Operator
 //       Access: Private
-//  Description: RenderEffectss are not meant to be copied.
+//  Description: RenderEffects are not meant to be copied.
 ////////////////////////////////////////////////////////////////////
 void RenderEffects::
 operator = (const RenderEffects &) {
@@ -181,7 +181,7 @@ xform(const LMatrix4f &mat) const {
 //     Function: RenderEffects::operator <
 //       Access: Published
 //  Description: Provides an arbitrary ordering among all unique
-//               RenderEffectss, so we can store the essentially
+//               RenderEffects, so we can store the essentially
 //               different ones in a big set and throw away the rest.
 //
 //               This method is not needed outside of the RenderEffects
@@ -193,9 +193,32 @@ bool RenderEffects::
 operator < (const RenderEffects &other) const {
   // We must compare all the properties of the effects, not just
   // the type; thus, we compare them one at a time using compare_to().
-  return lexicographical_compare(_effects.begin(), _effects.end(),
-                                 other._effects.begin(), other._effects.end(),
-                                 CompareTo<Effect>());
+
+  // We could use STL's lexicographical_compare() function here, but
+  // it seems to generate aberrant behavior on OSX and Linux.  We
+  // suspect a compiler bug in gcc.  Instead, we implement the
+  // comparison by hand.
+  Effects::const_iterator ai = _effects.begin();
+  Effects::const_iterator bi = other._effects.begin();
+  while (ai != _effects.end() && bi != other._effects.end()) {
+    int compare = (*ai).compare_to(*bi);
+    if (compare != 0) {
+      return compare < 0;
+    }
+    ++ai;
+    ++bi;
+  }
+  if (ai != _effects.end()) {
+    // bi ran out first..
+    return false;
+  }
+  if (bi != other._effects.end()) {
+    // ai ran out first.
+    return true;
+  }
+
+  // Lists are equivalent.
+  return false;
 }
 
 
@@ -475,7 +498,7 @@ get_num_states() {
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderEffects::list_states
 //       Access: Published, Static
-//  Description: Lists all of the RenderEffectss in the cache to the
+//  Description: Lists all of the RenderEffects in the cache to the
 //               output stream, one per line.  This can be quite a lot
 //               of output if the cache is large, so be prepared.
 ////////////////////////////////////////////////////////////////////
@@ -639,9 +662,13 @@ return_new(RenderEffects *state) {
     // The state was inserted; save the iterator and return the
     // input state.
     state->_saved_entry = result.first;
+    //    nassertr(validate_states(), pt_state);
+    nassertr(_states->find(state) == state->_saved_entry, pt_state);
     return pt_state;
   }
 
+  //  nassertr(validate_states(), *(result.first));
+
   // The state was not inserted; there must be an equivalent one
   // already in the set.  Return that one.
   return *(result.first);
@@ -666,6 +693,13 @@ release_new() {
       nassertv(*_saved_entry == this);
       cerr << "States wrong!\n";
       cerr << "validate = " << validate_states() << "\n";
+      cerr << "this = " << this << ": " << *this << "\n";
+      States::iterator fi = _states->find(this);
+      if (fi == _states->end()) {
+        cerr << "  not found\n";
+      } else {
+        cerr <<"   found: " << (*fi) << ": " << *(*fi) << "\n";
+      }
 
       if (!_states->empty()) {
         States::iterator si;
@@ -673,6 +707,10 @@ release_new() {
         cerr << (*si) << ": " << *(*si) << "\n";
         States::iterator ni = si;
         ++ni;
+        if ((*si) == this) {
+          _states->erase(si);
+          si = ni;
+        }
         while (ni != _states->end()) {
           if (*(*si) < *(*ni)) {
             cerr << "  ok, " << (*(*ni) < *(*si)) << "\n";
@@ -682,13 +720,24 @@ release_new() {
           si = ni;
           cerr << (*si) << ": " << *(*si) << "\n";
           ++ni;
+          if ((*si) == this) {
+            _states->erase(si);
+            si = ni;
+          }
         }
       }
     }
     */
     nassertv(_states->find(this) == _saved_entry);
+    //    nassertv(validate_states());
     _states->erase(_saved_entry);
     _saved_entry = _states->end();
+
+    /*
+    nassertd(validate_states()) {
+      cerr << "Removed " << this << ": " << *this << "\n";
+    }
+    */
   }
 }
 
@@ -837,7 +886,6 @@ complete_pointers(TypedWritable **p_list, BamReader *manager) {
     nassertr(effect._effect != (RenderEffect *)NULL, pi);
     effect._type = effect._effect->get_type();
   }
-  ReMutexHolder holder(*_states_lock);
 
   // Now make sure the array is properly sorted.  (It won't
   // necessarily preserve its correct sort after being read from bam,
@@ -845,6 +893,7 @@ complete_pointers(TypedWritable **p_list, BamReader *manager) {
   // from session to session.)
   _effects.sort();
 
+  nassertr(_saved_entry == _states->end(), pi);
   return pi;
 }
 
@@ -963,4 +1012,6 @@ fillin(DatagramIterator &scan, BamReader *manager) {
     manager->read_pointer(scan);
     _effects.push_back(Effect());
   }
+
+  nassertv(_saved_entry == _states->end());
 }