El dom, 26-04-2009 a las 13:04 +0900, Kouhei Sutou escribió: > Hi, > > In <1240679760.4223.30.ca...@charmaleon> > "Re: [poppler] unit test for GLib bindings" on Sat, 25 Apr 2009 19:16:00 > +0200, > Carlos Garcia Campos <[email protected]> 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
[email protected]
[email protected]
http://carlosgc.linups.org
PGP key: http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462
signature.asc
Description: Esta parte del mensaje está firmada digitalmente
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
