Bug 139905 – REGRESSION(r177637) [HarfBuzz][GTK][EFL] It made 3 performance tests crash and +24 layout tests crashes/failures

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

https://trac.webkit.org/changeset/177637 made 3 performance tests 
crash on the EFL and GTK perf bots:
- GTK: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Perf%29/builds/1547
- EFL: https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/4194

Unfortunately the GTK build was broken that time and EFL bot picked up a huge interval,
but I checked the EFL build on r177684 (ToT) with reverting r177637 and I can confirm
that r177637 caused this regression.

The EFL bot provides us some crash backtrace, maybe they can be useful to fix the regression:

Running Interactive/window-resize.html (71 of 141)
error: Interactive/window-resize.html
1   0x7fe533349c00
2   0x7fe53350aff0
3   0x7fe5351a7dca WebCore::SimpleFontData::canRenderCombiningCharacterSequence(unsigned short const*, unsigned long) const
4   0x7fe5351bc16c WebCore::HarfBuzzShaper::collectHarfBuzzRuns()
5   0x7fe5351bc588 WebCore::HarfBuzzShaper::shape(WebCore::GlyphBuffer*)
6   0x7fe5351994eb WebCore::Font::floatWidthForComplexText(WebCore::TextRun const&, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const
7   0x7fe534e0a218 WebCore::Font::width(WebCore::TextRun const&, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const
8   0x7fe534ff7d9f WebCore::RenderText::computePreferredLogicalWidths(float, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >&, WebCore::GlyphOverflow&)
9   0x7fe534ffa753 WebCore::RenderText::computePreferredLogicalWidths(float)
10  0x7fe534ff4abe WebCore::RenderText::trimmedPrefWidths(float, float&, bool&, float&, bool&, bool&, bool&, float&, float&, float&, float&, bool&)
11  0x7fe534ed2d73 WebCore::RenderBlockFlow::computeInlinePreferredLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const
12  0x7fe534ed3ef9 WebCore::RenderBlockFlow::computeIntrinsicLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const
13  0x7fe534eafe9c WebCore::RenderBlock::computePreferredLogicalWidths()
14  0x7fe534fe0c21 WebCore::RenderTableCell::computePreferredLogicalWidths()
15  0x7fe534ee6a8b WebCore::RenderBox::minPreferredLogicalWidth() const
16  0x7fe5356c967a WebCore::AutoTableLayout::recalcColumn(unsigned int)
17  0x7fe5356ca86f WebCore::AutoTableLayout::fullRecalc()
18  0x7fe5356cb235 WebCore::AutoTableLayout::computeIntrinsicLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&)
19  0x7fe534fd5e4a WebCore::RenderTable::computePreferredLogicalWidths()
20  0x7fe534ee6abb WebCore::RenderBox::maxPreferredLogicalWidth() const
21  0x7fe534fd2c81 WebCore::RenderTable::updateLogicalWidth()
22  0x7fe534fd65eb WebCore::RenderTable::layout()
23  0x7fe534ed0834 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
24  0x7fe534ed1956 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
25  0x7fe534ed6406 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
26  0x7fe534ebb261 WebCore::RenderBlock::layout()
27  0x7fe534f39d69 WebCore::RenderFlowThread::layout()
28  0x7fe534fa5651 WebCore::RenderMultiColumnFlowThread::layout()
29  0x7fe534ed84d8 WebCore::RenderBlockFlow::layoutSpecialExcludedChild(bool)
30  0x7fe534ed18cd WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
31  0x7fe534ed6406 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)

FAILED
Finished: 14.946824 s


Running Parser/HTML5-8266-FullRender.html (110 of 141)
error: Parser/HTML5-8266-FullRender.html
1   0x7f382957cc00
2   0x7f382973dff0
3   0x7f382b3dadca WebCore::SimpleFontData::canRenderCombiningCharacterSequence(unsigned short const*, unsigned long) const
4   0x7f382b3ef16c WebCore::HarfBuzzShaper::collectHarfBuzzRuns()
5   0x7f382b3ef588 WebCore::HarfBuzzShaper::shape(WebCore::GlyphBuffer*)
6   0x7f382b3cc4eb WebCore::Font::floatWidthForComplexText(WebCore::TextRun const&, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const
7   0x7f382b03d218 WebCore::Font::width(WebCore::TextRun const&, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const
8   0x7f382b22ad9f WebCore::RenderText::computePreferredLogicalWidths(float, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >&, WebCore::GlyphOverflow&)
9   0x7f382b22d753 WebCore::RenderText::computePreferredLogicalWidths(float)
10  0x7f382b227abe WebCore::RenderText::trimmedPrefWidths(float, float&, bool&, float&, bool&, bool&, bool&, float&, float&, float&, float&, bool&)
11  0x7f382b105d73 WebCore::RenderBlockFlow::computeInlinePreferredLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const
12  0x7f382b106ef9 WebCore::RenderBlockFlow::computeIntrinsicLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const
13  0x7f382b0e2e9c WebCore::RenderBlock::computePreferredLogicalWidths()
14  0x7f382b213c21 WebCore::RenderTableCell::computePreferredLogicalWidths()
15  0x7f382b119a8b WebCore::RenderBox::minPreferredLogicalWidth() const
16  0x7f382b8fc67a WebCore::AutoTableLayout::recalcColumn(unsigned int)
17  0x7f382b8fd86f WebCore::AutoTableLayout::fullRecalc()
18  0x7f382b8fe235 WebCore::AutoTableLayout::computeIntrinsicLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&)
19  0x7f382b208e4a WebCore::RenderTable::computePreferredLogicalWidths()
20  0x7f382b119abb WebCore::RenderBox::maxPreferredLogicalWidth() const
21  0x7f382b205c81 WebCore::RenderTable::updateLogicalWidth()
22  0x7f382b2095eb WebCore::RenderTable::layout()
23  0x7f382b103834 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
24  0x7f382b104956 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
25  0x7f382b109406 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
26  0x7f382b0ee261 WebCore::RenderBlock::layout()
27  0x7f382b16cd69 WebCore::RenderFlowThread::layout()
28  0x7f382b1d8651 WebCore::RenderMultiColumnFlowThread::layout()
29  0x7f382b10b4d8 WebCore::RenderBlockFlow::layoutSpecialExcludedChild(bool)
30  0x7f382b1048cd WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
31  0x7f382b109406 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)

