Bug 135563 – ASSERTION FAILED in WebCore::RenderFlowThread::getRegionRangeForBox

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Renata Hodovan]

Created attachment 235972 [details]
Test case

The following test fails on debug WebKit (r171973):

<style>
* {
    -webkit-columns:2;
    position:absolute;
}
</style>a


Backtrace:

ASSERTION FAILED: isRenderView() || (region && flowThread)
../../Source/WebCore/rendering/RenderBox.cpp(140) : WebCore::RenderRegion* WebCore::RenderBox::clampToStartAndEndRegions(WebCore::RenderRegion*) const

....

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff97334700 (LWP 16769)]
0x00007ffff30191c8 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:329
329	    *(int *)(uintptr_t)0xbbadbeef = 0;
#0  0x00007ffff30191c8 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:329
#1  0x00007ffff3da4476 in WebCore::RenderBox::clampToStartAndEndRegions (this=0x7da270, region=0x79e090) at ../../Source/WebCore/rendering/RenderBox.cpp:140
#2  0x00007ffff3db2bc4 in WebCore::RenderBox::containingBlockLogicalWidthForPositioned (this=0x7da340, containingBlock=0x7da270, region=0x79e090, checkForPerpendicularWritingMode=true) at ../../Source/WebCore/rendering/RenderBox.cpp:3102
#3  0x00007ffff3db38b8 in WebCore::RenderBox::computePositionedLogicalWidth (this=0x7da340, computedValues=..., region=0x79e090) at ../../Source/WebCore/rendering/RenderBox.cpp:3251
#4  0x00007ffff3dae3a3 in WebCore::RenderBox::computeLogicalWidthInRegion (this=0x7da340, computedValues=..., region=0x79e090) at ../../Source/WebCore/rendering/RenderBox.cpp:2274
#5  0x00007ffff3dafcf8 in WebCore::RenderBox::renderBoxRegionInfo (this=0x7da340, region=0x79e090, cacheFlag=WebCore::RenderBox::CacheRenderBoxRegionInfo) at ../../Source/WebCore/rendering/RenderBox.cpp:2555
#6  0x00007ffff3da4999 in WebCore::RenderBox::borderBoxRectInRegion (this=0x91c500, region=0x79e090, cacheFlag=WebCore::RenderBox::CacheRenderBoxRegionInfo) at ../../Source/WebCore/rendering/RenderBox.cpp:212
#7  0x00007ffff3d575da in WebCore::RenderBlock::logicalLeftOffsetForContent (this=0x91c500, region=0x79e090) at ../../Source/WebCore/rendering/RenderBlock.cpp:2373
#8  0x00007ffff3d628b4 in WebCore::RenderBlock::logicalLeftOffsetForContent (this=0x91c500, blockOffset=...) at ../../Source/WebCore/rendering/RenderBlock.h:273
#9  0x00007ffff3d625f5 in WebCore::RenderBlock::logicalLeftOffsetForLine (this=0x91c500, position=..., shouldIndentText=true, logicalHeight=...) at ../../Source/WebCore/rendering/RenderBlock.h:158
#10 0x00007ffff3f5d4f5 in WebCore::LineWidth::updateAvailableWidth (this=0x7fffffffab80, replacedHeight=...) at ../../Source/WebCore/rendering/line/LineWidth.cpp:73
#11 0x00007ffff3f5d3e2 in WebCore::LineWidth::LineWidth (this=0x7fffffffab80, block=..., isFirstLine=true, shouldIndentText=WebCore::IndentText) at ../../Source/WebCore/rendering/line/LineWidth.cpp:51
#12 0x00007ffff3f55451 in WebCore::LineBreaker::nextSegmentBreak (this=0x7fffffffaea0, resolver=..., lineInfo=..., renderTextInfo=..., lastFloatFromPreviousLine=0x0, consecutiveHyphenatedLines=0, wordMeasurements=...) at ../../Source/WebCore/rendering/line/LineBreaker.cpp:93
#13 0x00007ffff3f55323 in WebCore::LineBreaker::nextLineBreak (this=0x7fffffffaea0, resolver=..., lineInfo=..., renderTextInfo=..., lastFloatFromPreviousLine=0x0, consecutiveHyphenatedLines=0, wordMeasurements=...) at ../../Source/WebCore/rendering/line/LineBreaker.cpp:82
#14 0x00007ffff3d94f7c in WebCore::RenderBlockFlow::layoutRunsAndFloatsInRange (this=0x91c500, layoutState=..., resolver=..., cleanLineStart=..., cleanLineBidiStatus=..., consecutiveHyphenatedLines=0) at ../../Source/WebCore/rendering/RenderBlockLineLayout.cpp:1081
#15 0x00007ffff3d94b58 in WebCore::RenderBlockFlow::layoutRunsAndFloats (this=0x91c500, layoutState=..., hasInlineChild=true) at ../../Source/WebCore/rendering/RenderBlockLineLayout.cpp:1025
#16 0x00007ffff3d97223 in WebCore::RenderBlockFlow::layoutLineBoxes (this=0x91c500, relayoutChildren=false, repaintLogicalTop=..., repaintLogicalBottom=...) at ../../Source/WebCore/rendering/RenderBlockLineLayout.cpp:1442
#17 0x00007ffff3d7b54e in WebCore::RenderBlockFlow::layoutInlineChildren (this=0x91c500, relayoutChildren=false, repaintLogicalTop=..., repaintLogicalBottom=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:652
#18 0x00007ffff3d7a934 in WebCore::RenderBlockFlow::layoutBlock (this=0x91c500, relayoutChildren=false, pageLogicalHeight=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:483
#19 0x00007ffff3d50987 in WebCore::RenderBlock::layout (this=0x91c500) at ../../Source/WebCore/rendering/RenderBlock.cpp:1018
#20 0x00007ffff3e06af7 in WebCore::RenderFlowThread::layout (this=0x91c500) at ../../Source/WebCore/rendering/RenderFlowThread.cpp:201
#21 0x00007ffff3ea57be in WebCore::RenderMultiColumnFlowThread::layout (this=0x91c500) at ../../Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp:135
#22 0x00007ffff3d8908a in WebCore::RenderBlockFlow::layoutSpecialExcludedChild (this=0x7da340, relayoutChildren=true) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:3629
#23 0x00007ffff3d7b338 in WebCore::RenderBlockFlow::layoutBlockChildren (this=0x7da340, relayoutChildren=true, maxFloatLogicalBottom=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:604
#24 0x00007ffff3d7a958 in WebCore::RenderBlockFlow::layoutBlock (this=0x7da340, relayoutChildren=true, pageLogicalHeight=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:485
#25 0x00007ffff3d50987 in WebCore::RenderBlock::layout (this=0x7da340) at ../../Source/WebCore/rendering/RenderBlock.cpp:1018
#26 0x00007ffff3d1ddd7 in WebCore::RenderElement::layoutIfNeeded (this=0x7da340) at ../../Source/WebCore/rendering/RenderElement.h:102
#27 0x00007ffff3d52697 in WebCore::RenderBlock::layoutPositionedObjects (this=0x7da270, relayoutChildren=true, fixedPositionObjectsOnly=false) at ../../Source/WebCore/rendering/RenderBlock.cpp:1455
#28 0x00007ffff3d7add4 in WebCore::RenderBlockFlow::layoutBlock (this=0x7da270, relayoutChildren=true, pageLogicalHeight=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:524
#29 0x00007ffff3d50987 in WebCore::RenderBlock::layout (this=0x7da270) at ../../Source/WebCore/rendering/RenderBlock.cpp:1018
#30 0x00007ffff3d1ddd7 in WebCore::RenderElement::layoutIfNeeded (this=0x7da270) at ../../Source/WebCore/rendering/RenderElement.h:102
#31 0x00007ffff3d52697 in WebCore::RenderBlock::layoutPositionedObjects (this=0x8c1e20, relayoutChildren=true, fixedPositionObjectsOnly=false) at ../../Source/WebCore/rendering/RenderBlock.cpp:1455
#32 0x00007ffff3d7add4 in WebCore::RenderBlockFlow::layoutBlock (this=0x8c1e20, relayoutChildren=true, pageLogicalHeight=...) at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:524
#33 0x00007ffff3d50987 in WebCore::RenderBlock::layout (this=0x8c1e20) at ../../Source/WebCore/rendering/RenderBlock.cpp:1018
#34 0x00007ffff3f28aad in WebCore::RenderView::layoutContent (this=0x8c1e20, state=...) at ../../Source/WebCore/rendering/RenderView.cpp:232
#35 0x00007ffff3f29166 in WebCore::RenderView::layout (this=0x8c1e20) at ../../Source/WebCore/rendering/RenderView.cpp:357
#36 0x00007ffff3acf1f4 in WebCore::FrameView::layout (this=0xa09710, allowSubtree=true) at ../../Source/WebCore/page/FrameView.cpp:1282
#37 0x00007ffff353f72f in WebCore::Document::implicitClose (this=0x8b90b0) at ../../Source/WebCore/dom/Document.cpp:2438
#38 0x00007ffff399e745 in WebCore::FrameLoader::checkCallImplicitClose (this=0x9fc3e8) at ../../Source/WebCore/loader/FrameLoader.cpp:898
#39 0x00007ffff399e4f0 in WebCore::FrameLoader::checkCompleted (this=0x9fc3e8) at ../../Source/WebCore/loader/FrameLoader.cpp:844
#40 0x00007ffff399e278 in WebCore::FrameLoader::finishedParsing (this=0x9fc3e8) at ../../Source/WebCore/loader/FrameLoader.cpp:764
#41 0x00007ffff35470db in WebCore::Document::finishedParsing (this=0x8b90b0) at ../../Source/WebCore/dom/Document.cpp:4519
#42 0x00007ffff383a805 in WebCore::HTMLConstructionSite::finishedParsing (this=0xa09ff8) at ../../Source/WebCore/html/parser/HTMLConstructionSite.cpp:395
#43 0x00007ffff3874c57 in WebCore::HTMLTreeBuilder::finished (this=0xa09fe0) at ../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2997
#44 0x00007ffff38423a0 in WebCore::HTMLDocumentParser::end (this=0x7a6530) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:451
#45 0x00007ffff384248b in WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd (this=0x7a6530) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:462
#46 0x00007ffff3840ff9 in WebCore::HTMLDocumentParser::prepareToStopParsing (this=0x7a6530) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:165
#47 0x00007ffff38424ce in WebCore::HTMLDocumentParser::attemptToEnd (this=0x7a6530) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:474
#48 0x00007ffff3842585 in WebCore::HTMLDocumentParser::finish (this=0x7a6530) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:502
#49 0x00007ffff3991445 in WebCore::DocumentWriter::end (this=0x850a80) at ../../Source/WebCore/loader/DocumentWriter.cpp:250
#50 0x00007ffff397e49b in WebCore::DocumentLoader::finishedLoading (this=0x8509e0, finishTime=0) at ../../Source/WebCore/loader/DocumentLoader.cpp:441
#51 0x00007ffff397e204 in WebCore::DocumentLoader::notifyFinished (this=0x8509e0, resource=0x88b840) at ../../Source/WebCore/loader/DocumentLoader.cpp:375
#52 0x00007ffff3a24a99 in WebCore::CachedResource::checkNotify (this=0x88b840) at ../../Source/WebCore/loader/cache/CachedResource.cpp:334
#53 0x00007ffff3a24b80 in WebCore::CachedResource::finishLoading (this=0x88b840) at ../../Source/WebCore/loader/cache/CachedResource.cpp:350
#54 0x00007ffff3a21b3a in WebCore::CachedRawResource::finishLoading (this=0x88b840, data=0x820e30) at ../../Source/WebCore/loader/cache/CachedRawResource.cpp:98
#55 0x00007ffff39d87da in WebCore::SubresourceLoader::didFinishLoading (this=0x914620, finishTime=0) at ../../Source/WebCore/loader/SubresourceLoader.cpp:310
#56 0x00007ffff39d4cc7 in WebCore::ResourceLoader::didFinishLoading (this=0x914620, finishTime=0) at ../../Source/WebCore/loader/ResourceLoader.cpp:517
#57 0x00007ffff4297429 in WebCore::readCallback (asyncResult=0x9e81d0, data=0x914c90) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1302
#58 0x00007fffec1d82ea in async_ready_callback_wrapper (source_object=0x98cb30, res=0x9e81d0, user_data=0x914c90) at ginputstream.c:519
#59 0x00007fffec1f7ceb in g_task_return_now (task=0x9e81d0) at gtask.c:1108
#60 0x00007fffec1f7d09 in complete_in_idle_cb (task=0x9e81d0) at gtask.c:1117
#61 0x00007fffeb44e2e6 in g_main_dispatch (context=0x677bc0) at gmain.c:3065
#62 g_main_context_dispatch (context=context@entry=0x677bc0) at gmain.c:3641
#63 0x00007fffeb44e638 in g_main_context_iterate (context=0x677bc0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3712
#64 0x00007fffeb44ea3a in g_main_loop_run (loop=0x6f0010) at gmain.c:3906
#65 0x00007ffff306a156 in WTF::RunLoop::run () at ../../Source/WTF/wtf/gtk/RunLoopGtk.cpp:59
#66 0x00007ffff2fa35e0 in WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> (argc=2, argv=0x7fffffffd948) at ../../Source/WebKit2/Shared/unix/ChildProcessMain.h:61
#67 0x00007ffff2fa3445 in WebKit::WebProcessMainUnix (argc=2, argv=0x7fffffffd948) at ../../Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:73
#68 0x000000000040085d in main (argc=2, argv=0x7fffffffd948) at ../../Source/WebKit2/WebProcess/EntryPoint/unix/WebProcessMain.cpp:32

