Bug 135563

Summary: ASSERTION FAILED in WebCore::RenderFlowThread::getRegionRangeForBox
Product: WebKit Reporter: Renata Hodovan <rhodovan.u-szeged>
Component: Layout and RenderingAssignee: Andrei Bucur <abucur>
Status: RESOLVED FIXED    
Severity: Normal CC: abucur, commit-queue, esprehn+autocc, glenn, hyatt, kondapallykalyan, mihnea, simon.fraser, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Unspecified   
Bug Depends on: 132946    
Bug Blocks: 116980    
Attachments:
Description Flags
Test case
none
WIP Patch
none
Patch
none
Patch v2
none
Patch v3 none

Renata Hodovan
Reported 2014-08-04 05:48:00 PDT
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
Attachments
Test case (69 bytes, text/html)
2014-08-04 05:48 PDT, Renata Hodovan 		no flags
Details
WIP Patch (35.89 KB, patch)
2014-10-08 08:34 PDT, Andrei Bucur 		no flags
DetailsFormatted DiffDiff
Patch (40.67 KB, patch)
2014-10-14 08:10 PDT, Andrei Bucur 		no flags
DetailsFormatted DiffDiff
Patch v2 (40.72 KB, patch)
2014-10-14 09:47 PDT, Andrei Bucur 		no flags
DetailsFormatted DiffDiff
Patch v3 (44.76 KB, patch)
2014-10-15 05:40 PDT, Andrei Bucur 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Renata Hodovan
Comment 1 2014-09-19 05:09:54 PDT
If you run the attached test with a release build then it causes a crash in WebCore::RenderFlowThread::getRegionRangeForBox.
Andrei Bucur
Comment 2 2014-10-08 08:34:59 PDT
Created attachment 239476 [details] WIP Patch
Darin Adler
Comment 3 2014-10-08 09:05:39 PDT
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 4 2014-10-08 09:33:00 PDT
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
Comment 5 2014-10-09 06:27:33 PDT
@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
Comment 6 2014-10-14 04:46:19 PDT
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
Comment 7 2014-10-14 08:10:20 PDT
Created attachment 239800 [details] Patch
Andrei Bucur
Comment 8 2014-10-14 09:47:30 PDT
Created attachment 239803 [details] Patch v2
Dave Hyatt
Comment 9 2014-10-14 10:14:43 PDT
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
Comment 10 2014-10-15 05:40:25 PDT
Created attachment 239867 [details] Patch v3
Andrei Bucur
Comment 11 2014-10-15 05:46:00 PDT
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 12 2014-10-15 09:08:29 PDT
Comment on attachment 239867 [details] Patch v3 r=me
WebKit Commit Bot
Comment 13 2014-10-15 23:50:31 PDT
Comment on attachment 239867 [details] Patch v3 Clearing flags on attachment: 239867 Committed r174761: <http://trac.webkit.org/changeset/174761>
WebKit Commit Bot
Comment 14 2014-10-15 23:50:36 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.