patch 9.0.1035: object members are not being marked as used

Problem:    Object members are not being marked as used, garbage collection
            may free them.
Solution:   Mark object members as used.  Fix reference counting.

diff --git a/src/eval.c b/src/eval.c
index 917e0a852c1dec13ed73afe8e69b5e664788d8a3..703e1ea5292e4f4c6cba7d50b3d995c67284451c 100644 (file)
--- a/src/eval.c
+++ b/src/eval.c
@@ -5233,12 +5233,15 @@ free_unref_items(int copyID)
      * themselves yet, so that it is possible to decrement refcount counters
      */
 
-    // Go through the list of dicts and free items without the copyID.
+    // Go through the list of dicts and free items without this copyID.
     did_free |= dict_free_nonref(copyID);
 
-    // Go through the list of lists and free items without the copyID.
+    // Go through the list of lists and free items without this copyID.
     did_free |= list_free_nonref(copyID);
 
+    // Go through the list of objects and free items without this copyID.
+    did_free |= object_free_nonref(copyID);
+
 #ifdef FEAT_JOB_CHANNEL
     // Go through the list of jobs and free items without the copyID. This
     // must happen before doing channels, because jobs refer to channels, but
@@ -5405,7 +5408,8 @@ set_ref_in_callback(callback_T *cb, int copyID)
 }
 
 /*
- * Mark all lists and dicts referenced through typval "tv" with "copyID".
+ * Mark all lists, dicts and other container types referenced through typval
+ * "tv" with "copyID".
  * "list_stack" is used to add lists to be marked.  Can be NULL.
  * "ht_stack" is used to add hashtabs to be marked.  Can be NULL.
  *
@@ -5420,162 +5424,214 @@ set_ref_in_item(
 {
     int                abort = FALSE;
 
-    if (tv->v_type == VAR_DICT)
+    switch (tv->v_type)
     {
-       dict_T  *dd = tv->vval.v_dict;
-
-       if (dd != NULL && dd->dv_copyID != copyID)
+       case VAR_DICT:
        {
-           // Didn't see this dict yet.
-           dd->dv_copyID = copyID;
-           if (ht_stack == NULL)
-           {
-               abort = set_ref_in_ht(&dd->dv_hashtab, copyID, list_stack);
-           }
-           else
-           {
-               ht_stack_T *newitem = ALLOC_ONE(ht_stack_T);
+           dict_T      *dd = tv->vval.v_dict;
 
-               if (newitem == NULL)
-                   abort = TRUE;
+           if (dd != NULL && dd->dv_copyID != copyID)
+           {
+               // Didn't see this dict yet.
+               dd->dv_copyID = copyID;
+               if (ht_stack == NULL)
+               {
+                   abort = set_ref_in_ht(&dd->dv_hashtab, copyID, list_stack);
+               }
                else
                {
-                   newitem->ht = &dd->dv_hashtab;
-                   newitem->prev = *ht_stack;
-                   *ht_stack = newitem;
+                   ht_stack_T *newitem = ALLOC_ONE(ht_stack_T);
+
+                   if (newitem == NULL)
+                       abort = TRUE;
+                   else
+                   {
+                       newitem->ht = &dd->dv_hashtab;
+                       newitem->prev = *ht_stack;
+                       *ht_stack = newitem;
+                   }
                }
            }
+           break;
        }
-    }
-    else if (tv->v_type == VAR_LIST)
-    {
-       list_T  *ll = tv->vval.v_list;
 
-       if (ll != NULL && ll->lv_copyID != copyID)
+       case VAR_LIST:
        {
-           // Didn't see this list yet.
-           ll->lv_copyID = copyID;
-           if (list_stack == NULL)
-           {
-               abort = set_ref_in_list_items(ll, copyID, ht_stack);
-           }
-           else
-           {
-               list_stack_T *newitem = ALLOC_ONE(list_stack_T);
+           list_T      *ll = tv->vval.v_list;
 
-               if (newitem == NULL)
-                   abort = TRUE;
+           if (ll != NULL && ll->lv_copyID != copyID)
+           {
+               // Didn't see this list yet.
+               ll->lv_copyID = copyID;
+               if (list_stack == NULL)
+               {
+                   abort = set_ref_in_list_items(ll, copyID, ht_stack);
+               }
                else
                {
-                   newitem->list = ll;
-                   newitem->prev = *list_stack;
-                   *list_stack = newitem;
+                   list_stack_T *newitem = ALLOC_ONE(list_stack_T);
+
+                   if (newitem == NULL)
+                       abort = TRUE;
+                   else
+                   {
+                       newitem->list = ll;
+                       newitem->prev = *list_stack;
+                       *list_stack = newitem;
+                   }
                }
            }
+           break;
        }
-    }
-    else if (tv->v_type == VAR_FUNC)
-    {
-       abort = set_ref_in_func(tv->vval.v_string, NULL, copyID);
-    }
-    else if (tv->v_type == VAR_PARTIAL)
-    {
-       partial_T       *pt = tv->vval.v_partial;
-       int             i;
 
-       if (pt != NULL && pt->pt_copyID != copyID)
+       case VAR_FUNC:
        {
-           // Didn't see this partial yet.
-           pt->pt_copyID = copyID;
+           abort = set_ref_in_func(tv->vval.v_string, NULL, copyID);
+           break;
+       }
 
-           abort = set_ref_in_func(pt->pt_name, pt->pt_func, copyID);
+       case VAR_PARTIAL:
+       {
+           partial_T   *pt = tv->vval.v_partial;
+           int         i;
 
-           if (pt->pt_dict != NULL)
+           if (pt != NULL && pt->pt_copyID != copyID)
            {
-               typval_T dtv;
+               // Didn't see this partial yet.
+               pt->pt_copyID = copyID;
 
-               dtv.v_type = VAR_DICT;
-               dtv.vval.v_dict = pt->pt_dict;
-               set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
-           }
+               abort = set_ref_in_func(pt->pt_name, pt->pt_func, copyID);
+
+               if (pt->pt_dict != NULL)
+               {
+                   typval_T dtv;
+
+                   dtv.v_type = VAR_DICT;
+                   dtv.vval.v_dict = pt->pt_dict;
+                   set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
+               }
 
-           for (i = 0; i < pt->pt_argc; ++i)
-               abort = abort || set_ref_in_item(&pt->pt_argv[i], copyID,
-                                                       ht_stack, list_stack);
-           // pt_funcstack is handled in set_ref_in_funcstacks()
-           // pt_loopvars is handled in set_ref_in_loopvars()
+               for (i = 0; i < pt->pt_argc; ++i)
+                   abort = abort || set_ref_in_item(&pt->pt_argv[i], copyID,
+                                                        ht_stack, list_stack);
+               // pt_funcstack is handled in set_ref_in_funcstacks()
+               // pt_loopvars is handled in set_ref_in_loopvars()
+           }
+           break;
        }
-    }
-#ifdef FEAT_JOB_CHANNEL
-    else if (tv->v_type == VAR_JOB)
-    {
-       job_T       *job = tv->vval.v_job;
-       typval_T    dtv;
 
-       if (job != NULL && job->jv_copyID != copyID)
+       case VAR_JOB:
        {
-           job->jv_copyID = copyID;
-           if (job->jv_channel != NULL)
-           {
-               dtv.v_type = VAR_CHANNEL;
-               dtv.vval.v_channel = job->jv_channel;
-               set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
-           }
-           if (job->jv_exit_cb.cb_partial != NULL)
+#ifdef FEAT_JOB_CHANNEL
+           job_T           *job = tv->vval.v_job;
+           typval_T    dtv;
+
+           if (job != NULL && job->jv_copyID != copyID)
            {
-               dtv.v_type = VAR_PARTIAL;
-               dtv.vval.v_partial = job->jv_exit_cb.cb_partial;
-               set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
+               job->jv_copyID = copyID;
+               if (job->jv_channel != NULL)
+               {
+                   dtv.v_type = VAR_CHANNEL;
+                   dtv.vval.v_channel = job->jv_channel;
+                   set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
+               }
+               if (job->jv_exit_cb.cb_partial != NULL)
+               {
+                   dtv.v_type = VAR_PARTIAL;
+                   dtv.vval.v_partial = job->jv_exit_cb.cb_partial;
+                   set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
+               }
            }
+#endif
+           break;
        }
-    }
-    else if (tv->v_type == VAR_CHANNEL)
-    {
-       channel_T   *ch =tv->vval.v_channel;
-       ch_part_T   part;
-       typval_T    dtv;
-       jsonq_T     *jq;
-       cbq_T       *cq;
 
-       if (ch != NULL && ch->ch_copyID != copyID)
+       case VAR_CHANNEL:
        {
-           ch->ch_copyID = copyID;
-           for (part = PART_SOCK; part < PART_COUNT; ++part)
+#ifdef FEAT_JOB_CHANNEL
+           channel_T   *ch = tv->vval.v_channel;
+           ch_part_T   part;
+           typval_T    dtv;
+           jsonq_T     *jq;
+           cbq_T       *cq;
+
+           if (ch != NULL && ch->ch_copyID != copyID)
            {
-               for (jq = ch->ch_part[part].ch_json_head.jq_next; jq != NULL;
-                                                            jq = jq->jq_next)
-                   set_ref_in_item(jq->jq_value, copyID, ht_stack, list_stack);
-               for (cq = ch->ch_part[part].ch_cb_head.cq_next; cq != NULL;
-                                                            cq = cq->cq_next)
-                   if (cq->cq_callback.cb_partial != NULL)
+               ch->ch_copyID = copyID;
+               for (part = PART_SOCK; part < PART_COUNT; ++part)
+               {
+                   for (jq = ch->ch_part[part].ch_json_head.jq_next;
+                                                 jq != NULL; jq = jq->jq_next)
+                       set_ref_in_item(jq->jq_value, copyID,
+                                                        ht_stack, list_stack);
+                   for (cq = ch->ch_part[part].ch_cb_head.cq_next; cq != NULL;
+                                                             cq = cq->cq_next)
+                       if (cq->cq_callback.cb_partial != NULL)
+                       {
+                           dtv.v_type = VAR_PARTIAL;
+                           dtv.vval.v_partial = cq->cq_callback.cb_partial;
+                           set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
+                       }
+                   if (ch->ch_part[part].ch_callback.cb_partial != NULL)
                    {
                        dtv.v_type = VAR_PARTIAL;
-                       dtv.vval.v_partial = cq->cq_callback.cb_partial;
+                       dtv.vval.v_partial =
+                                     ch->ch_part[part].ch_callback.cb_partial;
                        set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
                    }
-               if (ch->ch_part[part].ch_callback.cb_partial != NULL)
+               }
+               if (ch->ch_callback.cb_partial != NULL)
                {
                    dtv.v_type = VAR_PARTIAL;
-                   dtv.vval.v_partial =
-                                     ch->ch_part[part].ch_callback.cb_partial;
+                   dtv.vval.v_partial = ch->ch_callback.cb_partial;
+                   set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
+               }
+               if (ch->ch_close_cb.cb_partial != NULL)
+               {
+                   dtv.v_type = VAR_PARTIAL;
+                   dtv.vval.v_partial = ch->ch_close_cb.cb_partial;
                    set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
                }
            }
-           if (ch->ch_callback.cb_partial != NULL)
-           {
-               dtv.v_type = VAR_PARTIAL;
-               dtv.vval.v_partial = ch->ch_callback.cb_partial;
-               set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
-           }
-           if (ch->ch_close_cb.cb_partial != NULL)
+#endif
+           break;
+       }
+
+       case VAR_CLASS:
+           // TODO: mark methods in class_obj_methods ?
+           break;
+
+       case VAR_OBJECT:
            {
-               dtv.v_type = VAR_PARTIAL;
-               dtv.vval.v_partial = ch->ch_close_cb.cb_partial;
-               set_ref_in_item(&dtv, copyID, ht_stack, list_stack);
+               object_T *obj = tv->vval.v_object;
+               if (obj != NULL && obj->obj_copyID != copyID)
+               {
+                   obj->obj_copyID = copyID;
+
+                   // The typval_T array is right after the object_T.
+                   typval_T *mtv = (typval_T *)(obj + 1);
+                   for (int i = 0; !abort
+                           && i < obj->obj_class->class_obj_member_count; ++i)
+                       abort = abort || set_ref_in_item(mtv + i, copyID,
+                                                        ht_stack, list_stack);
+               }
+               break;
            }
-       }
+
+       case VAR_UNKNOWN:
+       case VAR_ANY:
+       case VAR_VOID:
+       case VAR_BOOL:
+       case VAR_SPECIAL:
+       case VAR_NUMBER:
+       case VAR_FLOAT:
+       case VAR_STRING:
+       case VAR_BLOB:
+       case VAR_INSTR:
+           // Types that do not contain any other item
+           break;
     }
-#endif
+
     return abort;
 }
 

diff --git a/src/proto/vim9class.pro b/src/proto/vim9class.pro
index 4c6e12dab73991aae15b388fba8197d9ad943e6e..9a6b23fcb44418e303de2c019a7fa767a7c4282f 100644 (file)
--- a/src/proto/vim9class.pro
+++ b/src/proto/vim9class.pro
@@ -8,5 +8,8 @@ int class_object_index(char_u **arg, typval_T *rettv, evalarg_T *evalarg, int ve
 void copy_object(typval_T *from, typval_T *to);
 void object_unref(object_T *obj);
 void copy_class(typval_T *from, typval_T *to);
-void class_unref(typval_T *tv);
+void class_unref(class_T *cl);
+void object_created(object_T *obj);
+void object_cleared(object_T *obj);
+int object_free_nonref(int copyID);
 /* vim: set ft=c : */

