[gjs: 1/2] boxed: Track ownership of boxed pointer better

[Cosimo Cecchi <cosimoc src gnome org>]

commit caa79be35a13a29a02e015119a75bfc482138433
Author: Philip Chimento <philip chimento gmail com>
Date:   Sun Apr 21 19:35:12 2019 -0700

    boxed: Track ownership of boxed pointer better
    
    This is the second bug of this type that I've fixed since refactoring
    this code, so clearly we need to do something differently. We rename
    m_not_owning_ptr to m_owning_ptr and make sure it starts out false (so
    that we never try to free a pointer if it wasn't assigned yet.) Then we
    introduce two methods for setting the m_ptr field: own_ptr() which sets
    m_owning_ptr, and share_ptr() which does not. We use these methods
    consistently everywhere that we previously set m_ptr directly.
    
    Closes: #240

 gi/boxed.cpp | 24 ++++++++++--------------
 gi/boxed.h   | 17 +++++++++++++++--
 2 files changed, 25 insertions(+), 16 deletions(-)
---
diff --git a/gi/boxed.cpp b/gi/boxed.cpp
index 71b3c224..2e02c867 100644
--- a/gi/boxed.cpp
+++ b/gi/boxed.cpp
@@ -46,7 +46,8 @@
 #include <girepository.h>
 
 BoxedInstance::BoxedInstance(JSContext* cx, JS::HandleObject obj)
-    : GIWrapperInstance(cx, obj) {
+    : GIWrapperInstance(cx, obj), m_owning_ptr(false) {
+    m_ptr = nullptr;
     GJS_INC_COUNTER(boxed_instance);
 }
 
@@ -115,7 +116,7 @@ BoxedBase* BoxedBase::get_copy_source(JSContext* context,
 void BoxedInstance::allocate_directly(void) {
     g_assert(get_prototype()->can_allocate_directly());
 
-    m_ptr = g_slice_alloc0(g_struct_info_get_size(info()));
+    own_ptr(g_slice_alloc0(g_struct_info_get_size(info())));
     m_allocated_directly = true;
 
     debug_lifecycle("Boxed pointer directly allocated");
@@ -256,7 +257,7 @@ static bool boxed_invoke_constructor(JSContext* context, JS::HandleObject obj,
  * pointer or another BoxedInstance.
  */
 void BoxedInstance::copy_boxed(void* boxed_ptr) {
-    m_ptr = g_boxed_copy(gtype(), boxed_ptr);
+    own_ptr(g_boxed_copy(gtype(), boxed_ptr));
     debug_lifecycle("Boxed pointer created with g_boxed_copy()");
 }
 
@@ -309,7 +310,6 @@ bool BoxedInstance::constructor_impl(JSContext* context, JS::HandleObject obj,
         // The return value of GLib.Variant.new_internal() gets its own
         // BoxedInstance, and the one we're setting up in this constructor is
         // discarded.
-        m_not_owning_ptr = true;
         debug_lifecycle(
             "Boxed construction delegated to GVariant constructor, "
             "boxed object discarded");
@@ -340,7 +340,7 @@ bool BoxedInstance::constructor_impl(JSContext* context, JS::HandleObject obj,
             return false;
         }
 
-        m_ptr = rval_arg.v_pointer;
+        own_ptr(rval_arg.v_pointer);
 
         debug_lifecycle("Boxed pointer created from zero-args constructor");
 
@@ -363,9 +363,7 @@ bool BoxedInstance::constructor_impl(JSContext* context, JS::HandleObject obj,
         }
 
         // The return value of the JS constructor gets its own BoxedInstance,
-        // and this one is discarded. Mark that the boxed pointer doesn't need
-        // to be freed, since it remains null.
-        m_not_owning_ptr = true;
+        // and this one is discarded.
         debug_lifecycle(
             "Boxed construction delegated to JS constructor, "
             "boxed object discarded");
@@ -395,7 +393,7 @@ bool BoxedInstance::constructor_impl(JSContext* context, JS::HandleObject obj,
 }
 
 BoxedInstance::~BoxedInstance() {
-    if (!m_not_owning_ptr) {
+    if (m_owning_ptr) {
         if (m_allocated_directly) {
             g_slice_free1(g_struct_info_get_size(info()), m_ptr);
         } else {
@@ -478,8 +476,7 @@ bool BoxedInstance::get_nested_interface_object(
     BoxedInstance* priv = BoxedInstance::new_for_js_object(context, obj);
 
     /* A structure nested inside a parent object; doesn't have an independent allocation */
-    priv->m_ptr = raw_ptr() + offset;
-    priv->m_not_owning_ptr = true;
+    priv->share_ptr(raw_ptr() + offset);
     priv->debug_lifecycle(
         "Boxed pointer created, pointing inside memory owned by parent");
 
@@ -1041,8 +1038,7 @@ JSObject* BoxedInstance::new_for_c_struct(JSContext* cx, GIStructInfo* info,
 bool BoxedInstance::init_from_c_struct(JSContext* cx, void* gboxed, NoCopy) {
     // We need to create a JS Boxed which references the original C struct, not
     // a copy of it. Used for G_SIGNAL_TYPE_STATIC_SCOPE.
-    m_ptr = gboxed;
-    m_not_owning_ptr = true;
+    share_ptr(gboxed);
     debug_lifecycle("Boxed pointer acquired, memory not owned");
     return true;
 }
@@ -1052,7 +1048,7 @@ bool BoxedInstance::init_from_c_struct(JSContext* cx, void* gboxed) {
         copy_boxed(gboxed);
         return true;
     } else if (gtype() == G_TYPE_VARIANT) {
-        m_ptr = g_variant_ref_sink(static_cast<GVariant*>(gboxed));
+        own_ptr(g_variant_ref_sink(static_cast<GVariant*>(gboxed)));
         debug_lifecycle("Boxed pointer created by sinking GVariant ref");
         return true;
     } else if (get_prototype()->can_allocate_directly()) {
diff --git a/gi/boxed.h b/gi/boxed.h
index f2f004a9..3b74f9ee 100644
--- a/gi/boxed.h
+++ b/gi/boxed.h
@@ -155,12 +155,25 @@ class BoxedInstance
     friend class BoxedBase;  // for field_getter, etc.
 
     bool m_allocated_directly : 1;
-    bool m_not_owning_ptr : 1;  // if set, the JS wrapper does not own the C
-                                // memory referred to by m_ptr.
+    bool m_owning_ptr : 1;  // if set, the JS wrapper owns the C memory referred
+                            // to by m_ptr.
 
     explicit BoxedInstance(JSContext* cx, JS::HandleObject obj);
     ~BoxedInstance(void);
 
+    // Don't set GIWrapperBase::m_ptr directly. Instead, use one of these
+    // setters to express your intention to own the pointer or not.
+    void own_ptr(void* boxed_ptr) {
+        g_assert(!m_ptr);
+        m_ptr = boxed_ptr;
+        m_owning_ptr = true;
+    }
+    void share_ptr(void* unowned_boxed_ptr) {
+        g_assert(!m_ptr);
+        m_ptr = unowned_boxed_ptr;
+        m_owning_ptr = false;
+    }
+
     // Methods for different ways to allocate the GBoxed pointer
 
     void allocate_directly(void);