Created attachment 91095 [details]
sample crash-causing tif

Created attachment 91096 [details]
backtrace

Created attachment 91120 [details] [review]
a patch

Hmm, loads just fine here. Anyway, does the attached patch help ?

That stops the crashes, but this warning occurs:

(gthumb:24104): GdkPixbuf-WARNING **: Bug! loader 'tiff' didn't set an error on failure

Background info: gThumb 2.10.4+ checks the height and width of the image using gdk_pixbuf_get_file_info before deciding whether to generate a thumbnail with  gdk_pixbuf_new_from_file_at_scale or with gdk_pixbuf_new_from_file (scaled vs unscaled). (2.10.3 and earlier don't do this check.)

The get_info function seems to choke, but the actual file loader seems OK.

- Mike

I'll probably need a small testcase to make further progress on this.

2007-07-03  Matthias Clasen  <mclasen@redhat.com>

        * gdk-pixbuf-loader.c (gdk_pixbuf_loader_close): Be
        more careful when calling g_propagate_error().  (#453365,
        Michael Chudobiak)

Partial clue:

io-tiff.c:tiff_image_parse:196 does not set an error message, but all the other checks before and after it do...

- Mike

Ok, thanks, I have corrected that.

2007-07-03  Matthias Clasen  <mclasen@redhat.com>

        * io-tiff.c (tiff_image_parse): Always set an error
        when returning NULL.  (453365, Michael Chudobiak)

Created attachment 91133 [details] [review]
fix: width/height set after signal?

Matthias,

Thanks, your patch stops console warnings, but the underlying error is still there.

I think the problem is that gdk_pixbuf_loader_size_func sets the height/width values after emitting a signal, so weird race conditions happen (for instance, my problems went away after inserting debugging printf's...)

A simple patch is attached. I may have misunderstood things, so please make sure it really does make sense...

- Mike

Patch doesn't make sense to me. There is no race condition that I can see,
emitting signals is entirely synchronous. What is happening in size_func is
the following:

1. Initialize priv->width and priv->height to the pixbuf dimensions.
We allow to all gdk_pixbuf_loader_set_size() before this point, so 
we only do the initialization if priv->width and priv->height haven't
bee set yet.

        if (priv->width == -1 && priv->height == -1)
                {
                        priv->width = *width;
                        priv->height = *height;
                }

2. Emit the size-prepared signal. Handlers are expected to call gdk_pixbuf_loader_set_size().

        g_signal_emit (loader, pixbuf_loader_signals[SIZE_PREPARED], 0, *width, *height);

3. At this point we no longer accept set_size() calls
 
        priv->size_fixed = TRUE;

4. And we return the values set by the last set_size() call

        *width = priv->width;
        *height = priv->height;


Can you show your size-prepared handler ?

OK, there is no race condition - that was just a theory. But I think I've found the true root cause of the issues. gdk-pixbuf-io.c:info_cb has as its last line:

gdk_pixbuf_loader_set_size (loader, 0, 0);

which is setting the width and height to zero, which persists. Why? Shouldn't it be something like:

gdk_pixbuf_loader_set_size (loader, width, height);

- Mike

Oh, hmm, so my last io-tiff.c change was acutally wrong. Here is what the docs say:

<para>
If the function sets @width or @height to zero, the module should interpret
this as a hint that it will be closed soon and shouldn't allocate further
resources. This convention is used to implement gdk_pixbuf_get_file_info()
efficiently.
</para>

Let me know if this still doesn't fix things

2007-07-05  Matthias Clasen  <mclasen@redhat.com>

        * io-tiff.c: Revert the last change, it was wrong
        * gdk-pixbuf-loader.c (gdk_pixbuf_loader_close): Redo the last
        change in a different way.

Thanks, it all makes sense now.

I've taken the liberty of committing a comment to tiff_image_parse to explain the early termination signal.

- Mike