Bug 76574 – Assertion failure in BidiResolver::commitExplicitEmbedding() (!inIsolate() || m_currentExplicitEmbeddingSequence.isEmpty()) at wikipedia.org

• Component: Layout and Rendering
• Status: RESOLVED FIXED

Created attachment 122996 [details]
Reduction

Steps to reproduce:
1. Navigate to the URL
—or—
1. Open the attached recution

Result:
ASSERTION FAILED: !inIsolate() || m_currentExplicitEmbeddingSequence.isEmpty()

[Sergio Villar Senin]

I got the same assertion in some other sites. This is the backtrace I got:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff4280c46 in WebCore::BidiResolver<WebCore::InlineIterator, WebCore::BidiRun>::commitExplicitEmbedding (this=0x7fffffffb380) at ../../Source/WebCore/platform/text/BidiResolver.h:406
406	    ASSERT(!inIsolate() || m_currentExplicitEmbeddingSequence.isEmpty());
(gdb) bt
#0  0x00007ffff4280c46 in WebCore::BidiResolver<WebCore::InlineIterator, WebCore::BidiRun>::commitExplicitEmbedding (this=0x7fffffffb380) at ../../Source/WebCore/platform/text/BidiResolver.h:406
#1  0x00007ffff42716f4 in WebCore::bidiFirstSkippingEmptyInlines (root=0x2e10088, resolver=0x7fffffffb380) at ../../Source/WebCore/rendering/InlineIterator.h:281
#2  0x00007ffff42783cf in WebCore::RenderBlock::determineStartPosition (this=0x2e10088, layoutState=..., resolver=...) at ../../Source/WebCore/rendering/RenderBlockLineLayout.cpp:1676
#3  0x00007ffff4275304 in WebCore::RenderBlock::layoutRunsAndFloats (this=0x2e10088, layoutState=..., hasInlineChild=true) at ../../Source/WebCore/rendering/RenderBlockLineLayout.cpp:1150
#4  0x00007ffff4277403 in WebCore::RenderBlock::layoutInlineChildren (this=0x2e10088, relayoutChildren=true, repaintLogicalTop=@0x7fffffffb77c, repaintLogicalBottom=@0x7fffffffb778) at ../../Source/WebCore/rendering/RenderBlockLineLayout.cpp:1502
#5  0x00007ffff4237632 in WebCore::RenderBlock::layoutBlock (this=0x2e10088, relayoutChildren=true, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at ../../Source/WebCore/rendering/RenderBlock.cpp:1314
#6  0x00007ffff4236e58 in WebCore::RenderBlock::layout (this=0x2e10088) at ../../Source/WebCore/rendering/RenderBlock.cpp:1189
#7  0x00007ffff423a952 in WebCore::RenderBlock::layoutBlockChild (this=0x4470478, child=0x2e10088, marginInfo=..., previousFloatLogicalBottom=@0x7fffffffba5c, maxFloatLogicalBottom=@0x7fffffffbba4) at ../../Source/WebCore/rendering/RenderBlock.cpp:2102
#8  0x00007ffff423a576 in WebCore::RenderBlock::layoutBlockChildren (this=0x4470478, relayoutChildren=true, maxFloatLogicalBottom=@0x7fffffffbba4) at ../../Source/WebCore/rendering/RenderBlock.cpp:2038
#9  0x00007ffff4237653 in WebCore::RenderBlock::layoutBlock (this=0x4470478, relayoutChildren=true, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at ../../Source/WebCore/rendering/RenderBlock.cpp:1316
#10 0x00007ffff4236e58 in WebCore::RenderBlock::layout (this=0x4470478) at ../../Source/WebCore/rendering/RenderBlock.cpp:1189
#11 0x00007ffff423a952 in WebCore::RenderBlock::layoutBlockChild (this=0x56ad6c8, child=0x4470478, marginInfo=..., previousFloatLogicalBottom=@0x7fffffffbe8c, maxFloatLogicalBottom=@0x7fffffffbfd4) at ../../Source/WebCore/rendering/RenderBlock.cpp:2102
#12 0x00007ffff423a576 in WebCore::RenderBlock::layoutBlockChildren (this=0x56ad6c8, relayoutChildren=false, maxFloatLogicalBottom=@0x7fffffffbfd4) at ../../Source/WebCore/rendering/RenderBlock.cpp:2038
#13 0x00007ffff4237653 in WebCore::RenderBlock::layoutBlock (this=0x56ad6c8, relayoutChildren=false, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at ../../Source/WebCore/rendering/RenderBlock.cpp:1316
#14 0x00007ffff4236e58 in WebCore::RenderBlock::layout (this=0x56ad6c8) at ../../Source/WebCore/rendering/RenderBlock.cpp:1189
#15 0x00007ffff423a952 in WebCore::RenderBlock::layoutBlockChild (this=0x3264d38, child=0x56ad6c8, marginInfo=..., previousFloatLogicalBottom=@0x7fffffffc2bc, maxFloatLogicalBottom=@0x7fffffffc404) at ../../Source/WebCore/rendering/RenderBlock.cpp:2102
#16 0x00007ffff423a576 in WebCore::RenderBlock::layoutBlockChildren (this=0x3264d38, relayoutChildren=false, maxFloatLogicalBottom=@0x7fffffffc404) at ../../Source/WebCore/rendering/RenderBlock.cpp:2038
#17 0x00007ffff4237653 in WebCore::RenderBlock::layoutBlock (this=0x3264d38, relayoutChildren=false, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at ../../Source/WebCore/rendering/RenderBlock.cpp:1316
#18 0x00007ffff4236e58 in WebCore::RenderBlock::layout (this=0x3264d38) at ../../Source/WebCore/rendering/RenderBlock.cpp:1189
#19 0x00007ffff423a952 in WebCore::RenderBlock::layoutBlockChild (this=0x4cffb48, child=0x3264d38, marginInfo=..., previousFloatLogicalBottom=@0x7fffffffc6ec, maxFloatLogicalBottom=@0x7fffffffc834) at ../../Source/WebCore/rendering/RenderBlock.cpp:2102
#20 0x00007ffff423a576 in WebCore::RenderBlock::layoutBlockChildren (this=0x4cffb48, relayoutChildren=false, maxFloatLogicalBottom=@0x7fffffffc834) at ../../Source/WebCore/rendering/RenderBlock.cpp:2038
#21 0x00007ffff4237653 in WebCore::RenderBlock::layoutBlock (this=0x4cffb48, relayoutChildren=false, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at ../../Source/WebCore/rendering/RenderBlock.cpp:1316
#22 0x00007ffff4236e58 in WebCore::RenderBlock::layout (this=0x4cffb48) at ../../Source/WebCore/rendering/RenderBlock.cpp:1189
#23 0x00007ffff423a952 in WebCore::RenderBlock::layoutBlockChild (this=0x4c116a8, child=0x4cffb48, marginInfo=..., previousFloatLogicalBottom=@0x7fffffffcb1c, maxFloatLogicalBottom=@0x7fffffffcc64) at ../../Source/WebCore/rendering/RenderBlock.cpp:2102
#24 0x00007ffff423a576 in WebCore::RenderBlock::layoutBlockChildren (this=0x4c116a8, relayoutChildren=false, maxFloatLogicalBottom=@0x7fffffffcc64) at ../../Source/WebCore/rendering/RenderBlock.cpp:2038
#25 0x00007ffff4237653 in WebCore::RenderBlock::layoutBlock (this=0x4c116a8, relayoutChildren=false, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at ../../Source/WebCore/rendering/RenderBlock.cpp:1316
#26 0x00007ffff4236e58 in WebCore::RenderBlock::layout (this=0x4c116a8) at ../../Source/WebCore/rendering/RenderBlock.cpp:1189
#27 0x00007ffff423a952 in WebCore::RenderBlock::layoutBlockChild (this=0x29addc8, child=0x4c116a8, marginInfo=..., previousFloatLogicalBottom=@0x7fffffffcf4c, maxFloatLogicalBottom=@0x7fffffffd094) at ../../Source/WebCore/rendering/RenderBlock.cpp:2102
#28 0x00007ffff423a576 in WebCore::RenderBlock::layoutBlockChildren (this=0x29addc8, relayoutChildren=false, maxFloatLogicalBottom=@0x7fffffffd094) at ../../Source/WebCore/rendering/RenderBlock.cpp:2038
#29 0x00007ffff4237653 in WebCore::RenderBlock::layoutBlock (this=0x29addc8, relayoutChildren=false, pageLogicalHeight=0, layoutPass=WebCore::RenderBlock::NormalLayoutPass) at ../../Source/WebCore/rendering/RenderBlock.cpp:1316
#30 0x00007ffff4236e58 in WebCore::RenderBlock::layout (this=0x29addc8) at ../../Source/WebCore/rendering/RenderBlock.cpp:1189
#31 0x00007ffff4372543 in WebCore::RenderView::layout (this=0x29addc8) at ../../Source/WebCore/rendering/RenderView.cpp:137
#32 0x00007ffff40afe53 in WebCore::FrameView::layout (this=0x33ac160, allowSubtree=true) at ../../Source/WebCore/page/FrameView.cpp:1124
#33 0x00007ffff40b2813 in WebCore::FrameView::layoutTimerFired (this=0x33ac160) at ../../Source/WebCore/page/FrameView.cpp:1970
#34 0x00007ffff40bd686 in WebCore::Timer<WebCore::FrameView>::fired (this=0x33ac2c0) at ../../Source/WebCore/platform/Timer.h:100
#35 0x00007ffff41e2c41 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0xf731e0) at ../../Source/WebCore/platform/ThreadTimers.cpp:115

