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)
Created attachment 294114 [details] Test.woff
Created attachment 294115 [details] test.sfd
Created attachment 294131 [details] Patch
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?
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.
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?
(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.
(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?
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).
(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 :)
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.
Created attachment 294150 [details] Patch
(In reply to comment #12) > Created attachment 294150 [details] > Patch Looks good to me.
(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.
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.
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()?
(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.
Created attachment 294683 [details] Patch cq+ please.
Comment on attachment 294683 [details] Patch Clearing flags on attachment: 294683 Committed r208675: <http://trac.webkit.org/changeset/208675>
All reviewed patches have been landed. Closing bug.