[Renata Hodovan]

If you run the attached test with a release build then it causes a crash in WebCore::RenderFlowThread::getRegionRangeForBox.

[Andrei Bucur]

Created attachment 239476 [details]
WIP Patch

[Darin Adler]

Comment on attachment 239476 [details]
WIP Patch

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

> Source/WebCore/rendering/LayoutState.h:130
> +    RenderFlowThread* m_currentRenderFlowThread;

There’s a new idiom we can and should use for data member initialization now that we use new enough compilers. We would write:

    RenderFlowThread* m_currentRenderFlowThread { nullptr };

And then we would not need to change the various constructors to initialize this data member.

> Source/WebCore/rendering/RenderBlock.cpp:133
> +    RenderFlowThread* m_flowThreadContainingBlock;
> +    bool m_dirtyFlowThreadContainingBlock;

RenderFlowThread* m_flowThreadContainingBlock { nullptr };
    bool m_dirtyFlowThreadContainingBlock { true };

I find the m_dirtyFlowThreadContainingBlock data member name confusing. In fact, the same name is used below as a verb phrase. Maybe m_flowThreadContainingBlockNeedsUpdate, or reverse the sense and make it m_flowThreadContainingBlockIsValid? Or maybe use Optional<RenderFlowThread*> instead of a separate data member.

