express: weakptr.lock() should return null during object destruction

This came up in #330; the Character destructor caused something to call lock() on a weak pointer to that character, which would induce a ref() and unref() pair, but since the refcount was 0, this would call the destructor and thereby create infinite recursion.

I considered instead calling mark_deleted() inside unref() so that the callbacks get to run before the object is actually deleted, and was_deleted() will become true as soon as unref() reaches 0.  However, this would require grabbing the lock in unref() to be fully thread-safe, since we would need to bring the refcount to 0 and mark the object as deleted in one atomic operation, so this would be an unacceptable general performance penalty.

Instead, WeakPointerTo::lock() now atomically increments the reference count if it is not already zero, and returns null otherwise.  This should be safe because the object cannot be deleted while the WeakReferenceList lock is held.

panda/src/express/referenceCount.I

@@ -300,6 +300,27 @@ weak_unref() {
   nassertv(nonzero);
 }
 
+/**
+ * Atomically increases the reference count of this object if it is not zero.
+ * Do not use this.  This exists only to implement a special case for weak
+ * pointers.
+ * @return true if the reference count was incremented, false if it was zero.
+ */
+INLINE bool ReferenceCount::
+ref_if_nonzero() const {
+#ifdef _DEBUG
+  test_ref_count_integrity();
+#endif
+  AtomicAdjust::Integer ref_count;
+  do {
+    ref_count = AtomicAdjust::get(_ref_count);
+    if (ref_count <= 0) {
+      return false;
+    }
+  } while (ref_count != AtomicAdjust::compare_and_exchange(_ref_count, ref_count, ref_count + 1));
+  return true;
+}
+
 /**
  * This global helper function will unref the given ReferenceCount object, and
  * if the reference count reaches zero, automatically delete it.  It can't be

panda/src/express/referenceCount.h

@@ -63,6 +63,8 @@ public:
   INLINE WeakReferenceList *weak_ref();
   INLINE void weak_unref();
 
+  INLINE bool ref_if_nonzero() const;
+
 protected:
   bool do_test_ref_count_integrity() const;
   bool do_test_ref_count_nonzero() const;

panda/src/express/weakPointerTo.I

@@ -75,6 +75,9 @@ operator T * () const {
 /**
  * A thread-safe way to access the underlying pointer; will silently return
  * null if the underlying pointer was deleted or null.
+ * Note that this may return null even if was_deleted() still returns true,
+ * which can occur if the object has reached reference count 0 and is about to
+ * be destroyed.
  */
 template<class T>
 INLINE PointerTo<T> WeakPointerTo<T>::
@@ -84,7 +87,13 @@ lock() const {
     PointerTo<T> ptr;
     weak_ref->_lock.lock();
     if (!weak_ref->was_deleted()) {
-      ptr = (To *)WeakPointerToBase<T>::_void_ptr;
+      // We also need to check that the reference count is not zero (which can
+      // happen if the object is currently being destructed), since that could
+      // cause double deletion.
+      To *plain_ptr = (To *)WeakPointerToBase<T>::_void_ptr;
+      if (plain_ptr != nullptr && plain_ptr->ref_if_nonzero()) {
+        ptr.cheat() = plain_ptr;
+      }
     }
     weak_ref->_lock.unlock();
     return ptr;
@@ -239,7 +248,13 @@ lock() const {
     ConstPointerTo<T> ptr;
     weak_ref->_lock.lock();
     if (!weak_ref->was_deleted()) {
-      ptr = (const To *)WeakPointerToBase<T>::_void_ptr;
+      // We also need to check that the reference count is not zero (which can
+      // happen if the object is currently being destructed), since that could
+      // cause double deletion.
+      const To *plain_ptr = (const To *)WeakPointerToBase<T>::_void_ptr;
+      if (plain_ptr != nullptr && plain_ptr->ref_if_nonzero()) {
+        ptr.cheat() = plain_ptr;
+      }
     }
     weak_ref->_lock.unlock();
     return ptr;