When the children of an anonymous block inside a flow thread are moved, the anonymous block is first detached and then its children are moved to an upper block inside the flow thread (RenderBlock::collapseAnonymousBoxChild). In this case, the enclosing flow thread is null and it crashes in RenderObjectChildList::removeChildNode because we do not test if the enclosingRenderFlowThread is null (when moving the children with moverAllChildren). Here is a repro case: <style> .container { -webkit-column-count: 1; -webkit-flow-into: flow; } .columnSpan { -webkit-column-span: all; } </style> <div class="container"> <div id="test"> <div class="columnSpan"></div> </div> </div> <script> var test = document.getElementById("test"); test.innerHTML = "PASS"; </script>
Created attachment 152759 [details] Patch
Created attachment 156361 [details] Patch
Hi Abhishek, Would you please Take a look and review it? With this one, we are closing 90202. Thanks, Mihnea(vacation until 31 August)
Comment on attachment 156361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156361&action=review > Source/WebCore/rendering/RenderBlock.cpp:1140 > + anonBlock->moveAllChildrenTo(parent, nextSibling, child->hasLayer(), childFlowThread); Why are we adding so much complexity to moveAllChild functions, just for regions. We can probably get way with an easier fix here. How about CurrentRenderFlowThreadMaintainer local to keep the enclosingRenderFlowThread for the duration of this function. This is quite an unusual case in this function that we remove the anonymous block first and then move its children, but probably it is done to keep anonymous block alive (otherwise last anonymous block will be auto-deleted). > LayoutTests/fast/regions/move-anonymous-block-inside-named-flow-crash.html:1 > +<html> Please add <!DOCTYPE html> > LayoutTests/fast/regions/move-anonymous-block-inside-named-flow-crash.html:6 > +.flow {-webkit-flow-from: flow; width: 100px; height: 100px;} nit: spaces around { } > LayoutTests/fast/regions/move-anonymous-block-inside-named-flow-crash.html:15 > + if (window.layoutTestController) Please use window.testRunner > LayoutTests/fast/regions/move-anonymous-block-inside-named-flow-crash.html:18 > + test.innerHTML = "Bug 90865:[CSSRegions]Crash when moving anonymous block children inside a named flow. Test passes if it does not CRASH or ASSERT."; Split the message across lines.
Created attachment 160141 [details] Patch
Comment on attachment 160141 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160141&action=review > Source/WebCore/rendering/RenderObject.cpp:2440 > enclosingRenderFlowThread()->removeRenderBoxRegionInfo(toRenderBox(this)); I remember there was a helper function for these 2 lines. The assert can be moved in the helper.
(In reply to comment #6) > (From update of attachment 160141 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160141&action=review > > > Source/WebCore/rendering/RenderObject.cpp:2440 > > enclosingRenderFlowThread()->removeRenderBoxRegionInfo(toRenderBox(this)); > > I remember there was a helper function for these 2 lines. The assert can be moved in the helper. There is RenderBox::clearRenderBoxRegionInfo, but that doesn't take care of custom region styling for the object that was cached in RenderRegion. There is also RenderFlowThread::removeFlowChildInfo, which does both removeRenderBoxRegionInfo, and clearRenderObjectCustomStyle - but, obviously, I can't move the assert there. Were you thinking of the latter one?
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 160141 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=160141&action=review > > > > > Source/WebCore/rendering/RenderObject.cpp:2440 > > > enclosingRenderFlowThread()->removeRenderBoxRegionInfo(toRenderBox(this)); > > > > I remember there was a helper function for these 2 lines. The assert can be moved in the helper. > There is RenderBox::clearRenderBoxRegionInfo, but that doesn't take care of custom region styling for the object that was cached in RenderRegion. There is also RenderFlowThread::removeFlowChildInfo, which does both removeRenderBoxRegionInfo, and clearRenderObjectCustomStyle - but, obviously, I can't move the assert there. Were you thinking of the latter one? Yeah i was thinking of RenderFlowThread::removeFlowChildInfo. gotcha, you can't move the assert there, but atleast use the helper to avoid code duplication
Created attachment 160182 [details] Patch for landing
Comment on attachment 160182 [details] Patch for landing Clearing flags on attachment: 160182 Committed r126459: <http://trac.webkit.org/changeset/126459>
All reviewed patches have been landed. Closing bug.