Bug 66510 – [chromium] editing/selection/regional-indicators.html timing out on Linux

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Tony Chang]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=editing%2Fselection%2Fregional-indicators.html

It looks like we're hitting an assert in skia, but the bot says it's a timeout.  I'm going to mark it as timeout to make the bot happy.

webkit/support/webkit_support.cc:75: Failure
Failed
[15812:15812:10549516918781:FATAL:SkUtils.cpp(324)] third_party/skia/src/core/SkUtils.cpp:324: failed assertion "!(((c) & 0xFC00) == 0xDC00)"

Backtrace:
	base::debug::StackTrace::StackTrace() [0x8c92ca]
	logging::LogMessage::~LogMessage() [0x85c326]
	SkDebugf_FileLine() [0x9629a3]
	SkUTF16_NextUnichar() [0x956d71]
	SkPaint::textToGlyphs() [0x921156]
	WebCore::GlyphPage::fill() [0x1077564]
	WebCore::fill() [0x100480d]
	WebCore::GlyphPageTreeNode::initializePage() [0x1004f91]
	WebCore::GlyphPageTreeNode::getChild() [0x1005578]
	WebCore::GlyphPageTreeNode::getRootChild() [0xffba37]
	WebCore::Font::glyphDataAndPageForCharacter() [0xff96d2]
	WebCore::Font::glyphDataForCharacter() [0xff9508]
	WebCore::ComplexTextController::nextScriptRun() [0x10d8b06]
	WebCore::ComplexTextController::widthOfFullRun() [0x10d8c17]
	WebCore::Font::floatWidthForComplexText() [0x102a326]
	WebCore::Font::width() [0xfea074]
	WebCore::textWidth() [0x16fd93d]
	WebCore::RenderBlock::LineBreaker::nextLineBreak() [0x16ff93b]
	WebCore::RenderBlock::layoutRunsAndFloatsInRange() [0x16f9dbd]
	WebCore::RenderBlock::layoutRunsAndFloats() [0x16f9aa2]
	WebCore::RenderBlock::layoutInlineChildren() [0x16fb7d9]
	WebCore::RenderBlock::layoutBlock() [0x16be479]
	WebCore::RenderBlock::layout() [0x16bdd52]
	WebCore::RenderBlock::layoutBlockChild() [0x16c18e4]
	WebCore::RenderBlock::layoutBlockChildren() [0x16c150a]
	WebCore::RenderBlock::layoutBlock() [0x16be497]
	WebCore::RenderBlock::layout() [0x16bdd52]
	WebCore::RenderBlock::layoutBlockChild() [0x16c18e4]
	WebCore::RenderBlock::layoutBlockChildren() [0x16c150a]
	WebCore::RenderBlock::layoutBlock() [0x16be497]
	WebCore::RenderBlock::layout() [0x16bdd52]
	WebCore::RenderBlock::layoutBlockChild() [0x16c18e4]
	WebCore::RenderBlock::layoutBlockChildren() [0x16c150a]
	WebCore::RenderBlock::layoutBlock() [0x16be497]
	WebCore::RenderBlock::layout() [0x16bdd52]
	WebCore::RenderView::layout() [0x17e4ced]
	WebCore::FrameView::layout() [0x1451ab9]
	WebCore::Document::updateLayout() [0xdd8e6d]
	WebCore::Document::updateLayoutIgnorePendingStylesheets() [0xdd8f4e]
	WebCore::VisiblePosition::canonicalPosition() [0x12be15f]
	WebCore::VisiblePosition::init() [0x12bc4d7]
	WebCore::VisiblePosition::VisiblePosition() [0x12bc49d]
	WebCore::VisibleSelection::setBaseAndExtentToDeepEquivalents() [0x12c0660]
	WebCore::VisibleSelection::validate() [0x12c197d]
	WebCore::VisibleSelection::VisibleSelection() [0x12bfa5f]
	WebCore::DOMSelection::addRange() [0x141561a]
	WebCore::DOMSelectionInternal::addRangeCallback() [0x7a1bd2]
	v8::internal::HandleApiCallHelper<>() [0xa0901a]
	v8::internal::Builtin_Impl_HandleApiCall() [0xa03c57]
	v8::internal::Builtin_HandleApiCall() [0xa03c28]
	0x155c22d3e14e

