Bug 76574

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>

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

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

Created attachment 142375 [details] Patch

I'm sorry for not having fixed this sooner!

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?

(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.

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

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 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?

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 :)

Ping?

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?

Created attachment 142790 [details] Patch for landing

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 on attachment 142790 [details] Patch for landing Odd. Levi, would you re-do the honors?

Comment on attachment 142790 [details] Patch for landing Most certainly. Epic ChangLog FTW :)

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.

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

Comment on attachment 142790 [details] Patch for landing Let's see if my change has propagated...

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

(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.

(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 on attachment 142790 [details] Patch for landing Clearing flags on attachment: 142790 Committed r117658: <http://trac.webkit.org/changeset/117658>

All reviewed patches have been landed. Closing bug.