Freeze break request: GJS memory leak fix

[philip chimento gmail com]

Hi release team,

I'd like to request a break of the hard code freeze for https://bugzilla.gnome.org/show_bug.cgi?id=780171. This is a fix for a memory leak in GJS which has been present in the code since February 15.

I'm the maintainer of GJS, and I am comfortable committing it. It does not break API, change any features, or change any strings.

The memory leak was spotted by Luke Jones yesterday and brought to my attention with a Valgrind log. Both Luke and Hussam Al-Tayeb have confirmed that the fix prevents GNOME Shell from leaking hundreds of megabytes of memory per hour. Therefore, I believe this fix is necessary to improve the stability of the GNOME 3.24 release.

Cosimo Cecchi has reviewed the patch and Florian Müllner already reviewed a previous version. Due to the short time, it has not undergone extensive testing, but I have been running GJS with the patch since yesterday. In addition, the fix is in a function that is called thousands of times during a run of GJS's test suite. I have determined this using code coverage tools. So if it were going to cause crashes, it would be apparent, and by this method I did already discover and fix a mistake in an earlier version of the patch. (The tests don't yet cover all the function's exits, but the uncovered paths are exceptional and unlikely to come up in normal usage.)

Best,

Philip C

Patch follows:

From 9050118c9e77849a29d0193aea8f710038bfd5fa Mon Sep 17 00:00:00 2001

From: Philip Chimento <philip endlessm com>

Date: Thu, 16 Mar 2017 15:43:09 -0700

Subject: [PATCH] object: Fix memory leak in resolve

The "name" string, allocated in gjs_get_string_id(), wasn't getting freed

at every exit point of the function. (It will be nice to clean this up in

1.49.x with our autoptr classes...)

After a certain point in the function it's not needed, so we just free

the string there.

https://bugzilla.gnome.org/show_bug.cgi?id=780171

---

 gi/object.cpp | 15 +++++++++++----

 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/gi/object.cpp b/gi/object.cpp

index c68ad4c6..63731df5 100644

--- a/gi/object.cpp

+++ b/gi/object.cpp

@@ -607,7 +607,7 @@ is_vfunc_unchanged(GIVFuncInfo *info,

 

 static GIVFuncInfo *

 find_vfunc_on_parents(GIObjectInfo *info,

-                      gchar        *name,

+                      const char   *name,

                       bool         *out_defined_by_parent)

 {

     GIVFuncInfo *vfunc = NULL;

@@ -774,12 +774,14 @@ object_instance_resolve(JSContext       *context,

          * rest.

          */

 

-        gchar *name_without_vfunc_ = &name[6];

+        const char *name_without_vfunc_ = &name[6];  /* lifetime tied to name */

         GIVFuncInfo *vfunc;

         bool defined_by_parent;

 

         vfunc = find_vfunc_on_parents(priv->info, name_without_vfunc_, &defined_by_parent);

         if (vfunc != NULL) {

+            g_free(name);

+

             /* In the event that the vfunc is unchanged, let regular

              * prototypal inheritance take over. */

             if (defined_by_parent && is_vfunc_unchanged(vfunc, priv->gtype)) {

@@ -818,8 +820,13 @@ object_instance_resolve(JSContext       *context,

      * this could be done better.  See

      * https://bugzilla.gnome.org/show_bug.cgi?id=632922

      */

-    if (method_info == NULL)

-        return object_instance_resolve_no_info(context, obj, resolved, priv, name);

+    if (method_info == NULL) {

+        bool retval = object_instance_resolve_no_info(context, obj, resolved, priv, name);

+        g_free(name);

+        return retval;

+    }

+

+    g_free(name);

 

 #if GJS_VERBOSE_ENABLE_GI_USAGE

     _gjs_log_info_usage((GIBaseInfo*) method_info);

-- 

2.11.0