> Source/WebCore/rendering/RenderBlock.cpp:3346
> +    if (this->isRenderFlowThread())

No need for this-> here.

> Source/WebCore/rendering/RenderBlock.cpp:3351
> +    RenderBlockRareData* rareData = getRareData(this);
> +    if (!rareData)
> +        rareData = &ensureRareData(this);

This should just be:

    RenderBlockRareData& rareData = ensureRareData(this);

> Source/WebCore/rendering/RenderBlock.cpp:3361
> +    RenderBlockRareData* rareData = getRareData(this);
> +    if (!rareData)
> +        rareData = &ensureRareData(this);

This should just be:

    RenderBlockRareData& rareData = ensureRareData(this);

> Source/WebCore/rendering/RenderBlock.cpp:3369
> +RenderFlowThread* RenderBlock::locateFlowThreadContainingBlock() const

It’s not the best style to use const and const_cast to account for the side effect. Instead we would normally either make the function non-const to make the side effect clear, or alternatively make the relevant data members mutable and make updateFlowThreadContainingBlock also be const.

> Source/WebCore/rendering/RenderBlock.h:317
> +    RenderFlowThread* updateFlowThreadContainingBlock(RenderFlowThread*);
> +    void dirtyFlowThreadContainingBlock();
> +    RenderFlowThread* cachedFlowThreadContainingBlock() const;

