Bug 19542

Summary: Crash in Font::glyphDataForCharacter when getting small caps data
Product: WebKit Reporter: Brett Wilson (Google) <brettw>
Component: TextAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, mitz
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch hyatt: review+

Description Brett Wilson (Google) 2008-06-13 18:22:44 PDT
In Font::glyphDataForCharacter there is this code

  GlyphPageTreeNode* smallCapsNode =
      GlyphPageTreeNode::getRootChild(smallCapsFontData, pageNumber);
  const GlyphData& data = smallCapsNode->page()->glyphDataForCharacter(c);

I got a crash report that shows page() returning NULL here, which caused a crash, although I do not have a repro. In GlyphPageTreeNode.h, it says:

// Returns a page of glyphs (or NULL if there are no glyphs in this page's character range).
GlyphPage* page() const { return m_page.get(); }

So it looks like this Font.cpp code is wrong. Other callers of page() in this function NULL check it, but not this small caps case. It looks like we just need to add a check here.
Comment 1 Mark Rowe (bdash) 2008-06-13 22:17:02 PDT
<rdar://problem/6007976>
Comment 2 Eric Seidel (no email) 2008-06-13 22:42:36 PDT
This should get the page cleared:

<script>
 var ifr = document.createElement('iframe');
 ifr.onload = function() {
   var win = ifr.contentWindow;
   // Remove, pow pageDestroyed.
   ifr.parentNode.removeChild(ifr);
   // Your Page is now cleared, do as you will to reproduce the rest of the bug!
 };
 document.body.appendChild(ifr);
</script>

probably setting some font, or something to get it to relayout/resize a form control in smallcaps should trigger it.
Comment 3 mitz 2008-06-15 19:58:17 PDT
(In reply to comment #2)
> This should get the page cleared:

I think it's a different kind of page :)
Comment 4 Eric Seidel (no email) 2008-06-15 20:27:11 PDT
Oh yeah.  You're right... :)
Comment 5 Brett Wilson (Google) 2008-06-19 11:01:05 PDT
Created attachment 21842 [details]
Patch

This patch just adds a NULL check for the page() of glyphs like the rest of the file. If this fails, it does the same thing it would do if the GlyphData in the page is NULL.

I did not add a test. This patch is based on a crash report I saw. The stack is clear that the crash is dereferencing a NULL from the page() here, but I can not reproduce, even opening the page that triggered the crash report. I also tried to generate some small caps text in a funny language that wouldn't be in the font, but I could not trigger it. It is probably highly dependent on the WebKit port, OS, and installed fonts. If you have an idea for a test, I'll be happy to write it.
Comment 6 Dave Hyatt 2008-06-19 13:10:51 PDT
Comment on attachment 21842 [details]
Patch

r=me
Comment 7 mitz 2008-06-21 12:10:11 PDT
Landed in <http://trac.webkit.org/changeset/34717>.
Comment 8 Brett Wilson (Google) 2008-06-22 08:58:05 PDT
Thanks mitz!