Add note that bug has been fixed. (main): Instrumentation code for testing cache destruction.

Support tests that produce no output, (don't check image if (width,height) == (0,0)).
Add #include <assert.h> here rather than in multiple .c files.
Add const qualifier to static cache_arrangements table. (_cache_sane_state): Remove refcount assertion as it it false during the cairo_cache_destroy. (_cache_sane_state): #include <assert.h> moved up to cairoint.h (_entry_destroy): Fix bug in assertion (used_memory >= entry->memory), not >. (_cairo_cache_destroy): Fix timing of refcount decrement so that the destroy function actually works.

10 files changed, 120 insertions, 39 deletions

diff --git a/BUGS b/BUGS
index 18fcd5d7e..0f852b63f 100644
--- a/BUGS
+++ b/BUGS
@@ -97,3 +97,10 @@ move_to_show_surface (see cairo/test):
 
 cairo falls over with XFree86 4.2 (probably braindead depth handling
 somewhere).
+
+--
+
+The caches abort when asked for a too-large item, (should be possible
+to trigger by asking for a giant font, "cairo_scale_font (cr, 2000)"
+perhaps). Even if the caches don't want to hold them, we need to be
+able to allocate these objects.
diff --git a/ChangeLog b/ChangeLog
index 32aab5614..ba7806a61 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,24 @@
+2004-11-23  Carl Worth  <cworth@cworth.org>
+
+	* test/text_cache_crash.c: Add note that bug has been fixed.
+	(main): Instrumentation code for testing cache destruction.
+
+	* test/cairo_test.c (cairo_test): Support tests that produce no
+	output, (don't check image if (width,height) == (0,0)).
+
+	* src/cairoint.h: Add #include <assert.h> here rather than in
+	multiple .c files.
+
+	* src/cairo_cache.c: Add const qualifier to static
+	cache_arrangements table.
+	(_cache_sane_state): Remove refcount assertion as it it false
+	during the cairo_cache_destroy.
+	(_cache_sane_state): #include <assert.h> moved up to cairoint.h
+	(_entry_destroy): Fix bug in assertion (used_memory >=
+	entry->memory), not >.
+	(_cairo_cache_destroy): Fix timing of refcount decrement so that
+	the destroy function actually works.
+
 2004-11-14  Carl Worth  <cworth@cworth.org>
 
 	* src/cairo_gstate.c (_cairo_gstate_select_font): Don't destroy a
diff --git a/src/cairo-cache.c b/src/cairo-cache.c
index a33d69a04..fffd888ab 100644
--- a/src/cairo-cache.c
+++ b/src/cairo-cache.c
@@ -43,7 +43,7 @@
  * Packard.
  */
 
-static cairo_cache_arrangement_t cache_arrangements [] = {
+static const cairo_cache_arrangement_t cache_arrangements [] = {
     { 16,		43,		41        },
     { 32,		73,		71        },
     { 64,		151,		149       },
@@ -114,7 +114,6 @@ static cairo_cache_arrangement_t cache_arrangements [] = {
  (!((NULL_ENTRY_P((cache),(i))) || (DEAD_ENTRY_P((cache),(i)))))
 
 #ifdef CAIRO_DO_SANITY_CHECKING
-#include <assert.h>
 static void 
 _cache_sane_state (cairo_cache_t *cache)
 {
@@ -122,13 +121,14 @@ _cache_sane_state (cairo_cache_t *cache)
     assert (cache->entries != NULL);
     assert (cache->backend != NULL);
     assert (cache->arrangement != NULL);
+/* XXX: This check is broken during destroy
     assert (cache->refcount > 0);
+*/
     assert (cache->used_memory <= cache->max_memory);
     assert (cache->live_entries <= cache->arrangement->size);   
 }
 #else
 #define _cache_sane_state(c) 
-#define assert(x) 
 #endif
 
 static void
@@ -140,7 +140,7 @@ _entry_destroy (cairo_cache_t *cache, unsigned long i)
     {
 	cairo_cache_entry_base_t *entry = cache->entries[i];
 	assert(cache->live_entries > 0);
-	assert(cache->used_memory > entry->memory);
+	assert(cache->used_memory >= entry->memory);
 
 	cache->live_entries--;
  	cache->used_memory -= entry->memory;
@@ -230,8 +230,7 @@ _find_exact_live_entry_for (cairo_cache_t *cache,
     return _cache_lookup (cache, key, cache->backend->keys_equal);
 }
 
-
-static cairo_cache_arrangement_t *
+static const cairo_cache_arrangement_t *
 _find_cache_arrangement (unsigned long proposed_size)
 {
     unsigned long idx;
@@ -302,7 +301,7 @@ _cairo_cache_init (cairo_cache_t *cache,
 		   const cairo_cache_backend_t *backend,
 		   unsigned long max_memory)
 {    
-    assert(backend != NULL);
+    assert (backend != NULL);
 
     if (cache != NULL){
 	cache->arrangement = &cache_arrangements[0];
@@ -342,7 +341,7 @@ _cairo_cache_destroy (cairo_cache_t *cache)
 	
 	_cache_sane_state (cache);
 
-	if (cache->refcount-- > 0)
+	if (--cache->refcount > 0)
 	    return;
 	
 	for (i = 0; i < cache->arrangement->size; ++i) {
diff --git a/src/cairo-hash.c b/src/cairo-hash.c
index a33d69a04..fffd888ab 100644
--- a/src/cairo-hash.c
+++ b/src/cairo-hash.c
@@ -43,7 +43,7 @@
  * Packard.
  */
 
-static cairo_cache_arrangement_t cache_arrangements [] = {
+static const cairo_cache_arrangement_t cache_arrangements [] = {
     { 16,		43,		41        },
     { 32,		73,		71        },
     { 64,		151,		149       },
@@ -114,7 +114,6 @@ static cairo_cache_arrangement_t cache_arrangements [] = {
  (!((NULL_ENTRY_P((cache),(i))) || (DEAD_ENTRY_P((cache),(i)))))
 
 #ifdef CAIRO_DO_SANITY_CHECKING
-#include <assert.h>
 static void 
 _cache_sane_state (cairo_cache_t *cache)
 {
@@ -122,13 +121,14 @@ _cache_sane_state (cairo_cache_t *cache)
     assert (cache->entries != NULL);
     assert (cache->backend != NULL);
     assert (cache->arrangement != NULL);
+/* XXX: This check is broken during destroy
     assert (cache->refcount > 0);
+*/
     assert (cache->used_memory <= cache->max_memory);
     assert (cache->live_entries <= cache->arrangement->size);   
 }
 #else
 #define _cache_sane_state(c) 
-#define assert(x) 
 #endif
 
 static void
@@ -140,7 +140,7 @@ _entry_destroy (cairo_cache_t *cache, unsigned long i)
     {
 	cairo_cache_entry_base_t *entry = cache->entries[i];
 	assert(cache->live_entries > 0);
-	assert(cache->used_memory > entry->memory);
+	assert(cache->used_memory >= entry->memory);
 
 	cache->live_entries--;
  	cache->used_memory -= entry->memory;
@@ -230,8 +230,7 @@ _find_exact_live_entry_for (cairo_cache_t *cache,
     return _cache_lookup (cache, key, cache->backend->keys_equal);
 }
 
-
-static cairo_cache_arrangement_t *
+static const cairo_cache_arrangement_t *
 _find_cache_arrangement (unsigned long proposed_size)
 {
     unsigned long idx;
@@ -302,7 +301,7 @@ _cairo_cache_init (cairo_cache_t *cache,
 		   const cairo_cache_backend_t *backend,
 		   unsigned long max_memory)
 {    
-    assert(backend != NULL);
+    assert (backend != NULL);
 
     if (cache != NULL){
 	cache->arrangement = &cache_arrangements[0];
@@ -342,7 +341,7 @@ _cairo_cache_destroy (cairo_cache_t *cache)
 	
 	_cache_sane_state (cache);
 
-	if (cache->refcount-- > 0)
+	if (--cache->refcount > 0)
 	    return;
 	
 	for (i = 0; i < cache->arrangement->size; ++i) {
diff --git a/src/cairo_cache.c b/src/cairo_cache.c
index a33d69a04..fffd888ab 100644
--- a/src/cairo_cache.c
+++ b/src/cairo_cache.c
@@ -43,7 +43,7 @@
  * Packard.
  */
 
-static cairo_cache_arrangement_t cache_arrangements [] = {
+static const cairo_cache_arrangement_t cache_arrangements [] = {
     { 16,		43,		41        },
     { 32,		73,		71        },
     { 64,		151,		149       },
@@ -114,7 +114,6 @@ static cairo_cache_arrangement_t cache_arrangements [] = {
  (!((NULL_ENTRY_P((cache),(i))) || (DEAD_ENTRY_P((cache),(i)))))
 
 #ifdef CAIRO_DO_SANITY_CHECKING
-#include <assert.h>
 static void 
 _cache_sane_state (cairo_cache_t *cache)
 {
@@ -122,13 +121,14 @@ _cache_sane_state (cairo_cache_t *cache)
     assert (cache->entries != NULL);
     assert (cache->backend != NULL);
     assert (cache->arrangement != NULL);
+/* XXX: This check is broken during destroy
     assert (cache->refcount > 0);
+*/
     assert (cache->used_memory <= cache->max_memory);
     assert (cache->live_entries <= cache->arrangement->size);   
 }
 #else
 #define _cache_sane_state(c) 
-#define assert(x) 
 #endif
 
 static void
@@ -140,7 +140,7 @@ _entry_destroy (cairo_cache_t *cache, unsigned long i)
     {
 	cairo_cache_entry_base_t *entry = cache->entries[i];
 	assert(cache->live_entries > 0);
-	assert(cache->used_memory > entry->memory);
+	assert(cache->used_memory >= entry->memory);
 
 	cache->live_entries--;
  	cache->used_memory -= entry->memory;
@@ -230,8 +230,7 @@ _find_exact_live_entry_for (cairo_cache_t *cache,
     return _cache_lookup (cache, key, cache->backend->keys_equal);
 }
 
-
-static cairo_cache_arrangement_t *
+static const cairo_cache_arrangement_t *
 _find_cache_arrangement (unsigned long proposed_size)
 {
     unsigned long idx;
@@ -302,7 +301,7 @@ _cairo_cache_init (cairo_cache_t *cache,
 		   const cairo_cache_backend_t *backend,
 		   unsigned long max_memory)
 {    
-    assert(backend != NULL);
+    assert (backend != NULL);
 
     if (cache != NULL){
 	cache->arrangement = &cache_arrangements[0];
@@ -342,7 +341,7 @@ _cairo_cache_destroy (cairo_cache_t *cache)
 	
 	_cache_sane_state (cache);
 
-	if (cache->refcount-- > 0)
+	if (--cache->refcount > 0)
 	    return;
 	
 	for (i = 0; i < cache->arrangement->size; ++i) {
diff --git a/src/cairoint.h b/src/cairoint.h
index be0e54e78..f772e8af0 100644
--- a/src/cairoint.h
+++ b/src/cairoint.h
@@ -49,6 +49,7 @@
 #include "config.h"
 #endif
 
+#include <assert.h>
 #include <stdlib.h>
 #include <string.h>
 #include <math.h>
@@ -276,7 +277,6 @@ typedef struct cairo_cache_backend {
 
 } cairo_cache_backend_t;
 
-
 /* 
  * The cairo_cache system makes the following assumptions about
  * entries in its cache:
@@ -312,7 +312,7 @@ typedef struct {
 typedef struct {
     unsigned long refcount;
     const cairo_cache_backend_t *backend;
-    cairo_cache_arrangement_t *arrangement;
+    const cairo_cache_arrangement_t *arrangement;
     cairo_cache_entry_base_t **entries;
 
     unsigned long max_memory;
@@ -362,7 +362,7 @@ typedef struct {
 } cairo_unscaled_font_t;
 
 /* 
- * A cairo_font contains a pointer to a cairo_sizeless_font_t and a scale
+ * A cairo_font contains a pointer to a cairo_unscaled_font_t and a scale
  * matrix. These are the things the user holds references to.
  */
 
@@ -372,7 +372,6 @@ struct cairo_font {
     cairo_unscaled_font_t *unscaled;
 };
 
-
 /* cairo_font.c is responsible for two global caches: 
  *  
  *   - font entries:  [[[base], name, weight, slant], cairo_unscaled_font_t ]
@@ -416,7 +415,6 @@ _cairo_glyph_cache_keys_equal (void *cache,
 			       void *k1,
 			       void *k2);
 
-
 /* the font backend interface */
 
 typedef struct cairo_font_backend {
diff --git a/test/cairo-test.c b/test/cairo-test.c
index 75327c892..ee3c4ee74 100644
--- a/test/cairo-test.c
+++ b/test/cairo-test.c
@@ -127,6 +127,13 @@ cairo_test (cairo_test_t *test, cairo_test_draw_function_t draw)
 
     cairo_destroy (cr);
 
+    /* Skip image check for tests with no image (width,height == 0,0) */
+    if (test->width == 0 || test->height == 0) {
+	free (png_buf);
+	free (diff_buf);
+	return CAIRO_TEST_SUCCESS;
+    }
+
     /* Then we've got a bunch of string manipulation and file I/O for the check */
     srcdir = getenv ("srcdir");
     if (!srcdir)
diff --git a/test/cairo_test.c b/test/cairo_test.c
index 75327c892..ee3c4ee74 100644
--- a/test/cairo_test.c
+++ b/test/cairo_test.c
@@ -127,6 +127,13 @@ cairo_test (cairo_test_t *test, cairo_test_draw_function_t draw)
 
     cairo_destroy (cr);
 
+    /* Skip image check for tests with no image (width,height == 0,0) */
+    if (test->width == 0 || test->height == 0) {
+	free (png_buf);
+	free (diff_buf);
+	return CAIRO_TEST_SUCCESS;
+    }
+
     /* Then we've got a bunch of string manipulation and file I/O for the check */
     srcdir = getenv ("srcdir");
     if (!srcdir)
diff --git a/test/text-cache-crash.c b/test/text-cache-crash.c
index 56502345e..0a50b9324 100644
--- a/test/text-cache-crash.c
+++ b/test/text-cache-crash.c
@@ -51,17 +51,21 @@
  *	text_cache_crash: cairo_cache.c:422: _cairo_cache_lookup: Assertion `cache->max_memory >= (cache->used_memory + new_entry->memory)' failed.
  *
  *   I'll have to go back and try the original test after I fix this.
+ *
+ * 2004-11-13 Carl Worth <cworth@cworth.org>
+ *
+ *   Found the bug. cairo_gstate_select_font was noticing when the
+ *   same font was selected twice in a row and was erroneously failing
+ *   to free the old reference. Committed a fix and verified it also
+ *   fixed the orginal test case.
  */
 
 #include "cairo_test.h"
 
-#define WIDTH  100
-#define HEIGHT 60
-
 cairo_test_t test = {
     "text_cache_crash",
     "Test case for bug causing an assertion failure in _cairo_cache_lookup",
-    WIDTH, HEIGHT
+    0, 0,
 };
 #include <cairo.h>
 
@@ -80,6 +84,24 @@ draw (cairo_t *cr, int width, int height)
 int
 main (void)
 {
-    return cairo_test (&test, draw);
+    int ret;
+
+    ret = cairo_test (&test, draw);
+
+    /* It's convenient to be able to free all memory (including
+     * statically allocated memory). This makes it quite easy to use
+     * tools such as valgrind to verify that there are no memory leaks
+     * whatsoever.
+     *
+     * But I'm not sure what would be a sensible cairo API function
+     * for this. The cairo_destroy_caches call below is just something
+     * I made as a local modification to cairo.
+     */
+    /*
+    cairo_destroy_caches ();
+    FcFini ();
+    */
+
+    return ret;
 }
 
diff --git a/test/text_cache_crash.c b/test/text_cache_crash.c
index 56502345e..0a50b9324 100644
--- a/test/text_cache_crash.c
+++ b/test/text_cache_crash.c
@@ -51,17 +51,21 @@
  *	text_cache_crash: cairo_cache.c:422: _cairo_cache_lookup: Assertion `cache->max_memory >= (cache->used_memory + new_entry->memory)' failed.
  *
  *   I'll have to go back and try the original test after I fix this.
+ *
+ * 2004-11-13 Carl Worth <cworth@cworth.org>
+ *
+ *   Found the bug. cairo_gstate_select_font was noticing when the
+ *   same font was selected twice in a row and was erroneously failing
+ *   to free the old reference. Committed a fix and verified it also
+ *   fixed the orginal test case.
  */
 
 #include "cairo_test.h"
 
-#define WIDTH  100
-#define HEIGHT 60
-
 cairo_test_t test = {
     "text_cache_crash",
     "Test case for bug causing an assertion failure in _cairo_cache_lookup",
-    WIDTH, HEIGHT
+    0, 0,
 };
 #include <cairo.h>
 
@@ -80,6 +84,24 @@ draw (cairo_t *cr, int width, int height)
 int
 main (void)
 {
-    return cairo_test (&test, draw);
+    int ret;
+
+    ret = cairo_test (&test, draw);
+
+    /* It's convenient to be able to free all memory (including
+     * statically allocated memory). This makes it quite easy to use
+     * tools such as valgrind to verify that there are no memory leaks
+     * whatsoever.
+     *
+     * But I'm not sure what would be a sensible cairo API function
+     * for this. The cairo_destroy_caches call below is just something
+     * I made as a local modification to cairo.
+     */
+    /*
+    cairo_destroy_caches ();
+    FcFini ();
+    */
+
+    return ret;
 }