Do all three of these need to be public? Can they be private or protected instead?

I don’t think the name “dirtyFlowThreadContainingBlock” makes it clear that “dirty” is a verb. Normally we use different naming for such things, following style from Cocoa. For example, setFlowThreadContainingBlockNeedsUpdate.

> Source/WebCore/rendering/RenderObject.cpp:2019
> +    dirtyFlowThreadContainingBlockRecursive();

I prefer a name that reflects what is being done, not the algorithm. The name “recursive” is not entirely clear to me. Does it mean this call will dirty all descendants? All ancestors?

> Source/WebCore/rendering/RenderObject.cpp:2063
> +    RenderBlock* block = toRenderBlock(this);
> +    block->dirtyFlowThreadContainingBlock();

It should be:

    toRenderBlock(*this).dirtyFlowThreadContainingBlock();

We should use a reference rather than a pointer.

> Source/WebCore/rendering/RenderObject.cpp:2072
> +    for (RenderObject* child = firstChildSlow(); child; child = child->nextSibling())
> +        child->dirtyFlowThreadContainingBlockRecursive();

It’s very strange to use firstChildSlow here. But also, it would be better to use a non-recursive algorithm. Here’s one way to write it:

    for (RenderObject* descendant = firstChild(); descendant; descendant = descendant->nextInPreOrder(this))
        descendant->dirtyFlowThreadContainingBlock();

