| Summary: | REGRESSION(r177637) [HarfBuzz][GTK][EFL] It made 3 performance tests crash and +24 layout tests crashes/failures | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Critical | CC: | cgarcia, clopez, commit-queue, darin, d-r, gyuyoung.kim, kling, koivisto, mmaxfield, mrobinson, ossy, ryuan.choi, zan | ||||
| Priority: | P1 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 139864 | ||||||
| Attachments: |
|
||||||
|
Description
Csaba Osztrogonác
2014-12-23 09:09:18 PST
I don't have time and plan to fix this regression, I only reported it, feel free to pick it up, if you are interested in it. There are layout tests crashes too, but I don't have time to collect the detailed results from always red bots. I have updated the GTK TestExpectations on r178037 <https://trac.webkit.org/r178037> to reflect the 24 tests that crash or fail on the GTK port after r177637. I have double checked this, I tried reverting r177637 and all those test pass as expected, but after r177637 all crash or fail. One of the issues is that before r177637 WebCore::FontGlyphs::glyphDataForCharacter returned sometimes missingGlyphData() and now (after r177637 and r177876) instead returns a null GlyphData(), therefore the crash on WebCore::HarfBuzzShaper::collectHarfBuzzRuns(). I tried this patch: --- a/Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp +++ b/Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp @@ -410,6 +410,8 @@ bool HarfBuzzShaper::collectHarfBuzzRuns() do { const UChar* currentCharacterPosition = iterator.characters(); const SimpleFontData* currentFontData = nextFontData; + if (!currentFontData) + break; UScriptCode currentScript = nextScript; for (iterator.advance(clusterLength); iterator.consume(character, clusterLength); iterator.advance(clusterLength)) { And it fixes the crashes. The performance tests seems to work back and a few of the reported failing layout test pass. *But* there are still many layout tests failing (16 of 24). So it is an incomplete fix. Looking at the diffs of the results, seems that the width of the text run is 0. I hope this information can be useful. I'm not planning to work on this anytime soon, so feel free to pick it. (In reply to comment #4) > One of the issues is that before r177637 > WebCore::FontGlyphs::glyphDataForCharacter returned sometimes > missingGlyphData() and now (after r177637 and r177876) instead returns a > null GlyphData(), therefore the crash on > WebCore::HarfBuzzShaper::collectHarfBuzzRuns(). > > > I tried this patch: > > --- a/Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp > +++ b/Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp > @@ -410,6 +410,8 @@ bool HarfBuzzShaper::collectHarfBuzzRuns() > do { > const UChar* currentCharacterPosition = iterator.characters(); > const SimpleFontData* currentFontData = nextFontData; > + if (!currentFontData) > + break; > UScriptCode currentScript = nextScript; > > for (iterator.advance(clusterLength); iterator.consume(character, > clusterLength); iterator.advance(clusterLength)) { > > > And it fixes the crashes. The performance tests seems to work back and a few > of the reported failing layout test pass. > *But* there are still many layout tests failing (16 of 24). Yes, that's because we render the missing glyph (a square) and breaking the loop we don't render any glyph. That should advance the iterator and continue the loop instead of break, but still the missing characters wouldn't be rendered at all. If we want to keep rendering the missing glyphs as squares we need to use a fallback font I guess. > So it is an > incomplete fix. Looking at the diffs of the results, seems that the width of > the text run is 0. > > > I hope this information can be useful. > > I'm not planning to work on this anytime soon, so feel free to pick it. Created attachment 244254 [details] Patch I think this is equivalent to what we did before r177637, falling back to the primary font data for missing glhyps. I'm still getting a few failures, though, but I'm not sure if it's something related to my system, because I see the missing glyhps but rendered slightly different. I think we can land this anyway for now, since it fixes the crashes, most of the layout tests for sure, and the perf tests. (In reply to comment #6) > Created attachment 244254 [details] > Patch > > I think this is equivalent to what we did before r177637, falling back to > the primary font data for missing glhyps. I'm still getting a few failures, > though, but I'm not sure if it's something related to my system, because I > see the missing glyhps but rendered slightly different. I think we can land > this anyway for now, since it fixes the crashes, most of the layout tests > for sure, and the perf tests. That fixes the crashes, but there are still differences on the expected results of some tests. I'm not sure if that differences still indicate an incomplete fix, or they just need to be rebaselined. With your patch: Regressions: Unexpected text-only failures (15) fast/css/line-height-determined-by-primary-font.html [ Failure ] fast/ruby/bopomofo-letter-spacing.html [ Failure ] fast/ruby/bopomofo-rl.html [ Failure ] fast/ruby/bopomofo.html [ Failure ] fast/text/decorations-with-text-combine.html [ Failure ] fast/text/emphasis-combined-text.html [ Failure ] fast/text/emphasis-vertical.html [ Failure ] fast/text/international/khmer-selection.html [ Failure ] fast/text/international/plane2.html [ Failure ] fast/text/justify-ideograph-complex.html [ Failure ] fast/text/justify-ideograph-simple.html [ Failure ] fast/text/justify-ideograph-vertical.html [ Failure ] fast/text/khmer-lao-font.html [ Failure ] fast/text/midword-break-before-surrogate-pair-2.html [ Failure ] svg/custom/svg-fonts-fallback.xhtml [ Failure ] Check here the diffs: http://people.igalia.com/clopez/bug-139905-results/results.html (In reply to comment #7) > (In reply to comment #6) > > Created attachment 244254 [details] > > Patch > > > > I think this is equivalent to what we did before r177637, falling back to > > the primary font data for missing glhyps. I'm still getting a few failures, > > though, but I'm not sure if it's something related to my system, because I > > see the missing glyhps but rendered slightly different. I think we can land > > this anyway for now, since it fixes the crashes, most of the layout tests > > for sure, and the perf tests. > > That fixes the crashes, but there are still differences on the expected > results of some tests. I'm not sure if that differences still indicate an > incomplete fix, or they just need to be rebaselined. > > With your patch: > > Regressions: Unexpected text-only failures (15) > fast/css/line-height-determined-by-primary-font.html [ Failure ] > fast/ruby/bopomofo-letter-spacing.html [ Failure ] > fast/ruby/bopomofo-rl.html [ Failure ] > fast/ruby/bopomofo.html [ Failure ] > fast/text/decorations-with-text-combine.html [ Failure ] > fast/text/emphasis-combined-text.html [ Failure ] > fast/text/emphasis-vertical.html [ Failure ] > fast/text/international/khmer-selection.html [ Failure ] > fast/text/international/plane2.html [ Failure ] > fast/text/justify-ideograph-complex.html [ Failure ] > fast/text/justify-ideograph-simple.html [ Failure ] > fast/text/justify-ideograph-vertical.html [ Failure ] > fast/text/khmer-lao-font.html [ Failure ] > fast/text/midword-break-before-surrogate-pair-2.html [ Failure ] > svg/custom/svg-fonts-fallback.xhtml [ Failure ] > > Check here the diffs: > http://people.igalia.com/clopez/bug-139905-results/results.html I see, I'm not a fonts expert so I don't know how to properly fix this, so I suggest to land this patch for now to fix the crashes, and open a new bug for the remaining layout test failures. (In reply to comment #8) > I see, I'm not a fonts expert so I don't know how to properly fix this, so I > suggest to land this patch for now to fix the crashes, and open a new bug > for the remaining layout test failures. Sounds reasonable. At least this patch will fix the crashes and get the perf bot back to green. Do you have image results? Do they look like regressions visually? Committed r178115: <http://trac.webkit.org/changeset/178115> (In reply to comment #10) > Do you have image results? Do they look like regressions visually? Yes. Here are the results with images: http://people.igalia.com/clopez/bug-139905-results-img/results.html Based on that I think I'm going to just rebaseline the results for the GTK port. Although there are some differences on the images, none of it seems wrong to me. Actually some of them now look better :) (In reply to comment #12) > (In reply to comment #10) > > Do you have image results? Do they look like regressions visually? > > Yes. > > Here are the results with images: > http://people.igalia.com/clopez/bug-139905-results-img/results.html > > Based on that I think I'm going to just rebaseline the results for the GTK > port. Although there are some differences on the images, none of it seems > wrong to me. Actually some of them now look better :) Are these images with your change or Carlos'? I notice a the missing glyph symbols are now gone completely. (In reply to comment #13) > Are these images with your change or Carlos'? I notice a the missing glyph > symbols are now gone completely. With the one that just landed on r178115. Yes, you are right... i was failing to identify the square as the missing glyph :) I will open a bug. (In reply to comment #14) > (In reply to comment #13) > > Are these images with your change or Carlos'? I notice a the missing glyph > > symbols are now gone completely. > > With the one that just landed on r178115. > > Yes, you are right... i was failing to identify the square as the missing > glyph :) > > I will open a bug. Opened bug: https://bugs.webkit.org/show_bug.cgi?id=140252 |