FAILED
Finished: 6.431418 s



Running Parser/html5-full-render.html (114 of 141)
error: Parser/html5-full-render.html
1   0x7f6b64e9ec00
2   0x7f6b6505fff0
3   0x7f6b66cfcdca WebCore::SimpleFontData::canRenderCombiningCharacterSequence(unsigned short const*, unsigned long) const
4   0x7f6b66d1116c WebCore::HarfBuzzShaper::collectHarfBuzzRuns()
5   0x7f6b66d11588 WebCore::HarfBuzzShaper::shape(WebCore::GlyphBuffer*)
6   0x7f6b66cee4eb WebCore::Font::floatWidthForComplexText(WebCore::TextRun const&, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const
7   0x7f6b6695f218 WebCore::Font::width(WebCore::TextRun const&, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const
8   0x7f6b66b4cd9f WebCore::RenderText::computePreferredLogicalWidths(float, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >&, WebCore::GlyphOverflow&)
9   0x7f6b66b4f753 WebCore::RenderText::computePreferredLogicalWidths(float)
10  0x7f6b66b49abe WebCore::RenderText::trimmedPrefWidths(float, float&, bool&, float&, bool&, bool&, bool&, float&, float&, float&, float&, bool&)
11  0x7f6b66a27d73 WebCore::RenderBlockFlow::computeInlinePreferredLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const
12  0x7f6b66a28ef9 WebCore::RenderBlockFlow::computeIntrinsicLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const
13  0x7f6b66a04e9c WebCore::RenderBlock::computePreferredLogicalWidths()
14  0x7f6b66b35c21 WebCore::RenderTableCell::computePreferredLogicalWidths()
15  0x7f6b66a3ba8b WebCore::RenderBox::minPreferredLogicalWidth() const
16  0x7f6b6721e67a WebCore::AutoTableLayout::recalcColumn(unsigned int)
17  0x7f6b6721f86f WebCore::AutoTableLayout::fullRecalc()
18  0x7f6b67220235 WebCore::AutoTableLayout::computeIntrinsicLogicalWidths(WebCore::LayoutUnit&, WebCore::LayoutUnit&)
19  0x7f6b66b2ae4a WebCore::RenderTable::computePreferredLogicalWidths()
20  0x7f6b66a3babb WebCore::RenderBox::maxPreferredLogicalWidth() const
21  0x7f6b66b27c81 WebCore::RenderTable::updateLogicalWidth()
22  0x7f6b66b2b5eb WebCore::RenderTable::layout()
23  0x7f6b66a25834 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
24  0x7f6b66a26956 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
25  0x7f6b66a2b406 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
26  0x7f6b66a10261 WebCore::RenderBlock::layout()
27  0x7f6b66a8ed69 WebCore::RenderFlowThread::layout()
28  0x7f6b66afa651 WebCore::RenderMultiColumnFlowThread::layout()
29  0x7f6b66a2d4d8 WebCore::RenderBlockFlow::layoutSpecialExcludedChild(bool)
30  0x7f6b66a268cd WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
31  0x7f6b66a2b406 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)

FAILED
Finished: 5.081323 s

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

There are layout tests crashes too, but I don't have time to collect the detailed results from always red bots.

[Carlos Alberto Lopez Perez]

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.

[Carlos Alberto Lopez Perez]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Carlos Alberto Lopez Perez]

(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

[Carlos Garcia Campos]

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

[Carlos Alberto Lopez Perez]

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

[Antti Koivisto]

Do you have image results? Do they look like regressions visually?

[Carlos Garcia Campos]

Committed r178115: <http://trac.webkit.org/changeset/178115>

[Carlos Alberto Lopez Perez]

(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 :)

[Martin Robinson]

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

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

(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