Bug 76574 - Assertion failure in BidiResolver::commitExplicitEmbedding() (!inIsolate() || m_currentExplicitEmbeddingSequence.isEmpty()) at wikipedia.org
Summary: Assertion failure in BidiResolver::commitExplicitEmbedding() (!inIsolate() ||...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL: http://wikipedia.org/
Keywords: HasReduction, InRadar
Depends on: 50912 86140
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-18 14:24 PST by mitz
Modified: 2012-05-18 16:46 PDT (History)
9 users (show)

See Also:


Attachments
Reduction (65 bytes, text/html)
2012-01-18 14:24 PST, mitz
no flags Details
Patch (6.43 KB, patch)
2012-05-16 17:01 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Now with 100% moar spellin (6.43 KB, patch)
2012-05-16 17:05 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (6.94 KB, patch)
2012-05-18 14:49 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2012-01-18 14:24:49 PST
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()
Comment 1 Sergio Villar Senin 2012-01-21 01:18:35 PST
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
Comment 2 mitz 2012-02-27 14:35:33 PST
<rdar://problem/10939838>
Comment 3 Eric Seidel (no email) 2012-05-08 23:23:37 PDT
Sorry, it didn't register for me how prevalent this was.  I'll look later this week.
Comment 4 Eric Seidel (no email) 2012-05-16 15:05:27 PDT
This assert was from http://trac.webkit.org/changeset/94775, which is when unicode-bidi: isolate landed.
Comment 5 Eric Seidel (no email) 2012-05-16 17:01:56 PDT
Created attachment 142375 [details]
Patch
Comment 6 Eric Seidel (no email) 2012-05-16 17:02:20 PDT
I'm sorry for not having fixed this sooner!
Comment 7 Simon Fraser (smfr) 2012-05-16 17:02:58 PDT
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?
Comment 8 Eric Seidel (no email) 2012-05-16 17:03:37 PDT
(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.
Comment 9 Eric Seidel (no email) 2012-05-16 17:05:05 PDT
Created attachment 142376 [details]
Now with 100% moar spellin
Comment 10 Levi Weintraub 2012-05-16 17:13:20 PDT
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.
Comment 11 Ryosuke Niwa 2012-05-16 17:15:24 PDT
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?
Comment 12 Ryosuke Niwa 2012-05-16 17:16:27 PDT
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 :)
Comment 13 Eric Seidel (no email) 2012-05-18 14:08:28 PDT
Ping?
Comment 14 Levi Weintraub 2012-05-18 14:12:59 PDT
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?
Comment 15 Eric Seidel (no email) 2012-05-18 14:49:18 PDT
Created attachment 142790 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-05-18 14:55:49 PDT
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
Comment 17 Eric Seidel (no email) 2012-05-18 14:56:51 PDT
Comment on attachment 142790 [details]
Patch for landing

Odd.  Levi, would you re-do the honors?
Comment 18 Levi Weintraub 2012-05-18 14:57:51 PDT
Comment on attachment 142790 [details]
Patch for landing

Most certainly. Epic ChangLog FTW :)
Comment 19 WebKit Review Bot 2012-05-18 14:59:02 PDT
Comment on attachment 142790 [details]
Patch for landing

Rejecting attachment 142790 [details] from review queue.

leviw@chromium.org 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.
Comment 20 Ryosuke Niwa 2012-05-18 14:59:27 PDT
Levi, you have to update http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py first.
Comment 21 Levi Weintraub 2012-05-18 15:47:11 PDT
Comment on attachment 142790 [details]
Patch for landing

Let's see if my change has propagated...
Comment 22 Eric Seidel (no email) 2012-05-18 15:51:42 PDT
It can take up to 2 hours for the feeder queue to restart itself. :)  I can restart it manually if necessary.
Comment 23 Ryosuke Niwa 2012-05-18 16:42:51 PDT
(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.
Comment 24 Levi Weintraub 2012-05-18 16:43:36 PDT
(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
Comment 25 WebKit Review Bot 2012-05-18 16:46:24 PDT
Comment on attachment 142790 [details]
Patch for landing

Clearing flags on attachment: 142790

Committed r117658: <http://trac.webkit.org/changeset/117658>
Comment 26 WebKit Review Bot 2012-05-18 16:46:40 PDT
All reviewed patches have been landed.  Closing bug.