<rdar://problem/10939838>

[Eric Seidel (no email)]

Sorry, it didn't register for me how prevalent this was.  I'll look later this week.

[Eric Seidel (no email)]

This assert was from http://trac.webkit.org/changeset/94775, which is when unicode-bidi: isolate landed.

[Eric Seidel (no email)]

Created attachment 142375 [details]
Patch

[Eric Seidel (no email)]

I'm sorry for not having fixed this sooner!

[Simon Fraser (smfr)]

Comment on attachment 142375 [details]
Patch

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

> Source/WebCore/platform/text/BidiResolver.h:407
> +    // We should never acrew embedding levels while skipping over isolated content.

accrue?

[Eric Seidel (no email)]

(In reply to comment #7)
> (From update of attachment 142375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142375&action=review
> 
> > Source/WebCore/platform/text/BidiResolver.h:407
> > +    // We should never acrew embedding levels while skipping over isolated content.
> 
> accrue?

This english language is so complicated.

[Eric Seidel (no email)]

Created attachment 142376 [details]
Now with 100% moar spellin

[Levi Weintraub]

Comment on attachment 142376 [details]
Now with 100% moar spellin

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

> Source/WebCore/ChangeLog:20
> +        I believe the ASSERT here is valid, we shouldn't ever be actually changing embedding
> +        levels while we're skipping over isolated content (inIsolate()).
> +        We were hitting this, because when scaning for the first bidi-interesting object on the
> +        line, we had encountered an explicit LTR embed, but not yet committed it before entering
> +        into the isolated content.  Finding the first interesting bit inside the isolated content
> +        we then tried to commit the embedding levels and asserted.  Alternatively we could have fixed
> +        this by having bidiFirst return a pointer to the isolated span, before walking into it,
> +        but this seemed to make more sense.  This should cause the BidiContext stack to have
> +        the right levels of embedding before we created the placeholder BidiRun for the isolated
> +        content.
> +        I don't believe this changes behavior, beyond avoiding the ASSERT in Debug mode,
> +        even though the previous codepaths were out of order a bit, I believe they still got
> +        the same result as this now should more explicitly.

This is a lovely explanation, but it doesn't actually explain the solution, which I believe from the code is to commit embedding upon entering an isolate? Ahh, I read the description in the test case, and that seems to be the case. Couldn't hurt to have a one-line description here to that affect.

[Ryosuke Niwa]

Comment on attachment 142376 [details]
Now with 100% moar spellin

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

> Source/WebCore/ChangeLog:13
> +        into the isolated content.  Finding the first interesting bit inside the isolated content
> +        we then tried to commit the embedding levels and asserted.  Alternatively we could have fixed

I don't quite follow. Maybe you meant something along the line of "we asserted when trying to commit the embedding levels as we find the first interesting bit inside the isolated content"?

> LayoutTests/fast/text/bidi-isolate-embedding-crash.html:1
> +<script>

We can't have DOCTYPE?

> LayoutTests/fast/text/bidi-isolate-embedding-crash.html:11
> +<span style="unicode-bidi: embed;"><bdi dir="rtl">PASS if did not crash.</bdi></span>

Would it make sense to emulate all combinations of things? e.g. dir=ltr, RTL text, etc...? Just to make sure we don't regress those cases in the future?

[Ryosuke Niwa]

I think I understand the fix given the IRC discussion but I'll sleep on it and give mitz a chance to take a look at it :)

[Eric Seidel (no email)]

Ping?

[Levi Weintraub]

Comment on attachment 142376 [details]
Now with 100% moar spellin

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

>> Source/WebCore/ChangeLog:20
>> +        the same result as this now should more explicitly.
> 
> This is a lovely explanation, but it doesn't actually explain the solution, which I believe from the code is to commit embedding upon entering an isolate? Ahh, I read the description in the test case, and that seems to be the case. Couldn't hurt to have a one-line description here to that affect.

I still want a description of what exactly the fix is here (you mention what else could have been done, but not what *was* done :)

> LayoutTests/fast/text/bidi-isolate-embedding-crash.html:10
> +inside the isolate and is not affected by the outer embeddings. -->

How about a link to the bug?

[Eric Seidel (no email)]

Created attachment 142790 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142790 [details]
Patch for landing

Rejecting attachment 142790 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12726563

[Eric Seidel (no email)]

Comment on attachment 142790 [details]
Patch for landing

Odd.  Levi, would you re-do the honors?

[Levi Weintraub]

Comment on attachment 142790 [details]
Patch for landing

Most certainly. Epic ChangLog FTW :)

[WebKit Review Bot]

Comment on attachment 142790 [details]
Patch for landing

Rejecting attachment 142790 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Ryosuke Niwa]

Levi, you have to update http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py first.

[Levi Weintraub]

Comment on attachment 142790 [details]
Patch for landing

Let's see if my change has propagated...

[Eric Seidel (no email)]

It can take up to 2 hours for the feeder queue to restart itself. :)  I can restart it manually if necessary.

[Ryosuke Niwa]

(In reply to comment #22)
> It can take up to 2 hours for the feeder queue to restart itself. :)  I can restart it manually if necessary.

Assuming the uniform distribution of the queue restart time, we have 75% chance of succeeding.

[Levi Weintraub]

(In reply to comment #23)
> (In reply to comment #22)
> > It can take up to 2 hours for the feeder queue to restart itself. :)  I can restart it manually if necessary.
> 
> Assuming the uniform distribution of the queue restart time, we have 75% chance of succeeding.

:D

[WebKit Review Bot]

Comment on attachment 142790 [details]
Patch for landing

Clearing flags on attachment: 142790

Committed r117658: <http://trac.webkit.org/changeset/117658>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.