Bug 53528

Summary: ASSERT_NOT_REACHED reached with broken ideograph and system fallback
Product: WebKit Reporter: James Simonsen <simonjam>
Component: New BugsAssignee: James Simonsen <simonjam>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, eric, evan, hyatt, mitz, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description James Simonsen 2011-02-01 13:36:51 PST
ASSERT_NOT_REACHED reached with broken ideograph and system fallback
Comment 1 James Simonsen 2011-02-01 13:39:51 PST
Created attachment 80818 [details]
Patch
Comment 2 James Simonsen 2011-02-01 13:45:08 PST
We hit the ASSERT_NOT_REACHED in Font::glyphDataForCharacter() on chromium linux with one of the new layout tests. It seems to happen because we have missing glyphs that we're only able to get by relying on the system fallback. That will update the normal page, but not the corresponding page for broken ideographs, so the assert fails.

I'm new to this code, so I'd definitely appreciate your take on it.
Comment 3 David Levin 2011-02-07 13:17:58 PST
Comment on attachment 80818 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80818&action=review

> Source/WebCore/platform/graphics/FontFastPath.cpp:171
> +    else if (isBrokenIdeograph(characterFontData, c)) {

Why does the previous if check for characterFontData being 0 and this one does not?
Comment 4 Eric Seidel 2011-02-07 15:10:15 PST
Nevermind evan/tony.  This has little to do with cr-lx.
Comment 5 James Simonsen 2011-02-07 20:40:06 PST
Created attachment 81574 [details]
Patch
Comment 6 James Simonsen 2011-02-07 20:40:20 PST
(In reply to comment #3)
> (From update of attachment 80818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80818&action=review
> 
> > Source/WebCore/platform/graphics/FontFastPath.cpp:171
> > +    else if (isBrokenIdeograph(characterFontData, c)) {
> 
> Why does the previous if check for characterFontData being 0 and this one does not?

Good catch. Fixed.
Comment 7 James Simonsen 2011-02-08 21:47:44 PST
Created attachment 81745 [details]
Patch
Comment 8 James Simonsen 2011-02-08 21:49:58 PST
Comment on attachment 81745 [details]
Patch

Oops. That last patch was meant for a different bug.
Comment 9 James Simonsen 2011-03-01 18:07:42 PST
Created attachment 84341 [details]
Patch
Comment 10 James Simonsen 2011-03-01 18:08:42 PST
I updated the patch. I'd appreciate it if one of you guys could take a look.
Comment 11 mitz 2011-03-03 10:58:16 PST
Comment on attachment 84341 [details]
Patch

This is OK, but I think it would be a lot better to add a BrokenIdeograph value to FontDataVariant and just change the variant to BrokenIdeograph in this case.
Comment 12 James Simonsen 2011-03-03 16:19:22 PST
Created attachment 84647 [details]
Patch
Comment 13 James Simonsen 2011-03-03 16:20:09 PST
(In reply to comment #11)
> (From update of attachment 84341 [details])
> This is OK, but I think it would be a lot better to add a BrokenIdeograph value to FontDataVariant and just change the variant to BrokenIdeograph in this case.

Yeah, that turned out nice. I've done that in this patch.
Comment 14 mitz 2011-03-03 16:23:01 PST
Did you mean to flag attachment 84647 [details] for review?
Comment 15 James Simonsen 2011-03-03 16:24:53 PST
(In reply to comment #14)
> Did you mean to flag attachment 84647 [details] for review?

Yeah, it'd be nice if you took another look.
Comment 16 Build Bot 2011-03-03 20:22:42 PST
Attachment 84647 [details] did not build on win:
Build output: http://queues.webkit.org/results/8078822
Comment 17 James Simonsen 2011-03-04 15:25:55 PST
Created attachment 84813 [details]
Patch
Comment 18 Adam Barth 2011-05-05 01:35:07 PDT
Comment on attachment 84813 [details]
Patch

Looks like you did what mitz requested.  I'm tempted to mark this R+, but I'm not familiar with this code.  Mitz, would you be willing to take another look at this crash fix?
Comment 19 Eric Seidel 2011-05-23 14:42:18 PDT
Comment on attachment 84813 [details]
Patch

Looks reasonable to me.  Mitz is of course still welcome/encouraged to comment, but this has been up for over a month and looks reasonable.
Comment 20 James Simonsen 2011-05-23 20:39:35 PDT
Created attachment 94552 [details]
Patch
Comment 21 James Simonsen 2011-05-23 20:40:39 PDT
(In reply to comment #19)
> (From update of attachment 84813 [details])
> Looks reasonable to me.  Mitz is of course still welcome/encouraged to comment, but this has been up for over a month and looks reasonable.

I merged this with the changes to the file since I initially uploaded the patch. I'll land it tomorrow, barring any objections.
Comment 22 WebKit Commit Bot 2011-05-24 15:12:36 PDT
Comment on attachment 94552 [details]
Patch

Clearing flags on attachment: 94552

Committed r87215: <http://trac.webkit.org/changeset/87215>
Comment 23 WebKit Commit Bot 2011-05-24 15:12:44 PDT
All reviewed patches have been landed.  Closing bug.