Bug 164500 – [HarfBuzz] HarfBuzzShaper should not assume numGlyphs is greater than 0

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

Created attachment 294113 [details]
test.html

The result of shaping can be no glyphs because 
hb_ot_hide_default_ignorables deletes ignorable glyphs.
https://github.com/behdad/harfbuzz/blob/1.3.3/src/hb-ot-shape.cc#L470

> Thread 1 "WebKitWebProces" received signal SIGSEGV, Segmentation fault.
> 0x00007fe52f765afd in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:323
> 323	    *(int *)(uintptr_t)0xbbadbeef = 0;
> (gdb) bt
> #0  0x00007fe52f765afd in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:323
> #1  0x00007fe5358b0912 in WTF::CrashOnOverflow::crash () at ../../Source/WTF/wtf/CheckedArithmetic.h:85
> #2  0x00007fe5358b0909 in WTF::CrashOnOverflow::overflowed () at ../../Source/WTF/wtf/CheckedArithmetic.h:78
> #3  0x00007fe5378784b9 in WTF::Vector<unsigned short, 256ul, WTF::CrashOnOverflow, 16ul>::at (this=0x1f13fe0, i=0) at ../../Source/WTF/wtf/Vector.h:655
> #4  0x00007fe5378779fd in WTF::Vector<unsigned short, 256ul, WTF::CrashOnOverflow, 16ul>::operator[] (this=0x1f13fe0, i=0) at ../../Source/WTF/wtf/Vector.h:675
> #5  0x00007fe537877767 in (anonymous namespace)::HarfBuzzShaper::HarfBuzzRun::glyphToCharacterIndexes (this=0x1f139a0)
>     at ../../Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.h:83
> #6  0x00007fe53787659b in (anonymous namespace)::HarfBuzzShaper::setGlyphPositionsForHarfBuzzRun (this=0x7ffc87c51300, currentRun=0x1f139a0, harfBuzzBuffer=
>     0x1ec7850) at ../../Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:517
> #7  0x00007fe537876473 in (anonymous namespace)::HarfBuzzShaper::shapeHarfBuzzRuns (this=0x7ffc87c51300, shouldSetDirection=false)
>     at ../../Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:502
> #8  0x00007fe537875901 in (anonymous namespace)::HarfBuzzShaper::shape (this=0x7ffc87c51300, glyphBuffer=0x0)
>     at ../../Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:376
> #9  0x00007fe53785690a in (anonymous namespace)::FontCascade::floatWidthForComplexText (this=0x7fe516756678, run=...)
>     at ../../Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:73
> #10 0x00007fe53704bae4 in (anonymous namespace)::FontCascade::width (this=0x7fe516756678, run=..., fallbackFonts=0x7ffc87c51d48, glyphOverflow=0x7ffc87c51540)
>     at ../../Source/WebCore/platform/graphics/FontCascade.cpp:371
> #11 0x00007fe53744c883 in (anonymous namespace)::textWidth (text=..., from=1, len=3, font=..., xPos=0, isFixedPitch=false, collapseWhiteSpace=true, 
>     fallbackFonts=..., layout=0x0) at ../../Source/WebCore/rendering/line/BreakingContext.h:654
> #12 0x00007fe53744d148 in (anonymous namespace)::BreakingContext::computeAdditionalBetweenWordsWidth (this=0x7ffc87c51af0, renderText=..., textLayout=0x0, 
>     currentCharacter=10, wordTrailingSpace=..., fallbackFonts=..., wordMeasurements=..., font=..., isFixedPitch=false, lastSpace=1, lastSpaceWordSpacing=0, 
>     wordSpacingForWordMeasurement=0, offset=4) at ../../Source/WebCore/rendering/line/BreakingContext.h:749
> #13 0x00007fe53744ea62 in (anonymous namespace)::BreakingContext::handleText (this=0x7ffc87c51af0, wordMeasurements=..., hyphenated=@0x7ffc87c52dc8: false, 
>     consecutiveHyphenatedLines=@0x7ffc87c51cb8: 0) at ../../Source/WebCore/rendering/line/BreakingContext.h:913
> #14 0x00007fe537448bb9 in (anonymous namespace)::LineBreaker::nextLineBreak (this=0x7ffc87c52dc0, resolver=..., lineInfo=..., layoutState=..., renderTextInfo=..., 
>     lastFloatFromPreviousLine=0x0, consecutiveHyphenatedLines=0, wordMeasurements=...) at ../../Source/WebCore/rendering/line/LineBreaker.cpp:110
> #15 0x00007fe53723120a in (anonymous namespace)::RenderBlockFlow::layoutRunsAndFloatsInRange (this=0x7fe51671b990, layoutState=..., resolver=..., 
>     cleanLineStart=..., cleanLineBidiStatus=..., consecutiveHyphenatedLines=0) at ../../Source/WebCore/rendering/RenderBlockLineLayout.cpp:1368
> #16 0x00007fe537230e09 in (anonymous namespace)::RenderBlockFlow::layoutRunsAndFloats (this=0x7fe51671b990, layoutState=..., hasInlineChild=true)
>     at ../../Source/WebCore/rendering/RenderBlockLineLayout.cpp:1321
> #17 0x00007fe537233581 in (anonymous namespace)::RenderBlockFlow::layoutLineBoxes (this=0x7fe51671b990, relayoutChildren=false, repaintLogicalTop=..., 
>     repaintLogicalBottom=...) at ../../Source/WebCore/rendering/RenderBlockLineLayout.cpp:1747
> #18 0x00007fe537210fbd in (anonymous namespace)::RenderBlockFlow::layoutInlineChildren (this=0x7fe51671b990, relayoutChildren=false, repaintLogicalTop=..., 
>     repaintLogicalBottom=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:651
> #19 0x00007fe537210344 in (anonymous namespace)::RenderBlockFlow::layoutBlock (this=0x7fe51671b990, relayoutChildren=false, pageLogicalHeight=...)
>     at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:485
> #20 0x00007fe5371dde8a in (anonymous namespace)::RenderBlock::layout (this=0x7fe51671b990) at ../../Source/WebCore/rendering/RenderBlock.cpp:1072
> #21 0x00007fe53721137e in (anonymous namespace)::RenderBlockFlow::layoutBlockChild (this=0x7fe51671b880, child=..., marginInfo=..., 
>     previousFloatLogicalBottom=..., maxFloatLogicalBottom=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:709
> #22 0x00007fe537210ed7 in (anonymous namespace)::RenderBlockFlow::layoutBlockChildren (this=0x7fe51671b880, relayoutChildren=false, maxFloatLogicalBottom=...)
>     at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:632
> #23 0x00007fe537210368 in (anonymous namespace)::RenderBlockFlow::layoutBlock (this=0x7fe51671b880, relayoutChildren=false, pageLogicalHeight=...)
>     at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:487
> #24 0x00007fe5371dde8a in (anonymous namespace)::RenderBlock::layout (this=0x7fe51671b880) at ../../Source/WebCore/rendering/RenderBlock.cpp:1072
> #25 0x00007fe53721137e in (anonymous namespace)::RenderBlockFlow::layoutBlockChild (this=0x7fe4d4bf2b00, child=..., marginInfo=..., 
>     previousFloatLogicalBottom=..., maxFloatLogicalBottom=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:709
> #26 0x00007fe537210ed7 in (anonymous namespace)::RenderBlockFlow::layoutBlockChildren (this=0x7fe4d4bf2b00, relayoutChildren=false, maxFloatLogicalBottom=...)
>     at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:632
> #27 0x00007fe537210368 in (anonymous namespace)::RenderBlockFlow::layoutBlock (this=0x7fe4d4bf2b00, relayoutChildren=false, pageLogicalHeight=...)
>     at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:487
> #28 0x00007fe5371dde8a in (anonymous namespace)::RenderBlock::layout (this=0x7fe4d4bf2b00) at ../../Source/WebCore/rendering/RenderBlock.cpp:1072
> #29 0x00007fe537408af5 in (anonymous namespace)::RenderView::layoutContent (this=0x7fe4d4bf2b00, state=...) at ../../Source/WebCore/rendering/RenderView.cpp:244
> #30 0x00007fe537409219 in (anonymous namespace)::RenderView::layout (this=0x7fe4d4bf2b00) at ../../Source/WebCore/rendering/RenderView.cpp:370
> #31 0x00007fe536ea224b in (anonymous namespace)::FrameView::layout (this=0x7fe51678ba00, allowSubtree=true) at ../../Source/WebCore/page/FrameView.cpp:1461
> #32 0x00007fe536eac117 in (anonymous namespace)::FrameView::updateLayoutAndStyleIfNeededRecursive (this=0x7fe51678ba00)
>     at ../../Source/WebCore/page/FrameView.cpp:4288
> #33 0x00007fe535fc91fc in (anonymous namespace)::CompositingCoordinator::syncDisplayState (this=0x7fe5167d0338)
>     at ../../Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:155
> #34 0x00007fe535fd571f in (anonymous namespace)::CoordinatedLayerTreeHost::layerFlushTimerFired (this=0x7fe5167d0300)
>     at ../../Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:165
> #35 0x00007fe535fd5cb4 in WTF::RunLoop::Timer<WebKit::CoordinatedLayerTreeHost>::fired (this=0x7fe5167d0528) at ../../Source/WTF/wtf/RunLoop.h:145
> #36 0x00007fe52f7cb5f3 in WTF::RunLoop::TimerBase::<lambda(gpointer)>::operator()(gpointer) const (__closure=0x0, userData=0x7fe5167d0528)
>     at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:162
> #37 0x00007fe52f7cb62f in WTF::RunLoop::TimerBase::<lambda(gpointer)>::_FUN(gpointer) () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:166
> #38 0x00007fe52f7cac56 in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::operator()(GSource *, GSourceFunc, gpointer) const (__closure=0x0, source=0x1c59d90, 
>     callback=0x7fe52f7cb612 <WTF::RunLoop::TimerBase::<lambda(gpointer)>::_FUN(gpointer)>, userData=0x7fe5167d0528) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:44
> #39 0x00007fe52f7cac85 in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::_FUN(GSource *, GSourceFunc, gpointer) () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:45
> #40 0x00007fe52702754a in g_main_dispatch () at /home/fujii/work/webkit/w2/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3122
> #41 g_main_context_dispatch () at /home/fujii/work/webkit/w2/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3737
> #42 0x00007fe5270278c8 in g_main_context_iterate () at /home/fujii/work/webkit/w2/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3808
> #43 0x00007fe527027be2 in g_main_loop_run () at /home/fujii/work/webkit/w2/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:4002
> #44 0x00007fe52f7cb236 in WTF::RunLoop::run () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:94
> #45 0x00007fe535f98a72 in (anonymous namespace)::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> (argc=2, argv=0x7ffc87c54d78)
>     at ../../Source/WebKit2/Shared/unix/ChildProcessMain.h:61
> #46 0x00007fe535f98920 in (anonymous namespace)::WebProcessMainUnix (argc=2, argv=0x7ffc87c54d78) at ../../Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:69
> #47 0x0000000000400c7a in main (argc=2, argv=0x7ffc87c54d78) at ../../Source/WebKit2/WebProcess/EntryPoint/unix/WebProcessMain.cpp:44
> (gdb)