[Kenichi Ishibashi]

Created attachment 104466 [details]
Patch V0

[Kenichi Ishibashi]

(In reply to comment #1)
> Created an attachment (id=104466) [details]
> Patch V0

With this patch, editing/selection/regional-indicators.html still caused assertion failure on chromium part (on harfbuzz):

DumpRenderTree: third_party/harfbuzz/src/harfbuzz-shaper.cpp:433: void HB_HeuristicSetGlyphAttributes(HB_ShaperItem*): Assertion `length <= item->num_glyphs' failed.

http://codereview.chromium.org/165165 introduced this assertion.  IMHO, this assertion might be wrong.  I'll create a bug entry for this on http://crbug.com

[Kenichi Ishibashi]

Created attachment 106080 [details]
Patch V1

[Kenichi Ishibashi]

(In reply to comment #3)
> Created an attachment (id=106080) [details]
> Patch V1

Fix for Chromium part was landed.  This patch will make some regional indicators related test pass (or not being timeout).

[Kent Tamura]

Comment on attachment 106080 [details]
Patch V1

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

> LayoutTests/ChangeLog:11
> +        [chromium] editing/selection/regional-indicators.html timing out on Linux
> +        https://bugs.webkit.org/show_bug.cgi?id=66510
> +
> +        ComplexTextControllerLinux is now surrogate pairs aware, therefore;
> +        - editing/selection/regional-indicators.html should pass.
> +        - editing/deleting/regional-indicators.html should not timeout.
> +        - fast/text/regional-indicator-symbols.html should pass.
> +
> +        Reviewed by NOBODY (OOPS!).

The typical ChangeLog entry format is:

<summary>
<Bug URL>

Reviewed by ...

<Detail ....>

> Source/WebCore/ChangeLog:10
> +        [chromium] editing/selection/regional-indicators.html timing out on Linux
> +        https://bugs.webkit.org/show_bug.cgi?id=66510
> +
> +        Uses SurrogatePairAwareTextIerator in ComplexTextControllerLinux to handle surrogate pairs correctly.
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests. editing/selection/regional-indicators.html should pass with this patch.

ditto.

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:239
> +static UChar32 surrogatePairAwareCharacter(const UChar* characters, unsigned lastCharacterIndex)

Probably the name should be
  surrogatePairAwareFirstCharacter()
?

'lastCharacterIndex' might be confusing.  'length' or 'charactersLength' is enough.

[Kenichi Ishibashi]

Created attachment 106110 [details]
Patch V2

[Kenichi Ishibashi]

Comment on attachment 106080 [details]
Patch V1

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

Hi Kent-san,

Thank you for review! Addressed your comments.

>> LayoutTests/ChangeLog:11
>> +        Reviewed by NOBODY (OOPS!).
> 
> The typical ChangeLog entry format is:
> 
> <summary>
> <Bug URL>
> 
> Reviewed by ...
> 
> <Detail ....>

Done.

>> Source/WebCore/ChangeLog:10
>> +        No new tests. editing/selection/regional-indicators.html should pass with this patch.
> 
> ditto.

Done.

>> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:239
>> +static UChar32 surrogatePairAwareCharacter(const UChar* characters, unsigned lastCharacterIndex)
> 
> Probably the name should be
>   surrogatePairAwareFirstCharacter()
> ?
> 
> 'lastCharacterIndex' might be confusing.  'length' or 'charactersLength' is enough.

Done.

[Kent Tamura]

Comment on attachment 106110 [details]
Patch V2

Looks ok.

[Kenichi Ishibashi]

Comment on attachment 106110 [details]
Patch V2

Thanks!

[WebKit Review Bot]

Comment on attachment 106110 [details]
Patch V2

Clearing flags on attachment: 106110

Committed r94403: <http://trac.webkit.org/changeset/94403>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.