diff --git a/src/structs.h b/src/structs.h
index 0fb57f46dfbb9b83959a9ba258a00d92ec116698..09b0743886735a54655665eb9386cb9ac586e9ff 100644 (file)
--- a/src/structs.h
+++ b/src/structs.h
@@ -1487,8 +1487,13 @@ struct class_S
 // Used for v_object of typval of VAR_OBJECT.
 // The member variables follow in an array of typval_T.
 struct object_S {
-    class_T    *obj_class;  // class this object is created for
+    class_T    *obj_class;         // class this object is created for;
+                                   // pointer adds to class_refcount
     int                obj_refcount;
+
+    object_T   *obj_next_used;     // for list headed by "first_object"
+    object_T   *obj_prev_used;     // for list headed by "first_object"
+    int                obj_copyID;         // used by garbage collection
 };
 
 #define TTFLAG_VARARGS 0x01        // func args ends with "..."

diff --git a/src/testdir/test_vim9_class.vim b/src/testdir/test_vim9_class.vim
index 6742ea757c4acdc35c86ec12d498366a3e62118e..1945db1c2b01f92e446bb12366c5396aa9aa4fa8 100644 (file)
--- a/src/testdir/test_vim9_class.vim
+++ b/src/testdir/test_vim9_class.vim
@@ -132,11 +132,10 @@ def Test_class_basic()
        this.col: number
       endclass
 