[Fujii Hironori]

Created attachment 294114 [details]
Test.woff

[Fujii Hironori]

Created attachment 294115 [details]
test.sfd

[Fujii Hironori]

Created attachment 294131 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 294131 [details]
Patch

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

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:589
>      unsigned numRuns = m_harfBuzzRuns.size();

Probably a better fix would be not adding a HarfBuzzRun to m_harfBuzzRuns when the run has no glyph?

[Fujii Hironori]

Thank you for review.

(In reply to comment #4)
> Probably a better fix would be not adding a HarfBuzzRun to m_harfBuzzRuns
> when the run has no glyph?

I'm not so sure about HarfBuzzShaper, but I don't think so.
HarfBuzz removes ignorables only if a font does not has a glyph of SPACE (U+0020).

  https://github.com/behdad/harfbuzz/blob/1.3.3/src/hb-ot-shape.cc#L459

If the font has the glyph of SPACE, HarfBuzz replaces ignorables
with a zero-advance space glyph.

If I remove empty HarfBuzzRun, it would affect
firstOffsetOfNextRun of HarfBuzzShaper::fillGlyphBufferFromHarfBuzzRun.
I think it should be processed as if a zero-advance SPACE is there.

[Kenichi Ishibashi]

If HarfBuzz replaces (In reply to comment #5)
> Thank you for review.
> 
> (In reply to comment #4)
> > Probably a better fix would be not adding a HarfBuzzRun to m_harfBuzzRuns
> > when the run has no glyph?
> 
> I'm not so sure about HarfBuzzShaper, but I don't think so.
> HarfBuzz removes ignorables only if a font does not has a glyph of SPACE
> (U+0020).
> 
>   https://github.com/behdad/harfbuzz/blob/1.3.3/src/hb-ot-shape.cc#L459
> 
> If the font has the glyph of SPACE, HarfBuzz replaces ignorables
> with a zero-advance space glyph.
> 
> If I remove empty HarfBuzzRun, it would affect
> firstOffsetOfNextRun of HarfBuzzShaper::fillGlyphBufferFromHarfBuzzRun.
> I think it should be processed as if a zero-advance SPACE is there.

Could you help me understand the problem then? If HarfBuzz replaces an ignorable with a zero-advance SPACE, a HarfBuzzRun should have a glpyh and the crash won't occur. Am I missing something?

[Fujii Hironori]

(In reply to comment #6)
> Could you help me understand the problem then? If HarfBuzz replaces an
> ignorable with a zero-advance SPACE, a HarfBuzzRun should have a glpyh and
> the crash won't occur. Am I missing something?

This crash occurs only if a font does not has a glyph of SPACE.

[Kenichi Ishibashi]

(In reply to comment #7)
> (In reply to comment #6)
> > Could you help me understand the problem then? If HarfBuzz replaces an
> > ignorable with a zero-advance SPACE, a HarfBuzzRun should have a glpyh and
> > the crash won't occur. Am I missing something?
> 
> This crash occurs only if a font does not has a glyph of SPACE.

If a font doesn't have a glyph of SPACE and then HarfBuzz doesn't replace it, doesn't a run become an empty? In summary:

- a font has SPACE
-- a run always has at least one glyph. No length check is needed
- a font doesn't have SPACE
-- a run becomes an empty when there are only ignorables. The run can be removed

Is my understanding wrong?

[Fujii Hironori]

There is no doubt that it will solve this crash problem to remove empty
HarfBuzzRun from m_harfBuzzRuns.

But, I don't agree it for comment 5.
How do you think about affecting firstOffsetOfNextRun?


I will show you an imaginal example, where there are three HarfBuzzRun:

  [g1 g2][ ignorable ][g3 g4]

If the font has SPACE glyph, HarfBuzz replaces ignorables with a zero-advance SPACE glyph:

  [g1 g2][ zero-advance-space ][g3 g4]

In this case, next offset of g2 is offset of zero-advance-space.

If the font does not have SPACE glyph, HarfBuzz removes ignorables:

  [g1 g2][ ][g3 g4]

The second HarfBuzzRun has no glyphs. 

In your suggestion, this empty run will be removed:

  [g1 g2][g3 g4]

Then, next offset of g2 is offset of g3.

In my patch, I keep the empty run:

  [g1 g2][ ][g3 g4]

Then, next offset of g2 is zero (initial value of FloatPoint).

[Kenichi Ishibashi]

(In reply to comment #9)
> There is no doubt that it will solve this crash problem to remove empty
> HarfBuzzRun from m_harfBuzzRuns.
> 
> But, I don't agree it for comment 5.
> How do you think about affecting firstOffsetOfNextRun?
> 
> 
> I will show you an imaginal example, where there are three HarfBuzzRun:
> 
>   [g1 g2][ ignorable ][g3 g4]
> 
> If the font has SPACE glyph, HarfBuzz replaces ignorables with a
> zero-advance SPACE glyph:
> 
>   [g1 g2][ zero-advance-space ][g3 g4]
> 
> In this case, next offset of g2 is offset of zero-advance-space.
> 
> If the font does not have SPACE glyph, HarfBuzz removes ignorables:
> 
>   [g1 g2][ ][g3 g4]
> 
> The second HarfBuzzRun has no glyphs. 
> 
> In your suggestion, this empty run will be removed:
> 
>   [g1 g2][g3 g4]
> 
> Then, next offset of g2 is offset of g3.
> 
> In my patch, I keep the empty run:
> 
>   [g1 g2][ ][g3 g4]
> 
> Then, next offset of g2 is zero (initial value of FloatPoint).

I don't see much advantages of above as this seems a corner case and adding branches in hot path will regress performance. IIUC the path you modified will run most cases.

That said, if reviewers are happy with this change I won't oppose this fix :)

[Fujii Hironori]

Comment on attachment 294131 [details]
Patch

(In reply to comment #10)
> I don't see much advantages of above as this seems a corner case and adding
> branches in hot path will regress performance. IIUC the path you modified
> will run most cases.

It makes sense. I didn't have the performance perspective.
I will revise the patch.

[Fujii Hironori]

Created attachment 294150 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #12)
> Created attachment 294150 [details]
> Patch

Looks good to me.

[Fujii Hironori]

(In reply to comment #13)
> Looks good to me.

Thanks for your LGTM.

I'm wondering who is the best person to review HarfBuzzShaper patch nowadays?
Added GTK and EFL experts to CC.

[Michael Catanzaro]

The best reviewer is Myles, though he might not know about harfbuzz so I will rubber-stamp it if he doesn't review it. Thanks for the patch and remember we're not committing until Apple's continuous integration is back online.

[Myles C. Maxfield]

Comment on attachment 294150 [details]
Patch

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

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:87
> +        // HarfBuzzShaper::fillGlyphBuffer gets offsets()[0]

Why didn't you instead change fillGlyphBuffer()?

[Fujii Hironori]

(In reply to comment #16)
> Why didn't you instead change fillGlyphBuffer()?

For the performance reason (Comment 11).
At first, I created a patch of the approach.

[Fujii Hironori]

Created attachment 294683 [details]
Patch

cq+ please.

[WebKit Commit Bot]

Comment on attachment 294683 [details]
Patch

Clearing flags on attachment: 294683

Committed r208675: <http://trac.webkit.org/changeset/208675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.