Bug 15397 - Layout tests freeze in HashTable::lookup
Summary: Layout tests freeze in HashTable::lookup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac (PowerPC) OS X 10.4
: P1 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-10-06 05:24 PDT by Alexey Proskuryakov
Modified: 2007-10-08 11:15 PDT (History)
1 user (show)

See Also:


Attachments
proposed fix (1.80 KB, patch)
2007-10-06 05:29 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2007-10-06 05:24:43 PDT
The below reliably freezes for me:

$ run-webkit-tests svg/W3C-SVG-1.1/text-align-06-b.svg --pixel

The problem is in FontPlatformData constructor: "sending to nil" Cocoa magic doesn't work with non-integral values:

    if (f)
        CFRetain(f);
    m_size = [f pointSize];

This results in emptyValue being non-zero, and deeply confuses WTF::HashTable.
Comment 1 Alexey Proskuryakov 2007-10-06 05:29:32 PDT
Created attachment 16567 [details]
proposed fix
Comment 2 Alexey Proskuryakov 2007-10-06 05:38:34 PDT
Looks like this issue isn't present on trunk - at least, I couldn't find such code there.
Comment 3 Eric Seidel (no email) 2007-10-06 19:25:55 PDT
Comment on attachment 16567 [details]
proposed fix

The change looks sane. I don't understand the removal of the -1 checks (or why they were there in the first place).
Comment 4 Alexey Proskuryakov 2007-10-07 01:44:48 PDT
I have removed the -1 checks because there was an unchecked call to [font pointSize] in this function anyway - so guarding against the same problem in other places wasn't really helpful.
Comment 5 Darin Adler 2007-10-08 08:28:47 PDT
Comment on attachment 16567 [details]
proposed fix

If the checks for -1 really aren't necessary, then perhaps m_font can be changed into a RetainPtr?

r=me
Comment 6 Alexey Proskuryakov 2007-10-08 09:27:53 PDT
> If the checks for -1 really aren't necessary, then perhaps m_font can be
> changed into a RetainPtr?

It's OK for m_font to be -1 (it's a special value used for "Deleted"), but apparently not for setFont() parameter.
Comment 7 Alexey Proskuryakov 2007-10-08 09:31:20 PDT
Committed revision 26105.
Comment 8 Dave Hyatt 2007-10-08 10:38:06 PDT
Thanks for catching this.
Comment 9 Alexey Proskuryakov 2007-10-08 11:15:57 PDT
(In reply to comment #6)
> It's OK for m_font to be -1 (it's a special value used for "Deleted"), but
> apparently not for setFont() parameter.

Oops, one of the removed checks was necessary in fact:
-    if (m_font && m_font != (NSFont*)-1)
+    if (m_font)

Restored it in r26107.