-      # # FIXME: this works but leaks memory
-      # # use the automatically generated new() method
-      # var pos = TextPosition.new(2, 12)
-      # assert_equal(2, pos.lnum)
-      # assert_equal(12, pos.col)
+      # use the automatically generated new() method
+      var pos = TextPosition.new(2, 12)
+      assert_equal(2, pos.lnum)
+      assert_equal(12, pos.col)
   END
   v9.CheckScriptSuccess(lines)
 enddef

diff --git a/src/typval.c b/src/typval.c
index 6faebe40154ee7e861294ce13ac812ac24adf1c8..98915ccca9022ef4d2c226c634c50cc3958be517 100644 (file)
--- a/src/typval.c
+++ b/src/typval.c
@@ -85,7 +85,7 @@ free_tv(typval_T *varp)
                break;
 #endif
            case VAR_CLASS:
-               class_unref(varp);
+               class_unref(varp->vval.v_class);
                break;
            case VAR_OBJECT:
                object_unref(varp->vval.v_object);
@@ -161,7 +161,7 @@ clear_tv(typval_T *varp)
                VIM_CLEAR(varp->vval.v_instr);
                break;
            case VAR_CLASS:
-               class_unref(varp);
+               class_unref(varp->vval.v_class);
                break;
            case VAR_OBJECT:
                object_unref(varp->vval.v_object);

