[poppler] unit test for GLib bindings
Carlos Garcia Campos
carlosgc at gnome.org
Tue Apr 28 01:52:48 PDT 2009
El dom, 26-04-2009 a las 13:04 +0900, Kouhei Sutou escribió:
> Hi,
>
> In <1240679760.4223.30.camel at charmaleon>
> "Re: [poppler] unit test for GLib bindings" on Sat, 25 Apr 2009 19:16:00 +0200,
> Carlos Garcia Campos <carlosgc at gnome.org> wrote:
>
> >> }
> >>
> >> static void
> >> @@ -518,7 +523,8 @@ attachment_inspect (GString *string, gconstpointer data, gpointer user_data)
> >> attachment->size);
> >> g_string_append_printf (string, "mtime=<%d>, ", attachment->mtime);
> >> g_string_append_printf (string, "ctime=<%d>, ", attachment->ctime);
> >> - g_string_append_printf (string, "checksum=<%s>", attachment->checksum->str);
> >> + g_string_append_printf (string, "checksum=<%s>",
> >> + attachment->checksum ? attachment->checksum->str : "NULL");
> >> g_string_append (string, ">");
> >> }
> >
> > still crashes. I've noticed that the checksum string is created even
> > when there isn't a checksum, I've just fixed it.
>
> I've followed the changes in githumb.com repository.
> # It seems that the test is good test because it found a
> # bug. :)
>
> > isn't it reproducible for you?
>
> I can't reproduce it on my environment.
> But I can reproduce it on LANG=fr_FR.UTF8 environment.
weird
> I wrote a small test script to reproduce this problem
> without Cutter:
>
> diff --git a/glib/Makefile.am b/glib/Makefile.am
> index 2c7ca1a..79cb423 100644
> --- a/glib/Makefile.am
> +++ b/glib/Makefile.am
> @@ -86,7 +86,7 @@ libpoppler_glib_la_LIBADD = \
> libpoppler_glib_la_LDFLAGS = -version-info 4:0:0 @create_shared_lib@ @auto_import_flags@
>
> if BUILD_WITH_GDK
> -noinst_PROGRAMS = test-poppler-glib
> +noinst_PROGRAMS = test-poppler-glib test-attachment
>
> test_poppler_glib_SOURCES = \
> test-poppler-glib.cc
> @@ -98,6 +98,17 @@ test_poppler_glib_LDADD = \
> $(GDK_LIBS) \
> $(FREETYPE_LIBS) \
> $(cairo_libs)
> +
> +test_attachment_SOURCES = \
> + test-attachment.c
> +
> +test_attachment_LDADD = \
> + $(top_builddir)/poppler/libpoppler.la \
> + libpoppler-glib.la \
> + $(POPPLER_GLIB_LIBS) \
> + $(GDK_LIBS) \
> + $(FREETYPE_LIBS) \
> + $(cairo_libs)
> endif
>
> BUILT_SOURCES = \
> diff --git a/glib/test-attachment.c b/glib/test-attachment.c
> new file mode 100644
> index 0000000..137925d
> --- /dev/null
> +++ b/glib/test-attachment.c
> @@ -0,0 +1,36 @@
> +/* -*- c-file-style: "gnu" -*- */
> +
> +#include <poppler.h>
> +
> +int main (int argc, char *argv[])
> +{
> + int i;
> +
> + g_type_init ();
> +
> + for (i = 0; i < 10; i++)
> + {
> + PopplerDocument *document;
> + GList *attachments, *node;
> + GError *error = NULL;
> +
> + document = poppler_document_new_from_file (argv[1], NULL, &error);
> + if (document == NULL)
> + {
> + g_print ("%s\n", error->message);
> + return(-1);
> + }
> +
> + attachments = poppler_document_get_attachments (document);
> + for (node = attachments; node; node = g_list_next (node))
> + {
> + PopplerAttachment *attachment = node->data;
> + poppler_attachment_save (attachment, "/tmp/attachment", NULL);
> + }
> + g_object_unref (document);
> + g_list_foreach (attachments, (GFunc) g_object_unref, NULL);
> + g_list_free (attachments);
> + }
> +
> + return 0;
> +}
>
> PopplerAttachment should be g_object_unref()-ed before
> PopplerDocument is g_object_unref()-ed. Is it right
> specification?
Ok, thanks to you test case I've managed to figure out what the problem
is. PopplerAttachment currently receives a document in its new method,
but it doesn't use it for anything. Theoretically, an attachment copies
everything, so it shouldn't depend on the document. However, copying a
Stream is actually increasing its reference counter (which is good and
desirable). When the document is destroyed, the PDFDoc is destroyed too,
which calls fclose() freeing all the allocated memory including the
attachments file streams. Once the PDFDoc is destroyed all reference
counters for streams are not valid any more. Then, attachments finalize
method frees and already freed file stream.
> The behavior isn't good for bindings for script language
> that its GC isn't based on reference count. (e.g. Ruby)
> In the script language bindings, (e.g. Ruby/Poppler),
> PopplerAttachment may be destroyed before PopplerDocument if
> PopplerAttachment doesn't refer PopplerDocument.
>
> What about PopplerAttachment refers its document? It will
> fix the GC related problem. (And Cutter based test will
> also work. :-)
yes, that's the only solution I can think of
> diff --git a/glib/poppler-attachment.cc b/glib/poppler-attachment.cc
> index f6dbfd2..78bc72f 100644
> --- a/glib/poppler-attachment.cc
> +++ b/glib/poppler-attachment.cc
> @@ -29,11 +29,13 @@
> typedef struct _PopplerAttachmentPrivate PopplerAttachmentPrivate;
> struct _PopplerAttachmentPrivate
> {
> - Object obj_stream;
> + Object *obj_stream;
> + PopplerDocument *document;
> };
>
> #define POPPLER_ATTACHMENT_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE ((obj), POPPLER_TYPE_ATTACHMENT, PopplerAttachmentPrivate))
>
> +static void poppler_attachment_dispose (GObject *obj);
> static void poppler_attachment_finalize (GObject *obj);
>
> G_DEFINE_TYPE (PopplerAttachment, poppler_attachment, G_TYPE_OBJECT)
> @@ -46,11 +48,35 @@ poppler_attachment_init (PopplerAttachment *attachment)
> static void
> poppler_attachment_class_init (PopplerAttachmentClass *klass)
> {
> + G_OBJECT_CLASS (klass)->dispose = poppler_attachment_dispose;
> G_OBJECT_CLASS (klass)->finalize = poppler_attachment_finalize;
> g_type_class_add_private (klass, sizeof (PopplerAttachmentPrivate));
> }
>
> static void
> +poppler_attachment_dispose (GObject *obj)
> +{
> + PopplerAttachmentPrivate *priv;
> +
> + priv = POPPLER_ATTACHMENT_GET_PRIVATE (obj);
> +
> + if (priv->obj_stream)
> + {
> + priv->obj_stream->free();
> + delete priv->obj_stream;
> + priv->obj_stream = NULL;
> + }
> +
> + if (priv->document)
> + {
> + g_object_unref (priv->document);
> + priv->document = NULL;
> + }
> +
> + G_OBJECT_CLASS (poppler_attachment_parent_class)->dispose (obj);
> +}
> +
> +static void
> poppler_attachment_finalize (GObject *obj)
> {
> PopplerAttachment *attachment;
> @@ -69,8 +95,6 @@ poppler_attachment_finalize (GObject *obj)
> g_string_free (attachment->checksum, TRUE);
> attachment->checksum = NULL;
>
> - POPPLER_ATTACHMENT_GET_PRIVATE (attachment)->obj_stream.free();
> -
> G_OBJECT_CLASS (poppler_attachment_parent_class)->finalize (obj);
> }
>
> @@ -81,12 +105,16 @@ _poppler_attachment_new (PopplerDocument *document,
> EmbFile *emb_file)
> {
> PopplerAttachment *attachment;
> + PopplerAttachmentPrivate *priv;
>
> g_assert (document != NULL);
> g_assert (emb_file != NULL);
>
> attachment = (PopplerAttachment *) g_object_new (POPPLER_TYPE_ATTACHMENT, NULL);
> -
> + priv = POPPLER_ATTACHMENT_GET_PRIVATE (attachment);
> +
> + priv->document = (PopplerDocument *) g_object_ref (document);
> +
> if (emb_file->name ())
> attachment->name = _poppler_goo_string_to_utf8 (emb_file->name ());
> if (emb_file->description ())
> @@ -101,7 +129,8 @@ _poppler_attachment_new (PopplerDocument *document,
> attachment->checksum = g_string_new_len (emb_file->checksum ()->getCString (),
> emb_file->checksum ()->getLength ());
>
> - emb_file->streamObject().copy(&POPPLER_ATTACHMENT_GET_PRIVATE (attachment)->obj_stream);
> + priv->obj_stream = new Object();
> + emb_file->streamObject().copy(priv->obj_stream);
>
> return attachment;
> }
> @@ -214,7 +243,7 @@ poppler_attachment_save_to_callback (PopplerAttachment *attachment,
>
> g_return_val_if_fail (POPPLER_IS_ATTACHMENT (attachment), FALSE);
>
> - stream = POPPLER_ATTACHMENT_GET_PRIVATE (attachment)->obj_stream.getStream();
> + stream = POPPLER_ATTACHMENT_GET_PRIVATE (attachment)->obj_stream->getStream();
> stream->reset();
>
> do
patch looks good to me, could you send it again attached to the mail and
generated with git format-patch, please?
Thank you very much :-)
>
> Thanks,
> --
> kou
--
Carlos Garcia Campos
elkalmail at yahoo.es
carlosgc at gnome.org
http://carlosgc.linups.org
PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Esta parte del mensaje =?ISO-8859-1?Q?est=E1?= firmada
digitalmente
Url : http://lists.freedesktop.org/archives/poppler/attachments/20090428/54a134f3/attachment.pgp
More information about the poppler
mailing list