[cairo] [cairo-commit] font_freeze changes

[Chris Wilson]

On Wed, 2008-10-22 at 12:24 -0700, Carl Worth wrote:
> On Tue, 2008-10-21 at 16:54 -0700, Chris Wilson wrote:
> > New commits:
> > commit 1db8949f2baf1e620e1d5ef73a66de211420bd0a
> > Author: Chris Wilson <chris at chris-wilson.co.uk>
> > Date:   Tue Oct 21 22:48:17 2008 +0100
> > 
> >     Ensure that the scaled font is frozen for the lifetime of the scaled glyph.
> ...
> > +void
> > +_cairo_scaled_font_fini (cairo_scaled_font_t *scaled_font)
> > +{
> > +    /* Release the lock to avoid the possibility of a recursive
> > +     * deadlock when the scaled font destroy closure gets called. */
> > +    CAIRO_MUTEX_UNLOCK (_cairo_scaled_font_map_mutex);
> > +    _cairo_scaled_font_fini_internal (scaled_font);
> > +    CAIRO_MUTEX_LOCK (_cairo_scaled_font_map_mutex);
> > +}
> 
> Do we want an assert (CAIRO_MUTEX_IS_LOCKED) in that function?

I felt is was redundant with the CAIRO_MUTEX_UNLOCK, since an attempt to
unlock will be caught and it added no further documentation value.

> ...
> > diff --git a/src/cairo-win32-font.c b/src/cairo-win32-font.c
> > index f82d528..dc93758 100644
> > --- a/src/cairo-win32-font.c
> > +++ b/src/cairo-win32-font.c
> > @@ -640,21 +640,25 @@ _cairo_win32_scaled_font_type1_text_to_glyphs (cairo_win32_scaled_font_t *scaled
> >  
> >      if (GetGlyphIndicesW (hdc, utf16, n16, glyph_indices, 0) == GDI_ERROR) {
> >  	status = _cairo_win32_print_gdi_error ("_cairo_win32_scaled_font_type1_text_to_glyphs:GetGlyphIndicesW");
> > -	goto FAIL2;
> > +	goto FAIL3;
> >      }
> >  
> >      *num_glyphs = n16;
> >      *glyphs = _cairo_malloc_ab (n16, sizeof (cairo_glyph_t));
> >      if (!*glyphs) {
> >  	status = _cairo_error (CAIRO_STATUS_NO_MEMORY);
> > -	goto FAIL2;
> > +	goto FAIL3;
> >      }
> >  
> >      x_pos = x;
> >      y_pos = y;
> > +
> >      mat = scaled_font->base.ctm;
> >      status = cairo_matrix_invert (&mat);
> >      assert (status == CAIRO_STATUS_SUCCESS);
> > +
> > +    _cairo_scaled_font_freeze_cache (&scaled_font->base);
> > +
> >      for (i = 0; i < n16; i++) {
> >  	cairo_scaled_glyph_t *scaled_glyph;
> >  
> > @@ -668,7 +672,7 @@ _cairo_win32_scaled_font_type1_text_to_glyphs (cairo_win32_scaled_font_t *scaled
> >  					     &scaled_glyph);
> >  	if (status) {
> >  	    free (*glyphs);
> > -	    goto FAIL2;
> > +	    goto FAIL3;
> >  	}
> >  
> >  	x = scaled_glyph->x_advance;
> > @@ -678,6 +682,8 @@ _cairo_win32_scaled_font_type1_text_to_glyphs (cairo_win32_scaled_font_t *scaled
> >  	y_pos += y;
> >      }
> >  
> > +FAIL3:
> > +    _cairo_scaled_font_thaw_cache (&scaled_font->base);
> >      cairo_win32_scaled_font_done_font (&scaled_font->base);
> >  
> >  FAIL2:
> 
> This looks wrong to me. The two new gotos to FAIL3 will make it attempt
> to thaw the cache without any call to freeze_cache, no?

Indeed that is bogus. I felt justified at the time, I then probably
moved the freeze and broke the error paths.

> I'm feeling quite nervous seeing all these locking changes go by just as
> we're trying to push 1.8.2 out the door. :-/

I've tested image/pdf/ps/svg/test*/xlib under linux and I'm confident
about the locking there (well, for all the code paths covered by the
test suite). I've run make check under win32 as well, but I haven't yet
tried that under memfault/lockdep yet (since valgrind is a linux mostly
tool, that will require running valgrind on the wine binaries). Quartz
I've not even tried compiling, for lack of a host.
-- 
Chris