dnrops
/
gimp
mirror of https://github.com/GNOME/gimp.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Issue #3912: Object proxy management not happy with bindings. 

We cannot be 100% sure generically (i.e. for all possible bindings available
with GObject Introspection) if bindings add their own reference to objects or
not. Clearly we have cases when they always do (Lua, Javascript), cases when
they do only in certain conditions (global Python variables) and cases when they
don't (Vala). What we know for sure is that in these script languages,
developers don't manually manage memory anyway. So the additional reference is
not their fact.

So let's just maintain a list of automatic memory managed binding languages,
among the few we officially support (i.e. the ones for which we have working
test plug-ins) and verify by executable extension if the plug-in is written in
one of these.
Both keeping a manually-updated list and verifying by extension are not so
pretty solution, but for now it will do.
This commit is contained in:
Jehan 2023-10-15 16:34:11 +02:00
parent bd7423915c
commit 317be5f4ce
8 changed files with 152 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
libgimp/gimp.c
View File

@ -497,6 +497,7 @@ gimp_main (GType plug_in_type,
_gimp_debug_configure (stack_trace_mode);
PLUG_IN = g_object_new (plug_in_type,
"program-name", progname,
"read-channel", read_channel,
"write-channel", write_channel,
NULL);

9
libgimp/gimpimageprocedure.c
View File

@ -22,7 +22,10 @@
#include "gimp.h"
#include "libgimpbase/gimpwire.h" /* FIXME kill this include */
#include "gimpimageprocedure.h"
#include "gimpplugin-private.h"
/**
@ -144,6 +147,7 @@ static GimpValueArray *
gimp_image_procedure_run (GimpProcedure *procedure,
const GimpValueArray *args)
{
GimpPlugIn *plug_in;
GimpImageProcedure *image_proc = GIMP_IMAGE_PROCEDURE (procedure);
GimpPDBStatusType status = GIMP_PDB_EXECUTION_ERROR;
GimpProcedureConfig *config;
@ -188,11 +192,12 @@ gimp_image_procedure_run (GimpProcedure *procedure,
/* This is debug printing to help plug-in developers figure out best
* practices.
*/
if (G_OBJECT (config)->ref_count > 1)
plug_in = gimp_procedure_get_plug_in (procedure);
if (G_OBJECT (config)->ref_count > 1 &&
_gimp_plug_in_manage_memory_manually (plug_in))
g_printerr ("%s: ERROR: the GimpProcedureConfig object was refed "
"by plug-in, it MUST NOT do that!\n", G_STRFUNC);
g_object_unref (config);
gimp_value_array_unref (remaining);

8
libgimp/gimploadprocedure.c
View File

@ -22,8 +22,11 @@
#include "gimp.h"
#include "libgimpbase/gimpwire.h" /* FIXME kill this include */
#include "gimploadprocedure.h"
#include "gimppdb_pdb.h"
#include "gimpplugin-private.h"
#include "libgimp-intl.h"
@ -176,6 +179,7 @@ static GimpValueArray *
gimp_load_procedure_run (GimpProcedure *procedure,
const GimpValueArray *args)
{
GimpPlugIn *plug_in;
GimpLoadProcedure *load_proc = GIMP_LOAD_PROCEDURE (procedure);
GimpValueArray *remaining;
GimpValueArray *return_values;
@ -274,7 +278,9 @@ gimp_load_procedure_run (GimpProcedure *procedure,
/* This is debug printing to help plug-in developers figure out best
* practices.
*/
if (G_OBJECT (config)->ref_count > 1)
plug_in = gimp_procedure_get_plug_in (procedure);
if (G_OBJECT (config)->ref_count > 1 &&
_gimp_plug_in_manage_memory_manually (plug_in))
g_printerr ("%s: ERROR: the GimpSaveProcedureConfig object was refed "
"by plug-in, it MUST NOT do that!\n", G_STRFUNC);

50
libgimp/gimpplugin-private.h
View File

@ -25,36 +25,38 @@
G_BEGIN_DECLS
void _gimp_plug_in_query (GimpPlugIn *plug_in);
void _gimp_plug_in_init (GimpPlugIn *plug_in);
void _gimp_plug_in_run (GimpPlugIn *plug_in);
void _gimp_plug_in_quit (GimpPlugIn *plug_in);
void _gimp_plug_in_query (GimpPlugIn *plug_in);
void _gimp_plug_in_init (GimpPlugIn *plug_in);
void _gimp_plug_in_run (GimpPlugIn *plug_in);
void _gimp_plug_in_quit (GimpPlugIn *plug_in);
GIOChannel * _gimp_plug_in_get_read_channel (GimpPlugIn *plug_in);
GIOChannel * _gimp_plug_in_get_write_channel (GimpPlugIn *plug_in);
GIOChannel * _gimp_plug_in_get_read_channel (GimpPlugIn *plug_in);
GIOChannel * _gimp_plug_in_get_write_channel (GimpPlugIn *plug_in);
void _gimp_plug_in_read_expect_msg (GimpPlugIn *plug_in,
GimpWireMessage *msg,
gint type);
void _gimp_plug_in_read_expect_msg (GimpPlugIn *plug_in,
GimpWireMessage *msg,
gint type);
gboolean _gimp_plug_in_set_i18n (GimpPlugIn *plug_in,
const gchar *procedure_name,
gchar **gettext_domain,
gchar **catalog_dir);
gboolean _gimp_plug_in_set_i18n (GimpPlugIn *plug_in,
const gchar *procedure_name,
gchar **gettext_domain,
gchar **catalog_dir);
GimpProcedure * _gimp_plug_in_create_procedure (GimpPlugIn *plug_in,
const gchar *procedure_name);
GimpProcedure * _gimp_plug_in_create_procedure (GimpPlugIn *plug_in,
const gchar *procedure_name);
GimpProcedure * _gimp_plug_in_get_procedure (GimpPlugIn *plug_in);
GimpProcedure * _gimp_plug_in_get_procedure (GimpPlugIn *plug_in);
GimpDisplay * _gimp_plug_in_get_display (GimpPlugIn *plug_in,
gint32 display_id);
GimpImage * _gimp_plug_in_get_image (GimpPlugIn *plug_in,
gint32 image_id);
GimpItem * _gimp_plug_in_get_item (GimpPlugIn *plug_in,
gint32 item_id);
GimpResource * _gimp_plug_in_get_resource (GimpPlugIn *plug_in,
gint32 resource_id);
GimpDisplay * _gimp_plug_in_get_display (GimpPlugIn *plug_in,
gint32 display_id);
GimpImage * _gimp_plug_in_get_image (GimpPlugIn *plug_in,
gint32 image_id);
GimpItem * _gimp_plug_in_get_item (GimpPlugIn *plug_in,
gint32 item_id);
GimpResource * _gimp_plug_in_get_resource (GimpPlugIn *plug_in,
gint32 resource_id);
gboolean _gimp_plug_in_manage_memory_manually (GimpPlugIn *plug_in);
G_END_DECLS

112
libgimp/gimpplugin.c
View File

@ -114,6 +114,7 @@ G_DEFINE_QUARK (gimp-plug-in-error-quark, gimp_plug_in_error)
enum
{
PROP_0,
PROP_PROGRAM_NAME,
PROP_READ_CHANNEL,
PROP_WRITE_CHANNEL,
N_PROPS
@ -130,6 +131,8 @@ struct _GimpPlugInMenuBranch
struct _GimpPlugInPrivate
{
gchar *program_name;
GIOChannel *read_channel;
GIOChannel *write_channel;
@ -207,7 +210,8 @@ static void gimp_plug_in_push_procedure (GimpPlugIn *plug_in,
static void gimp_plug_in_pop_procedure (GimpPlugIn *plug_in,
GimpProcedure *procedure);
static void gimp_plug_in_destroy_hashes (GimpPlugIn *plug_in);
static void gimp_plug_in_destroy_proxies (GHashTable *hash_table,
static void gimp_plug_in_destroy_proxies (GimpPlugIn *plug_in,
GHashTable *hash_table,
const gchar *type,
gboolean destroy_all);
@ -234,6 +238,19 @@ gimp_plug_in_class_init (GimpPlugInClass *klass)
klass->set_i18n = gimp_plug_in_real_set_i18n;
/**
* GimpPlugIn:program-name:
*
* The program name as usually found on argv[0]
*/
props[PROP_PROGRAM_NAME] =
g_param_spec_string ("program-name",
"The plug-in executable",
"The executable name as usually found on argv[0]",
NULL,
GIMP_PARAM_WRITABLE |
G_PARAM_CONSTRUCT_ONLY);
/**
* GimpPlugIn:read-channel:
*
@ -311,6 +328,7 @@ gimp_plug_in_finalize (GObject *object)
GimpPlugIn *plug_in = GIMP_PLUG_IN (object);
GList *list;
g_clear_pointer (&plug_in->priv->program_name, g_free);
g_clear_pointer (&plug_in->priv->translation_domain_name, g_free);
g_clear_object (&plug_in->priv->translation_domain_path);
@ -328,10 +346,10 @@ gimp_plug_in_finalize (GObject *object)
g_clear_pointer (&plug_in->priv->menu_branches, g_list_free);
gimp_plug_in_destroy_proxies (plug_in->priv->displays, "display", TRUE);
gimp_plug_in_destroy_proxies (plug_in->priv->images, "image", TRUE);
gimp_plug_in_destroy_proxies (plug_in->priv->items, "item", TRUE);
gimp_plug_in_destroy_proxies (plug_in->priv->resources, "resource", TRUE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->displays, "display", TRUE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->images, "image", TRUE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->items, "item", TRUE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->resources, "resource", TRUE);
gimp_plug_in_destroy_hashes (plug_in);
@ -348,6 +366,10 @@ gimp_plug_in_set_property (GObject *object,
switch (property_id)
{
case PROP_PROGRAM_NAME:
plug_in->priv->program_name = g_value_dup_string (value);
break;
case PROP_READ_CHANNEL:
plug_in->priv->read_channel = g_value_get_boxed (value);
break;
@ -1442,17 +1464,17 @@ gimp_plug_in_pop_procedure (GimpPlugIn *plug_in,
_gimp_procedure_destroy_proxies (procedure);
gimp_plug_in_destroy_proxies (plug_in->priv->displays, "display", FALSE);
gimp_plug_in_destroy_proxies (plug_in->priv->images, "image", FALSE);
gimp_plug_in_destroy_proxies (plug_in->priv->items, "item", FALSE);
gimp_plug_in_destroy_proxies (plug_in->priv->resources, "resource", FALSE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->displays, "display", FALSE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->images, "image", FALSE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->items, "item", FALSE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->resources, "resource", FALSE);
if (! plug_in->priv->procedure_stack)
{
gimp_plug_in_destroy_proxies (plug_in->priv->displays, "display", TRUE);
gimp_plug_in_destroy_proxies (plug_in->priv->images, "image", TRUE);
gimp_plug_in_destroy_proxies (plug_in->priv->items, "item", TRUE);
gimp_plug_in_destroy_proxies (plug_in->priv->resources, "resource", TRUE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->displays, "display", TRUE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->images, "image", TRUE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->items, "item", TRUE);
gimp_plug_in_destroy_proxies (plug_in, plug_in->priv->resources, "resource", TRUE);
gimp_plug_in_destroy_hashes (plug_in);
}
@ -1648,6 +1670,42 @@ _gimp_plug_in_get_resource (GimpPlugIn *plug_in,
return resource;
}
gboolean
_gimp_plug_in_manage_memory_manually (GimpPlugIn *plug_in)
{
gboolean manual_management = TRUE;
if (plug_in->priv->program_name)
{
GFile *file = g_file_new_for_path (plug_in->priv->program_name);
/* Limitations:
* 1. Checking a file extension (and trusting argv[0] in general) is not
* the best idea. I thought about the possibility of passing through
* the name of the used interpreter from core to plug-in, but even this
* should not be trusted because it is configurable.
* 2. An alternative could be to pass the information if a plug-in is
* interpreted (i.e. it has an interp file). Though I'm unsure if there
* might be some interpreted language out there with manual memory
* management.
* 3. This list only contains the bindings we actually tested with (and
* have demo plug-ins for). Much more bindings are possible.
*
* So in the end, this test is just worth so far, but it's
* acceptable for the officially supported bindings at least.
*/
if (gimp_file_has_extension (file, ".lua") ||
gimp_file_has_extension (file, ".py") ||
gimp_file_has_extension (file, ".js") ||
gimp_file_has_extension (file, ".gjs"))
manual_management = FALSE;
g_object_unref (file);
}
return manual_management;
}
static void
gimp_plug_in_destroy_hashes (GimpPlugIn *plug_in)
{
@ -1658,7 +1716,8 @@ gimp_plug_in_destroy_hashes (GimpPlugIn *plug_in)
}
static void
gimp_plug_in_destroy_proxies (GHashTable *hash_table,
gimp_plug_in_destroy_proxies (GimpPlugIn *plug_in,
GHashTable *hash_table,
const gchar *type,
gboolean destroy_all)
{
@ -1701,15 +1760,26 @@ gimp_plug_in_destroy_proxies (GHashTable *hash_table,
g_object_get (object, "id", &id, NULL);
g_printerr ("%s: ERROR: %s proxy with ID %d was refed "
"by plug-in, it MUST NOT do that!\n",
G_STRFUNC, G_OBJECT_TYPE_NAME (object), id);
/* Some bindings are always keeping a reference to objects, in
* particular this is the behavior of lua and gjs bindings where
* object->ref_count == 2 at this point.
* As for the Python binding, it is only true for global
* variables (scoped variables are properly freed before the
* plug-in ends), which anyway is not the best practice.
* In any case, it's not something which the plug-in developer can do
* anything about so this error is only confusing.
* See #3912.
*/
if (_gimp_plug_in_manage_memory_manually (plug_in))
g_printerr ("%s: ERROR: %s proxy with ID %d was refed "
"by plug-in, it MUST NOT do that!\n",
G_STRFUNC, G_OBJECT_TYPE_NAME (object), id);
#if 0
/* the code used to do this, but it appears that some bindings
* keep references around until later, let's keep this for
* reference and as a reminder to figure if we can do anything
* generic about it that works for all bindings.
/* The code used to do this, which is only meaningful when the bug is
* really in the plug-in code. If it's one of the cases where the
* reference is actually held by a bindings, it's unwise and will
* likely end up in double-free crashes.
*/
while (object->ref_count > 1)
g_object_unref (object);

6
libgimp/gimpprocedure.c
View File

@ -32,6 +32,7 @@
#include "gimppdb-private.h"
#include "gimpplugin-private.h"
#include "gimppdb_pdb.h"
#include "gimpplugin-private.h"
#include "gimpplugin_pdb.h"
#include "gimpprocedure-private.h"
#include "gimpprocedureconfig-private.h"
@ -480,6 +481,7 @@ static GimpValueArray *
gimp_procedure_real_run (GimpProcedure *procedure,
const GimpValueArray *args)
{
GimpPlugIn *plug_in;
GimpProcedureConfig *config;
GimpImage *image = NULL;
GimpRunMode run_mode = GIMP_RUN_NONINTERACTIVE;
@ -511,7 +513,9 @@ gimp_procedure_real_run (GimpProcedure *procedure,
/* This is debug printing to help plug-in developers figure out best
* practices.
*/
if (G_OBJECT (config)->ref_count > 1)
plug_in = gimp_procedure_get_plug_in (procedure);
if (G_OBJECT (config)->ref_count > 1 &&
_gimp_plug_in_manage_memory_manually (plug_in))
g_printerr ("%s: ERROR: the GimpProcedureConfig object was refed "
"by plug-in, it MUST NOT do that!\n", G_STRFUNC);

10
libgimp/gimpsaveprocedure.c
View File

@ -22,8 +22,11 @@
#include "gimp.h"
#include "gimpsaveprocedure.h"
#include "libgimpbase/gimpwire.h" /* FIXME kill this include */
#include "gimppdb_pdb.h"
#include "gimpplugin-private.h"
#include "gimpsaveprocedure.h"
#include "libgimp-intl.h"
@ -337,6 +340,7 @@ static GimpValueArray *
gimp_save_procedure_run (GimpProcedure *procedure,
const GimpValueArray *args)
{
GimpPlugIn *plug_in;
GimpSaveProcedure *save_proc = GIMP_SAVE_PROCEDURE (procedure);
GimpValueArray *remaining;
GimpValueArray *return_values;
@ -413,7 +417,9 @@ gimp_save_procedure_run (GimpProcedure *procedure,
/* This is debug printing to help plug-in developers figure out best
* practices.
*/
if (G_OBJECT (config)->ref_count > 1)
plug_in = gimp_procedure_get_plug_in (procedure);
if (G_OBJECT (config)->ref_count > 1 &&
_gimp_plug_in_manage_memory_manually (plug_in))
g_printerr ("%s: ERROR: the GimpSaveProcedureConfig object was refed "
"by plug-in, it MUST NOT do that!\n", G_STRFUNC);

8
libgimp/gimpthumbnailprocedure.c
View File

@ -22,6 +22,9 @@
#include "gimp.h"
#include "libgimpbase/gimpwire.h" /* FIXME kill this include */
#include "gimpplugin-private.h"
#include "gimpthumbnailprocedure.h"
@ -138,6 +141,7 @@ static GimpValueArray *
gimp_thumbnail_procedure_run (GimpProcedure *procedure,
const GimpValueArray *args)
{
GimpPlugIn *plug_in;
GimpThumbnailProcedure *thumbnail_proc = GIMP_THUMBNAIL_PROCEDURE (procedure);
GimpValueArray *remaining;
GimpValueArray *return_values;
@ -178,7 +182,9 @@ gimp_thumbnail_procedure_run (GimpProcedure *procedure,
/* This is debug printing to help plug-in developers figure out best
* practices.
*/
if (G_OBJECT (config)->ref_count > 1)
plug_in = gimp_procedure_get_plug_in (procedure);
if (G_OBJECT (config)->ref_count > 1 &&
_gimp_plug_in_manage_memory_manually (plug_in))
g_printerr ("%s: ERROR: the GimpThumbnailProcedure config object was refed "
"by plug-in, it MUST NOT do that!\n", G_STRFUNC);