Bug 90865

Summary:

[CSSRegions]Crash when moving anonymous block children inside a named flow

Product:

WebKit

Reporter:

Mihnea Ovidenie <mihnea>

Component:

CSS

Assignee:

Mihnea Ovidenie <mihnea>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andreionea3000, eric, inferno, jchaffraix, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch for landing	none

Mihnea Ovidenie

Reported 2012-07-10 02:10:35 PDT

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>

Attachments
Patch (14.90 KB, patch) 2012-07-17 07:23 PDT, Andrei Onea	no flags	Details Formatted Diff Diff
Patch (15.09 KB, patch) 2012-08-03 07:01 PDT, Andrei Onea	no flags	Details Formatted Diff Diff
Patch (8.83 KB, patch) 2012-08-23 05:58 PDT, Andrei Onea	no flags	Details Formatted Diff Diff
Patch for landing (8.97 KB, patch) 2012-08-23 09:17 PDT, Andrei Onea	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Andrei Onea

Comment 1 2012-07-17 07:23:41 PDT

Created attachment 152759 [details] Patch

Andrei Onea

Comment 2 2012-08-03 07:01:47 PDT

Created attachment 156361 [details] Patch

Mihnea Ovidenie

Comment 3 2012-08-23 00:00:15 PDT

Hi Abhishek, Would you please Take a look and review it? With this one, we are closing 90202. Thanks, Mihnea(vacation until 31 August)

Abhishek Arya

Comment 4 2012-08-23 00:36:20 PDT

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.

Andrei Onea

Comment 5 2012-08-23 05:58:19 PDT

Created attachment 160141 [details] Patch

Abhishek Arya

Comment 6 2012-08-23 07:49:42 PDT

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.

Andrei Onea

Comment 7 2012-08-23 08:47:06 PDT

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

Abhishek Arya

Comment 8 2012-08-23 08:55:22 PDT

(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

Andrei Onea

Comment 9 2012-08-23 09:17:10 PDT

Created attachment 160182 [details] Patch for landing

WebKit Review Bot

Comment 10 2012-08-23 11:45:25 PDT

Comment on attachment 160182 [details] Patch for landing Clearing flags on attachment: 160182 Committed r126459: <http://trac.webkit.org/changeset/126459>

WebKit Review Bot

Comment 11 2012-08-23 11:45:28 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.