Then we use a loop instead of a recursive function call and we don’t have to use stack space proportional to the size of the tree.

> Source/WebCore/rendering/RenderObject.cpp:2076
> +    if (!isRenderBlock()) {

Code like this makes me wonder why we aren’t using a virtual function.

> Source/WebCore/rendering/RenderObject.cpp:2077
> +        RenderBlock* cb = containingBlock();

Please don’t abbreviate to cb. Instead write something more like this:

    if (auto* containingBlock = this->containingBlock())
        flowThread = containingBlock->cachedFlowThreadContainingBlock();

> Source/WebCore/rendering/RenderObject.cpp:2081
> +        RenderBlock* block = toRenderBlock(this);

Please use a reference instead of a pointer here.

> Source/WebCore/rendering/RenderObject.cpp:2619
> +    RenderBlock* cb = containingBlock();

Please don’t use the name “cb” in new code:

    RenderBlock* containingBlock = this->containingBlock();

> Source/WebCore/rendering/RenderObject.h:222
> +    RenderFlowThread* flowThreadContainingBlock() const;

What’s behind the decision to not make this inline any more?

[Dave Hyatt]

Comment on attachment 239476 [details]
WIP Patch

So I think the only time a positioned object is looked at when the wrong flow thread is on the stack is for static position computation, which involves doing stuff with positioned objects in the normal flow. I have no real opinion on the change and think it's probably fine, but if you were looking to do something smaller, you could probably just pop the flow thread while examining the positioned object from normal flow layout, and then push the thread back on the stack when done.

[Andrei Bucur]

@Darin
Thanks for the feedback and for the useful information about the Optional template class. I'll update the patch if we decide to go down this road.

Here are the performance results for Regions and html5-full-render. The first column HAS the patch, the second column doesn't have the patch.

/Layout/RegionsAuto:Time	ms	300.90	2.75% Worse 	292.85
/Layout/RegionsAutoMaxHeight:Time	ms	1070.55	3.45% Worse 	1034.85
/Layout/RegionsExtendingSelectionMixedContent:Time	ms	2017.85	6.15% Better	2150.00
/Layout/RegionsFixed:Time	ms	935.85	4.26% Worse 	897.60
/Layout/RegionsFixedShort:Time	ms	1136.80	3.58% Worse 	1097.50
/Layout/RegionsSelectAllMixedContent:Time	ms	697.35	1.04% Better	704.65
/Layout/RegionsSelection:Time	ms	1538.70	8.20% Better	1676.10
/Layout/RegionsShapes:Time	ms	418.45	2.83% Worse 	406.95
/Parser/html5-full-render:Time	ms	3888.90		3853.65

There is a good performance gain in selection tests. However, there's a slight performance degradation for the other tests. I'll run some numbers for multi-col as well.

[Andrei Bucur]

Here are the performance results when running the tests from https://bugs.webkit.org/show_bug.cgi?id=137687 :
/Layout/Multicol/MulticolManyColumns:Time	ms	7130.25	± 0.05%	1.80% Better	7260.85	± 0.45%
/Layout/Multicol/MulticolNested:Time	ms	4604.70	± 0.17%	5.01% Better	4847.50	± 0.11%

The build using this approach proves to be 1.8% faster on a test with many columns and 5.01% faster on a test with many nested multi-column containers.

Even though the performance for regions drops a bit I see the following advantages for moving forward with this approach:
1. The multi-column implementation benefits from this.
2. The selection code and anything not using currentRenderFlowThread benefits from this.
3. We can drop the CurrentRenderFlowThreadMaintainer/Disabler classes. During the development of this patch I've discovered two places that needed CurrentRenderFlowThreadDisabler but didn't have it.

The downside is the regions code is a bit slower. I believe we can regain the lost performance in those tests by optimizing the repaint code which is the major bottleneck in those tests (each test has around 1200 regions and repainting the flow thread actually means calling repaint() 1200 times).

[Andrei Bucur]

Created attachment 239800 [details]
Patch

[Andrei Bucur]

Created attachment 239803 [details]
Patch v2

[Dave Hyatt]

Comment on attachment 239803 [details]
Patch v2

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

Seems ok. If you're trying to catch all things that can change your containing block, I would actually eyeball the containingBlock() function. transforms can do it in addition to positioning for example.

> Source/WebCore/rendering/RenderBlock.cpp:319
> +    if (oldStyle && oldStyle->position() != newStyle.position())
> +        invalidateFlowThreadContainingBlockIncludingDescendants();

transforms can change your containing block too. You may want to include that?

[Andrei Bucur]

Created attachment 239867 [details]
Patch v3

[Andrei Bucur]

I've changed some things in the patch:
- I've refactored and renamed RenderObject::removeFromRenderFlowThreadRecursive . Now it doesn't invalidate the flow thread state flag if the function descends inside a nested flow thread.
- Changing the transform state of a block also invalidates the flowThreadContainingBlock chain
- There's a new test verifying this works correctly and doesn't crash

[Dave Hyatt]

Comment on attachment 239867 [details]
Patch v3

r=me

[WebKit Commit Bot]

Comment on attachment 239867 [details]
Patch v3

Clearing flags on attachment: 239867

Committed r174761: <http://trac.webkit.org/changeset/174761>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.