fix circular cleanup in TransformState and RenderState

panda/src/collide/collisionSolid.cxx

@@ -388,7 +388,6 @@ get_solid_viz_state() {
       (CullFaceAttrib::make(CullFaceAttrib::M_cull_clockwise),
        RenderModeAttrib::make(RenderModeAttrib::M_filled),
        TransparencyAttrib::make(TransparencyAttrib::M_alpha));
-    base_state->ref();  // once more to guard against static destruction
   }
 
   if (_tangible) {
@@ -396,7 +395,6 @@ get_solid_viz_state() {
     if (tangible_state == (const RenderState *)NULL) {
       tangible_state = base_state->add_attrib
         (ColorAttrib::make_flat(Colorf(1.0f, 1.0f, 1.0f, 0.5f)));
-      tangible_state->ref();
     }
     return tangible_state;
 
@@ -405,7 +403,6 @@ get_solid_viz_state() {
     if (intangible_state == (const RenderState *)NULL) {
       intangible_state = base_state->add_attrib
         (ColorAttrib::make_flat(Colorf(1.0f, 0.3f, 0.5f, 0.5f)));
-      intangible_state->ref();
     }
     return intangible_state;
   }
@@ -430,7 +427,6 @@ get_wireframe_viz_state() {
       (CullFaceAttrib::make(CullFaceAttrib::M_cull_none),
        RenderModeAttrib::make(RenderModeAttrib::M_wireframe),
        TransparencyAttrib::make(TransparencyAttrib::M_none));
-    base_state->ref();  // once more to guard against static destruction
   }
 
   if (_tangible) {
@@ -438,7 +434,6 @@ get_wireframe_viz_state() {
     if (tangible_state == (const RenderState *)NULL) {
       tangible_state = base_state->add_attrib
         (ColorAttrib::make_flat(Colorf(0.0f, 0.0f, 1.0f, 1.0f)));
-      tangible_state->ref();
     }
     return tangible_state;
 
@@ -447,7 +442,6 @@ get_wireframe_viz_state() {
     if (intangible_state == (const RenderState *)NULL) {
       intangible_state = base_state->add_attrib
         (ColorAttrib::make_flat(Colorf(1.0f, 1.0f, 0.0f, 1.0f)));
-      intangible_state->ref();
     }
     return intangible_state;
   }
@@ -471,7 +465,6 @@ get_other_viz_state() {
       (CullFaceAttrib::make(CullFaceAttrib::M_cull_clockwise),
        RenderModeAttrib::make(RenderModeAttrib::M_filled),
        TransparencyAttrib::make(TransparencyAttrib::M_alpha));
-    base_state->ref();  // once more to guard against static destruction
   }
 
   // We don't bother to make a distinction here between tangible and

panda/src/glgsg/glGraphicsStateGuardian.cxx

@@ -3794,7 +3794,6 @@ get_unlit_state() {
   static CPT(RenderState) state = NULL;
   if (state == (const RenderState *)NULL) {
     state = RenderState::make(LightAttrib::make_all_off());
-    state->ref();
   }
   return state;
 }

panda/src/pgraph/cullTraverserData.cxx

@@ -157,7 +157,6 @@ get_fake_view_frustum_cull_state() {
        TextureAttrib::make_off(),
        RenderModeAttrib::make(RenderModeAttrib::M_wireframe),
        1000);
-    state->ref();  // once more to guard against static destruction
   }
   return state;
 }

panda/src/pgraph/qpcullTraverser.cxx

@@ -264,7 +264,6 @@ get_bounds_outer_viz_state() {
       (ColorAttrib::make_flat(Colorf(0.3f, 1.0f, 0.5f, 1.0f)),
        RenderModeAttrib::make(RenderModeAttrib::M_wireframe),
        CullFaceAttrib::make(CullFaceAttrib::M_cull_clockwise));
-    state->ref();  // once more to guard against static destruction
   }
   return state;
 }
@@ -285,7 +284,6 @@ get_bounds_inner_viz_state() {
       (ColorAttrib::make_flat(Colorf(0.15f, 0.5f, 0.25f, 1.0f)),
        RenderModeAttrib::make(RenderModeAttrib::M_wireframe),
        CullFaceAttrib::make(CullFaceAttrib::M_cull_counter_clockwise));
-    state->ref();  // once more to guard against static destruction
   }
   return state;
 }
