ASSERT_NOT_REACHED reached with broken ideograph and system fallback

[James Simonsen]

ASSERT_NOT_REACHED reached with broken ideograph and system fallback

[James Simonsen]

Created attachment 80818 [details]
Patch

[James Simonsen]

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.

[David Levin]

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?

[Eric Seidel (no email)]

Nevermind evan/tony.  This has little to do with cr-lx.

[James Simonsen]

Created attachment 81574 [details]
Patch

[James Simonsen]

(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.

[James Simonsen]

Created attachment 81745 [details]
Patch

[James Simonsen]

Comment on attachment 81745 [details]
Patch

Oops. That last patch was meant for a different bug.

[James Simonsen]

Created attachment 84341 [details]
Patch

[James Simonsen]

I updated the patch. I'd appreciate it if one of you guys could take a look.

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.

[James Simonsen]

Created attachment 84647 [details]
Patch

[James Simonsen]

(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.

Did you mean to flag attachment 84647 [details] for review?

[James Simonsen]

(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.

[Build Bot]

Attachment 84647 [details] did not build on win:
Build output: http://queues.webkit.org/results/8078822

[James Simonsen]

Created attachment 84813 [details]
Patch

[Adam Barth]

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?

[Eric Seidel (no email)]

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.

[James Simonsen]

Created attachment 94552 [details]
Patch

[James Simonsen]

(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.

[WebKit Commit Bot]

Comment on attachment 94552 [details]
Patch

Clearing flags on attachment: 94552

Committed r87215: <http://trac.webkit.org/changeset/87215>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.