diff --git a/src/version.c b/src/version.c
index f8977bc205f489a794634c2208985eaed9afb35c..7ae34f475b90178187918a4e929eed6b48dc80d6 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -695,6 +695,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    1035,
 /**/
     1034,
 /**/

diff --git a/src/vim9class.c b/src/vim9class.c
index 5804b129b797ee093b19aded760037d874f175fa..864a2e00b937f7f7612f67cfd9484c641c0a36c3 100644 (file)
--- a/src/vim9class.c
+++ b/src/vim9class.c
@@ -419,13 +419,18 @@ class_object_index(
                CLEAR_FIELD(funcexe);
                funcexe.fe_evaluate = TRUE;
 
+               // Clear the class or object after calling the function, in
+               // case the refcount is one.
+               typval_T tv_tofree = *rettv;
+               rettv->v_type = VAR_UNKNOWN;
+
                // Call the user function.  Result goes into rettv;
                // TODO: pass the object
-               rettv->v_type = VAR_UNKNOWN;
                int error = call_user_func_check(fp, argcount, argvars,
                                                        rettv, &funcexe, NULL);
 
-               // Clear the arguments.
+               // Clear the previous rettv and the arguments.
+               clear_tv(&tv_tofree);
                for (int idx = 0; idx < argcount; ++idx)
                    clear_tv(&argvars[idx]);
 
@@ -494,7 +499,11 @@ object_clear(object_T *obj)
     for (int i = 0; i < cl->class_obj_member_count; ++i)
        clear_tv(tv + i);
 
+    // Remove from the list headed by "first_object".
+    object_cleared(obj);
+
     vim_free(obj);
+    class_unref(cl);
 }
 
 /*
@@ -522,9 +531,8 @@ copy_class(typval_T *from, typval_T *to)
  * Unreference a class.  Free it when the reference count goes down to zero.
  */
     void
