Bug 38049 - Hash table contains zero referenced font faces!
Summary: Hash table contains zero referenced font faces!
Status: RESOLVED FIXED
Alias: None
Product: cairo
Classification: Unclassified
Component: win32 backend (show other bugs)
Version: 1.10.3
Hardware: All Windows (All)
: medium critical
Assignee: Maximilian Heinzler
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-07 12:58 UTC by Maximilian Heinzler
Modified: 2011-06-20 01:27 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Simple example that shows the bug under windows. (1.01 KB, text/x-csrc)
2011-06-08 10:52 UTC, Maximilian Heinzler
Details
Implementation of win32 font destruction (2.84 KB, patch)
2011-06-09 00:18 UTC, Andrea Canciani
Details | Splinter Review

Description Maximilian Heinzler 2011-06-07 12:58:36 UTC
After two hours of debugging and searching I found the source of this bug.
The win32 backend does not remove font faces from its hash table it they are about to be destroyed.

I think this code will fix it:
static void
_cairo_win32_font_face_destroy (void *abstract_face)
{
	cairo_win32_font_face_t *font_face = abstract_face;

	_cairo_hash_table_remove (cairo_win32_font_face_hash_table,
				      &font_face->base.hash_entry);
}

I don't know for sure if there also has to be an check for "font_face->base.hash_entry.hash != 0" like the toy-font backend has it.
Comment 1 Andrea Canciani 2011-06-08 03:10:20 UTC
(In reply to comment #0)
> After two hours of debugging and searching I found the source of this bug.

Which bug?

I get a font-related crash in the testsuite on win32, but it is not fixed by your patch, so I assume you are seeing some other crash.

> The win32 backend does not remove font faces from its hash table it they are
> about to be destroyed.
> 
> I think this code will fix it:
> static void
> _cairo_win32_font_face_destroy (void *abstract_face)
> {
>     cairo_win32_font_face_t *font_face = abstract_face;
> 
>     _cairo_hash_table_remove (cairo_win32_font_face_hash_table,
>                       &font_face->base.hash_entry);
> }
> 
> I don't know for sure if there also has to be an check for
> "font_face->base.hash_entry.hash != 0" like the toy-font backend has it.

I think that you should not be accessing cairo_win32_font_face_hash_table directly, but using _lock()/_unlock().
Comment 2 Maximilian Heinzler 2011-06-08 10:52:09 UTC
Created attachment 47726 [details]
Simple example that shows the bug under windows.

I've added an file that shows the bug i'm talking about. It also includes the whole patch.

Maybe the test crashes because there is a second thing i have forgotten. In the "_cairo_win32_font_face_hash_table_destroy" there is an argument missing to "_cairo_hash_table_random_entry". The attachment includes this.

This is the first time i'm using git but i will try myself to upload this patch and other patches.
Comment 3 Andrea Canciani 2011-06-09 00:18:39 UTC
Created attachment 47754 [details] [review]
Implementation of win32 font destruction
Comment 4 Andrea Canciani 2011-06-09 00:19:12 UTC
(In reply to comment #2)
> Created an attachment (id=47726) [details]
> Simple example that shows the bug under windows.
> 
> I've added an file that shows the bug i'm talking about. It also includes the
> whole patch.

I can confirm that your program triggers the bug and your patch fixes it.

> 
> Maybe the test crashes because there is a second thing i have forgotten. In the
> "_cairo_win32_font_face_hash_table_destroy" there is an argument missing to
> "_cairo_hash_table_random_entry". The attachment includes this.
> 
> This is the first time i'm using git but i will try myself to upload this patch
> and other patches.

I tried to improve your patch a little (see the new attachment).

Some investigation of the magic 0 hash might be needed before merging (and I'm probably going to make your program a cairo testcase so that it can be easily run along with the other regression tests).
Comment 5 Andrea Canciani 2011-06-09 01:06:59 UTC
Sorry, I was wrong about being able to reproduce this bug.
I'm still seeing another bug (which might be also interacting with this one) which seems to be in toy fonts on win32.
Having cairo/master working on win32 would be a good start to easily reproduce/fix bugs :(
Comment 6 Maximilian Heinzler 2011-06-10 10:28:20 UTC
Could you please tell me which file or code leads to the bug your are talking about?
Comment 7 Andrea Canciani 2011-06-17 10:53:18 UTC
(In reply to comment #6)
> Could you please tell me which file or code leads to the bug your are talking
> about?

In the last few days I pushed some commits to fix the win32 build, now it should be easier to reproduce the problem.

I also pushed a branch containing some more win32 (and/or font) related fixes here:
http://cgit.freedesktop.org/~ranma42/cairo/log/?h=wip/win32-font

Does your program hit the assert if linked to this branch?
Comment 8 Maximilian Heinzler 2011-06-18 02:27:20 UTC
No, it works fine.
Thank you!
Comment 9 Andrea Canciani 2011-06-20 01:27:37 UTC
It should be fixed in master by:

commit 94bc20da50c5984e5c04929a7fde4c2f04e66380
Author: Andrea Canciani <ranma42@gmail.com>
Date:   Fri Jun 17 19:15:44 2011 +0200

    win32-font: Implement destroy function
    
    Win32 font faces can be removed from the hashtable upon destruction.
    Based on the toy-font destruction code.
    
    See https://bugs.freedesktop.org/show_bug.cgi?id=38049

and


commit 101fab7cd8a90f7cf3d8113c792b3f8c2a9afb7d
Author: Andrea Canciani <ranma42@gmail.com>
Date:   Wed Jun 15 11:37:36 2011 +0200

    win32-font: Improve static data reset function
    
    The hashtable is guaranteed to only contain font faces which are
    currently referenced, hence there is no need to remove any font face
    when it is reset (just like for toy-font).
    
    This makes the function simpler and fixes the assertion
    
    Assertion failed: predicate != NULL, file cairo-hash.c, line 373
    
    hit by multiple tests (the first one being "clear").
    
    See https://bugs.freedesktop.org/show_bug.cgi?id=38049


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.