express: refactor WeakPointerTo::lock() a little bit

panda/src/express/weakPointerTo.I

@@ -125,23 +125,9 @@ operator T * () const {
 template<class T>
 INLINE PointerTo<T> WeakPointerTo<T>::
 lock() const {
-  WeakReferenceList *weak_ref = this->_weak_ref;
+  PointerTo<T> ptr;
-  if (weak_ref != nullptr) {
+  this->lock_into(ptr);
-    PointerTo<T> ptr;
+  return ptr;
-    weak_ref->_lock.lock();
-    if (!weak_ref->was_deleted()) {
-      // 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;
-  }
-  return nullptr;
 }
 
 /**
@@ -415,23 +401,9 @@ operator const T * () const {
 template<class T>
 INLINE ConstPointerTo<T> WeakConstPointerTo<T>::
 lock() const {
-  WeakReferenceList *weak_ref = this->_weak_ref;
+  ConstPointerTo<T> ptr;
-  if (weak_ref != nullptr) {
+  this->lock_into(ptr);
-    ConstPointerTo<T> ptr;
+  return ptr;
-    weak_ref->_lock.lock();
-    if (!weak_ref->was_deleted()) {
-      // 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;
-  }
-  return nullptr;
 }
 
 /**

panda/src/express/weakPointerToBase.I

@@ -238,6 +238,34 @@ update_type(To *ptr) {
 #endif  // DO_MEMORY_USAGE
 }
 
+/**
+ * A thread-safe way to access the underlying pointer; will only write to the
+ * given pointer if the underlying pointer has not yet been deleted and is not
+ * null.  Note that it may leave the pointer unassigned 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 void WeakPointerToBase<T>::
+lock_into(PointerToBase<To> &locked) const {
+  WeakReferenceList *weak_ref = this->_weak_ref;
+  if (weak_ref != nullptr) {
+    weak_ref->_lock.lock();
+    if (!weak_ref->was_deleted()) {
+      // 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()) {
+        // It is valid and we successfully grabbed a reference.  Assign it,
+        // noting we have already incremented the reference count.
+        locked._void_ptr = plain_ptr;
+      }
+    }
+    weak_ref->_lock.unlock();
+  }
+}
+
 #ifndef CPPPARSER
 /**
  *

panda/src/express/weakPointerToBase.h

@@ -47,6 +47,8 @@ protected:
 
   INLINE void update_type(To *ptr);
 
+  INLINE void lock_into(PointerToBase<To> &locked) const;
+
   // No assignment or retrieval functions are declared in WeakPointerToBase,
   // because we will have to specialize on const vs.  non-const later.
 

panda/src/express/weakPointerToVoid.I

@@ -40,7 +40,11 @@ remove_callback(WeakPointerCallback *callback) const {
 
 /**
  * Returns true if the object we are pointing to has been deleted, false
- * otherwise.
+ * otherwise.  If this returns true, it means that the pointer can not yet be
+ * reused, but it does not guarantee that it can be safely accessed.  See the
+ * lock() method for a safe way to access the underlying pointer.
+ *
+ * This will always return true for a null pointer, unlike is_valid_pointer().
  */
 INLINE bool WeakPointerToVoid::
 was_deleted() const {
@@ -49,7 +53,7 @@ was_deleted() const {
 
 /**
  * Returns true if the pointer is not null and the object has not been
- * deleted.
+ * deleted.  See was_deleted() for caveats.
  */
 INLINE bool WeakPointerToVoid::
 is_valid_pointer() const {