-class_unref(typval_T *tv)
+class_unref(class_T *cl)
 {
-    class_T *cl = tv->vval.v_class;
     if (cl != NULL && --cl->class_refcount <= 0)
     {
        vim_free(cl->class_name);
@@ -547,5 +555,60 @@ class_unref(typval_T *tv)
     }
 }
 
+static object_T *first_object = NULL;
+
+/*
+ * Call this function when an object has been created.  It will be added to the
+ * list headed by "first_object".
+ */
+    void
+object_created(object_T *obj)
+{
+    if (first_object != NULL)
+    {
+       obj->obj_next_used = first_object;
+       first_object->obj_prev_used = obj;
+    }
+    first_object = obj;
+}
+
+/*
+ * Call this function when an object has been cleared and is about to be freed.
+ * It is removed from the list headed by "first_object".
+ */
+    void
+object_cleared(object_T *obj)
+{
+    if (obj->obj_next_used != NULL)
+       obj->obj_next_used->obj_prev_used = obj->obj_prev_used;
+    if (obj->obj_prev_used != NULL)
+       obj->obj_prev_used->obj_next_used = obj->obj_next_used;
+    else if (first_object == obj)
+       first_object = obj->obj_next_used;
+}
+
+/*
+ * Go through the list of all objects and free items without "copyID".
+ */
+    int
+object_free_nonref(int copyID)
+{
+    int                did_free = FALSE;
+    object_T   *next_obj;
+
+    for (object_T *obj = first_object; obj != NULL; obj = next_obj)
+    {
+       next_obj = obj->obj_next_used;
+       if ((obj->obj_copyID & COPYID_MASK) != (copyID & COPYID_MASK))
+       {
+           // Free the object and items it contains.
+           object_clear(obj);
+           did_free = TRUE;
+       }
+    }
+
+    return did_free;
+}
+
 
 #endif // FEAT_EVAL

diff --git a/src/vim9execute.c b/src/vim9execute.c
index b164502576354223bfe8edb27b6d5d90946e90b4..3b142b8586b19836ac88b31c502b1ba1c6eaec9e 100644 (file)
--- a/src/vim9execute.c
+++ b/src/vim9execute.c
@@ -3018,7 +3018,9 @@ exec_instructions(ectx_T *ectx)
                                       iptr->isn_arg.construct.construct_size);
                tv->vval.v_object->obj_class =
                                       iptr->isn_arg.construct.construct_class;
+               ++tv->vval.v_object->obj_class->class_refcount;
                tv->vval.v_object->obj_refcount = 1;
+               object_created(tv->vval.v_object);
                break;
 
            // execute Ex command line