@@ -304,7 +302,6 @@ get_depth_offset_state() {
   if (state == (const RenderState *)NULL) {
     state = RenderState::make
       (DepthOffsetAttrib::make(1));
-    state->ref();  // once more to guard against static destruction
   }
   return state;
 }

panda/src/pgraph/renderEffect.cxx

@@ -154,7 +154,6 @@ return_new(RenderEffect *effect) {
     // The effect was inserted; save the iterator and return the
     // input effect.
     effect->_saved_entry = result.first;
-    effect->ref();  // **** TEMPORARY HACK
     return pt_effect;
   }
 

panda/src/pgraph/renderState.I

@@ -17,6 +17,26 @@
 ////////////////////////////////////////////////////////////////////
 
 
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Composition::Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::Composition::
+Composition() {
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::Composition::Copy Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE RenderState::Composition::
+Composition(const RenderState::Composition &copy) :
+  _result(copy._result)
+{
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderState::Attribute::Constructor
 //       Access: Public
@@ -235,3 +255,37 @@ get_transparency() const {
   }
   return _transparency;
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::set_destructing
+//       Access: Private
+//  Description: This function should only be called from the
+//               destructor; it indicates that this RenderState
+//               object is beginning destruction.  It is only used as
+//               a sanity check, and is only meaningful when NDEBUG is
+//               not defined.
+////////////////////////////////////////////////////////////////////
+INLINE void RenderState::
+set_destructing() {
+#ifndef NDEBUG
+  _flags |= F_is_destructing;
+#endif
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::is_destructing
+//       Access: Private
+//  Description: Returns true if the RenderState object is
+//               currently within its destructor
+//               (i.e. set_destructing() has been called).  This is
+//               only used as a sanity check, and is only meaningful
+//               when NDEBUG is not defined.
+////////////////////////////////////////////////////////////////////
+INLINE bool RenderState::
+is_destructing() const {
+#ifndef NDEBUG
+  return (_flags & F_is_destructing) != 0;
+#else
+  return false;
+#endif
+}

panda/src/pgraph/renderState.cxx

@@ -84,6 +84,10 @@ operator = (const RenderState &) {
 ////////////////////////////////////////////////////////////////////
 RenderState::
 ~RenderState() {
+  // We'd better not call the destructor twice on a particular object.
+  nassertv(!is_destructing());
+  set_destructing();
+
   if (_saved_entry != _states->end()) {
     nassertv(_states->find(this) == _saved_entry);
     _states->erase(_saved_entry);
@@ -99,52 +103,116 @@ RenderState::
   // cache: it's the same set of RenderState objects that we have in
   // our own cache.
 
-  // We do need to put some thought into this loop, because as we
-  // clear out cache entries we'll cause other RenderState objects to
-  // destruct, which could cause things to get pulled out of our own
-  // _composition_cache map.  We don't want to get bitten by this
-  // cascading effect.
-  CompositionCache::iterator ci;
-  ci = _composition_cache.begin();
-  while (ci != _composition_cache.end()) {
-    {
-      PT(RenderState) other = (RenderState *)(*ci).first;
+  // We do need to put considerable thought into this loop, because as
+  // we clear out cache entries we'll cause other RenderState
+  // objects to destruct, which could cause things to get pulled out
+  // of our own _composition_cache map.  We want to allow this (so
+  // that we don't encounter any just-destructed pointers in our
+  // cache), but we don't want to get bitten by this cascading effect.
+  // Instead of walking through the map from beginning to end,
+  // therefore, we just pull out the first one each time, and erase
+  // it.
+
+  // There are lots of ways to do this loop wrong.  Be very very
+  // careful if you need to modify it for any reason.
+  while (!_composition_cache.empty()) {
+    CompositionCache::iterator ci = _composition_cache.begin();
+    if ((*ci).first->get_ref_count() == 0) {
+      // If the reference count of the other one is zero, it must be
+      // in the middle of destructing (but not yet completely
+      // destructed, or it wouldn't even be here any more).  This can
+      // happen because of the cascading effects of these destructors.
+      nassertv((*ci).first->is_destructing());
+
+      // We can't use a PT() to hold a pointer to a destructing
+      // object; that would likely call its destructor twice.
+      RenderState *other = (RenderState *)(*ci).first;
       Composition comp = (*ci).second;
+      _composition_cache.erase(ci);
 
       // We should never have a reflexive entry in this map.  If we
       // do, something got screwed up elsewhere.
       nassertv(other != (const RenderState *)this);
+      nassertv(comp._result == (RenderState *)NULL ||
+               !comp._result->is_destructing());
+
+      CompositionCache::iterator oci = other->_composition_cache.find(this);
+
+      // Since the other one is in the middle of destructing, we may
+      // or may not still be listed in its cache.
+      if (oci != other->_composition_cache.end()) {
+        Composition ocomp = (*oci).second;
+        nassertv(ocomp._result == (RenderState *)NULL ||
+                 !ocomp._result->is_destructing());
+        
+        // Now we're holding a reference count to both computed
+        // results, so no objects will be tempted to destruct while we
+        // clear the pointer here.
+        other->_composition_cache.erase(oci);
+      }
+
+    } else {
+      // If the other one hasn't yet started to destruct, hold its
+      // reference count now, so it won't start to destruct until
+      // we're done with this operation.
+      PT(RenderState) other = (RenderState *)(*ci).first;
+      Composition comp = (*ci).second;
+      _composition_cache.erase(ci);
+
+      nassertv(other != (const RenderState *)this);
+      nassertv(!other->is_destructing());
+      nassertv(comp._result == (RenderState *)NULL ||
+               !comp._result->is_destructing());
+
+      CompositionCache::iterator oci = other->_composition_cache.find(this);
+
+      // Since the other one is not destructing yet, we should still
+      // be listed in its cache.
+      nassertv(oci != other->_composition_cache.end());
+      Composition ocomp = (*oci).second;
+      nassertv(ocomp._result == (RenderState *)NULL ||
+               !ocomp._result->is_destructing());
       
-      // Now we're holding a reference count to the other state, as well
-      // as to the computed result (if any), so neither object will be
-      // tempted to destruct.  Go ahead and remove ourselves from the
-      // other cache.
-      nassertv(other->_composition_cache.count(this) == 1);
-      other->_composition_cache.erase(this);
-
-      // It's all right if the other state destructs now, since it
-      // won't try to remove itself from our own composition cache any
-      // more.  Someone might conceivably delete the *next* entry,
-      // though, so we should be sure to let all that deleting finish
-      // up before we attempt to increment ci, by closing the scope
-      // here.
+      // All reference counts are now held; clear the pointer.
+      other->_composition_cache.erase(oci);
     }
-    // Now it's safe to increment ci, because the current cache entry
-    // has not gone away, and if the next one has, by now it's safely
-    // gone.
-    ++ci;
   }
 
   // A similar bit of code for the invert cache.
-  ci = _invert_composition_cache.begin();
-  while (ci != _invert_composition_cache.end()) {
-    {
+  while (!_invert_composition_cache.empty()) {
+    CompositionCache::iterator ci = _invert_composition_cache.begin();
+    if ((*ci).first->get_ref_count() == 0) {
+      nassertv((*ci).first->is_destructing());
+      RenderState *other = (RenderState *)(*ci).first;
+      Composition comp = (*ci).second;
+      _invert_composition_cache.erase(ci);
+      nassertv(other != (const RenderState *)this);
+      nassertv(comp._result == (RenderState *)NULL ||
+               !comp._result->is_destructing());
+      CompositionCache::iterator oci = 
+        other->_invert_composition_cache.find(this);
+      if (oci != other->_invert_composition_cache.end()) {
+        Composition ocomp = (*oci).second;
+        nassertv(ocomp._result == (RenderState *)NULL ||
+                 !ocomp._result->is_destructing());
+        other->_invert_composition_cache.erase(oci);
+      }
+    } else {
       PT(RenderState) other = (RenderState *)(*ci).first;
       Composition comp = (*ci).second;
+      _invert_composition_cache.erase(ci);
       nassertv(other != (const RenderState *)this);
-      other->_invert_composition_cache.erase(this);
+      nassertv(!other->is_destructing());
+      nassertv(comp._result == (RenderState *)NULL ||
+               !comp._result->is_destructing());
+      CompositionCache::iterator oci = 
+        other->_invert_composition_cache.find(this);
+      nassertv(oci != other->_invert_composition_cache.end());
+      Composition ocomp = (*oci).second;
+      nassertv(ocomp._result == (RenderState *)NULL ||
+               !ocomp._result->is_destructing());
+      other->_invert_composition_cache.erase(oci);
     }
-    ++ci;
   }
 
   // Also, if we called compose(this) at some point and the return

panda/src/pgraph/renderState.h

@@ -110,6 +110,9 @@ private:
   void determine_fog();
   void determine_transparency();
 
+  INLINE void set_destructing();
+  INLINE bool is_destructing() const;
+
 private:
   typedef pset<const RenderState *, IndirectLess<RenderState> > States;
   static States *_states;
@@ -127,6 +130,9 @@ private:
   // one in each of the two involved RenderState objects.
   class Composition {
   public:
+    INLINE Composition();
+    INLINE Composition(const Composition &copy);
+
     CPT(RenderState) _result;
   };
     
@@ -176,8 +182,9 @@ private:
     F_checked_bin_index    = 0x0001,
     F_checked_fog          = 0x0002,
     F_checked_transparency = 0x0004,
+    F_is_destructing       = 0x8000,
   };
-  short _flags;
+  unsigned short _flags;
 
 public:
   static void register_with_read_factory();

panda/src/pgraph/transformState.I

@@ -17,6 +17,26 @@
 ////////////////////////////////////////////////////////////////////
 
 
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::Composition::Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE TransformState::Composition::
+Composition() {
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::Composition::Copy Constructor
+//       Access: Public
+//  Description: 
+////////////////////////////////////////////////////////////////////
+INLINE TransformState::Composition::
+Composition(const TransformState::Composition &copy) :
+  _result(copy._result)
+{
+}
+
 ////////////////////////////////////////////////////////////////////
 //     Function: TransformState::make_pos
 //       Access: Published, Static
@@ -302,3 +322,37 @@ check_mat() const {
     ((TransformState *)this)->calc_mat();
   }
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::set_destructing
+//       Access: Private
+//  Description: This function should only be called from the
+//               destructor; it indicates that this TransformState
+//               object is beginning destruction.  It is only used as
+//               a sanity check, and is only meaningful when NDEBUG is
+//               not defined.
+////////////////////////////////////////////////////////////////////
+INLINE void TransformState::
+set_destructing() {
+#ifndef NDEBUG
+  _flags |= F_is_destructing;
+#endif
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::is_destructing
+//       Access: Private
+//  Description: Returns true if the TransformState object is
+//               currently within its destructor
+//               (i.e. set_destructing() has been called).  This is
+//               only used as a sanity check, and is only meaningful
+//               when NDEBUG is not defined.
+////////////////////////////////////////////////////////////////////
+INLINE bool TransformState::
+is_destructing() const {
+#ifndef NDEBUG
+  return (_flags & F_is_destructing) != 0;
+#else
+  return false;
+#endif
+}

panda/src/pgraph/transformState.cxx

@@ -79,6 +79,10 @@ operator = (const TransformState &) {
 ////////////////////////////////////////////////////////////////////
 TransformState::
 ~TransformState() {
+  // We'd better not call the destructor twice on a particular object.
+  nassertv(!is_destructing());
+  set_destructing();
+
   // Free the inverse matrix computation, if it has been stored.
   if (_inv_mat != (LMatrix4f *)NULL) {
     delete _inv_mat;
@@ -100,51 +104,116 @@ TransformState::
   // cache: it's the same set of TransformState objects that we have in
   // our own cache.
 
-  // We do need to put some thought into this loop, because as we
-  // clear out cache entries we'll cause other TransformState objects to
-  // destruct, which could cause things to get pulled out of our own
-  // _composition_cache map.  We don't want to get bitten by this
-  // cascading effect.
-  CompositionCache::iterator ci;
-  ci = _composition_cache.begin();
-  while (ci != _composition_cache.end()) {
-    {
-      PT(TransformState) other = (TransformState *)(*ci).first;
+  // We do need to put considerable thought into this loop, because as
+  // we clear out cache entries we'll cause other TransformState
+  // objects to destruct, which could cause things to get pulled out
+  // of our own _composition_cache map.  We want to allow this (so
+  // that we don't encounter any just-destructed pointers in our
+  // cache), but we don't want to get bitten by this cascading effect.
+  // Instead of walking through the map from beginning to end,
+  // therefore, we just pull out the first one each time, and erase
+  // it.
+
+  // There are lots of ways to do this loop wrong.  Be very very
+  // careful if you need to modify it for any reason.
+  while (!_composition_cache.empty()) {
+    CompositionCache::iterator ci = _composition_cache.begin();
+    if ((*ci).first->get_ref_count() == 0) {
+      // If the reference count of the other one is zero, it must be
+      // in the middle of destructing (but not yet completely
+      // destructed, or it wouldn't even be here any more).  This can
+      // happen because of the cascading effects of these destructors.
+      nassertv((*ci).first->is_destructing());
+
+      // We can't use a PT() to hold a pointer to a destructing
+      // object; that would likely call its destructor twice.
+      TransformState *other = (TransformState *)(*ci).first;
       Composition comp = (*ci).second;
+      _composition_cache.erase(ci);
 
       // We should never have a reflexive entry in this map.  If we
       // do, something got screwed up elsewhere.
       nassertv(other != (const TransformState *)this);
+      nassertv(comp._result == (TransformState *)NULL ||
+               !comp._result->is_destructing());
+
+      CompositionCache::iterator oci = other->_composition_cache.find(this);
+
+      // Since the other one is in the middle of destructing, we may
+      // or may not still be listed in its cache.
+      if (oci != other->_composition_cache.end()) {
+        Composition ocomp = (*oci).second;
+        nassertv(ocomp._result == (TransformState *)NULL ||
+                 !ocomp._result->is_destructing());
+        
+        // Now we're holding a reference count to both computed
+        // results, so no objects will be tempted to destruct while we
+        // clear the pointer here.
+        other->_composition_cache.erase(oci);
+      }
+
+    } else {
+      // If the other one hasn't yet started to destruct, hold its
+      // reference count now, so it won't start to destruct until
+      // we're done with this operation.
+      PT(TransformState) other = (TransformState *)(*ci).first;
+      Composition comp = (*ci).second;
+      _composition_cache.erase(ci);
+
+      nassertv(other != (const TransformState *)this);
+      nassertv(!other->is_destructing());
+      nassertv(comp._result == (TransformState *)NULL ||
+               !comp._result->is_destructing());
+
+      CompositionCache::iterator oci = other->_composition_cache.find(this);
+
+      // Since the other one is not destructing yet, we should still
+      // be listed in its cache.
+      nassertv(oci != other->_composition_cache.end());
+      Composition ocomp = (*oci).second;
+      nassertv(ocomp._result == (TransformState *)NULL ||
+               !ocomp._result->is_destructing());
       
-      // Now we're holding a reference count to the other state, as well
-      // as to the computed result (if any), so neither object will be
-      // tempted to destruct.  Go ahead and remove ourselves from the
-      // other cache.
-      other->_composition_cache.erase(this);
-
-      // It's all right if the other state destructs now, since it
-      // won't try to remove itself from our own composition cache any
-      // more.  Someone might conceivably delete the *next* entry,
-      // though, so we should be sure to let all that deleting finish
-      // up before we attempt to increment ci, by closing the scope
-      // here.
+      // All reference counts are now held; clear the pointer.
+      other->_composition_cache.erase(oci);
     }
-    // Now it's safe to increment ci, because the current cache entry
-    // has not gone away, and if the next one has, by now it's safely
-    // gone.
-    ++ci;
   }
 
   // A similar bit of code for the invert cache.
-  ci = _invert_composition_cache.begin();
-  while (ci != _invert_composition_cache.end()) {
-    {
+  while (!_invert_composition_cache.empty()) {
+    CompositionCache::iterator ci = _invert_composition_cache.begin();
+    if ((*ci).first->get_ref_count() == 0) {
+      nassertv((*ci).first->is_destructing());
+      TransformState *other = (TransformState *)(*ci).first;
+      Composition comp = (*ci).second;
+      _invert_composition_cache.erase(ci);
+      nassertv(other != (const TransformState *)this);
+      nassertv(comp._result == (TransformState *)NULL ||
+               !comp._result->is_destructing());
+      CompositionCache::iterator oci = 
+        other->_invert_composition_cache.find(this);
+      if (oci != other->_invert_composition_cache.end()) {
+        Composition ocomp = (*oci).second;
+        nassertv(ocomp._result == (TransformState *)NULL ||
+                 !ocomp._result->is_destructing());
+        other->_invert_composition_cache.erase(oci);
+      }
+    } else {
       PT(TransformState) other = (TransformState *)(*ci).first;
       Composition comp = (*ci).second;
+      _invert_composition_cache.erase(ci);
       nassertv(other != (const TransformState *)this);
-      other->_invert_composition_cache.erase(this);
+      nassertv(!other->is_destructing());
+      nassertv(comp._result == (TransformState *)NULL ||
+               !comp._result->is_destructing());
+      CompositionCache::iterator oci = 
+        other->_invert_composition_cache.find(this);
+      nassertv(oci != other->_invert_composition_cache.end());
+      Composition ocomp = (*oci).second;
+      nassertv(ocomp._result == (TransformState *)NULL ||
+               !ocomp._result->is_destructing());
+      other->_invert_composition_cache.erase(oci);
     }
-    ++ci;
   }
 
   // Also, if we called compose(this) at some point and the return
@@ -153,6 +222,10 @@ TransformState::
   if (_self_compose != (TransformState *)NULL && _self_compose != this) {
     unref_delete((TransformState *)_self_compose);
   }
+
+  // If this was true at the beginning of the destructor, but is no
+  // longer true now, probably we've been double-deleted.
+  nassertv(get_ref_count() == 0);
 }
 
 ////////////////////////////////////////////////////////////////////

panda/src/pgraph/transformState.h

@@ -124,6 +124,9 @@ private:
   // one in each of the two involved TransformState objects.
   class Composition {
   public:
+    INLINE Composition();
+    INLINE Composition(const Composition &copy);
+
     CPT(TransformState) _result;
   };
     
@@ -145,21 +148,25 @@ private:
   void calc_components();
   void calc_mat();
 
+  INLINE void set_destructing();
+  INLINE bool is_destructing() const;
+
   enum Flags {
-    F_is_identity      =  0x0001,
-    F_is_singular      =  0x0002,
-    F_singular_known   =  0x0004,
-    F_components_given =  0x0008,
-    F_components_known =  0x0010,
-    F_has_components   =  0x0020,
-    F_mat_known        =  0x0040,
-    F_is_invalid       =  0x0080,
+    F_is_identity      = 0x0001,
+    F_is_singular      = 0x0002,
+    F_singular_known   = 0x0004,
+    F_components_given = 0x0008,
+    F_components_known = 0x0010,
+    F_has_components   = 0x0020,
+    F_mat_known        = 0x0040,
+    F_is_invalid       = 0x0080,
+    F_is_destructing   = 0x8000,
   };
   LVecBase3f _pos, _hpr, _scale;
   LMatrix4f _mat;
   LMatrix4f *_inv_mat;
   
-  short _flags;
+  unsigned short _flags;
 
 public:
   static void register_with_read_factory();