Bug 19542 - Crash in Font::glyphDataForCharacter when getting small caps data
Summary: Crash in Font::glyphDataForCharacter when getting small caps data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-06-13 18:22 PDT by Brett Wilson (Google)
Modified: 2008-06-22 08:58 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.84 KB, patch)
2008-06-19 11:01 PDT, Brett Wilson (Google)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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!