Summary: | ASSERT_NOT_REACHED reached with broken ideograph and system fallback | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Simonsen <simonjam> | ||||||||||||||||
Component: | New Bugs | Assignee: | 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
James Simonsen
2011-02-01 13:36:51 PST
Created attachment 80818 [details]
Patch
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 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? Nevermind evan/tony. This has little to do with cr-lx. Created attachment 81574 [details]
Patch
(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. Created attachment 81745 [details]
Patch
Comment on attachment 81745 [details]
Patch
Oops. That last patch was meant for a different bug.
Created attachment 84341 [details]
Patch
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.
Created attachment 84647 [details]
Patch
(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?
(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. Attachment 84647 [details] did not build on win: Build output: http://queues.webkit.org/results/8078822 Created attachment 84813 [details]
Patch
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 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.
Created attachment 94552 [details]
Patch
(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 on attachment 94552 [details] Patch Clearing flags on attachment: 94552 Committed r87215: <http://trac.webkit.org/changeset/87215> All reviewed patches have been landed. Closing bug. |