[CSS Grid Layout] Avoid inserting grid items twice in the grid

[CSS Grid Layout] Avoid inserting grid items twice in the grid

Given a grid with one item wrapped by an anonymous block, every attempt to
insert a new grid item before that one will trigger a crash due to a double
call to RenderGrid::addChild for the new item, in the first one beforeChild
is the already present item while in the second is the anonymous block.

We should avoid inserting the new item in the grid twice by using similar
checks to the ones already present in RenderBlock.

BUG=313293
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177653

[svillar, 2014-06-25 16:51:36]

I'll appreciate a review here :)

[Manuel Rego, 2014-06-25 20:13:11]

I detected this very same issue past week. I reported it in a different
bug though: https://code.google.com/p/chromium/issues/detail?id=387255

You could probably add this bug in the description too, or mark the bug
as duplicated.

Also, if you think it's interesting maybe you can create another test 
using the example attached to the bug to increase coverage.
In your test case the element is being removed before it's added (as 
it's removed to be wrapped by <italic>), in my example it's directly
added (without removing it before).

https://codereview.chromium.org/358673003/diff/1/LayoutTests/fast/css-grid-la...
File
LayoutTests/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html
(right):

https://codereview.chromium.org/358673003/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html:2:
<p>This test checks that adding a new grid item before an already existing item
wrapped by an anonymous block does not crash under ASAN.</p>
Actually you don't need an ASAN build, but a regular debug build will 
assert with:
ASSERTION FAILED: !m_gridItemsIndexesMap.contains(child)

https://codereview.chromium.org/358673003/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html:13:
window.getSelection().selectAllChildren(container);
Nit: I guess it's enough with select the children of the grid itself to 
reproduce the issue. I mean, you don't need to make italic the
description paragraph.

https://codereview.chromium.org/358673003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/358673003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:239: addChildToIndexesMap(newChildBox);
Probably we could add an ASSERT here, similar to the one in 
RenderGrid::addChildToIndexesMap(), to be sure that the map doesn't
contain the element yet. If this is called twice again at some point
it'll help to detect it.

[Julien - ping for review, 2014-06-25 21:25:08]

https://codereview.chromium.org/358673003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/358673003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:215: dirtyGrid();
Do we really need to invalidate the grid in this case if we assume we are going
to get called for the real children?

Also other renderers (tables, blocks) have to deal with similar issues and they
end up splitting the renderer in 2. I think we may need some similar logic here
(see splitAnonymousBoxesAroundChild).

https://codereview.chromium.org/358673003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:239: addChildToIndexesMap(newChildBox);
On 2014/06/25 20:13:11, Manuel Rego wrote:
> Probably we could add an ASSERT here, similar to the one in 
> RenderGrid::addChildToIndexesMap(), to be sure that the map doesn't
> contain the element yet. If this is called twice again at some point
> it'll help to detect it.

I would make it a RELEASE_ASSERT as it seems like a potential for a disaster
down the road so we may as well catch it earlier.

[svillar, 2014-06-26 09:45:53]

Thanks for the very useful comments guys.

https://codereview.chromium.org/358673003/diff/1/LayoutTests/fast/css-grid-la...
File
LayoutTests/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html
(right):

https://codereview.chromium.org/358673003/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html:2:
<p>This test checks that adding a new grid item before an already existing item
wrapped by an anonymous block does not crash under ASAN.</p>
On 2014/06/25 20:13:11, Manuel Rego wrote:
> Actually you don't need an ASAN build, but a regular debug build will 
> assert with:
> ASSERTION FAILED: !m_gridItemsIndexesMap.contains(child)

Yeah but the issue was detected with ASAN, and the issue is not really the
assertion but the heap after free, that's why I decided to mention it.

https://codereview.chromium.org/358673003/diff/1/LayoutTests/fast/css-grid-la...
LayoutTests/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html:13:
window.getSelection().selectAllChildren(container);
On 2014/06/25 20:13:11, Manuel Rego wrote:
> Nit: I guess it's enough with select the children of the grid itself to 
> reproduce the issue. I mean, you don't need to make italic the
> description paragraph.

Sure, I'll switch to selecting the grid children.

https://codereview.chromium.org/358673003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/358673003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderGrid.cpp:215: dirtyGrid();
On 2014/06/25 21:25:08, Julien Chaffraix - PST wrote:
> Do we really need to invalidate the grid in this case if we assume we are
going
> to get called for the real children?

Good point, should be safe to just return indeed.

> Also other renderers (tables, blocks) have to deal with similar issues and
they
> end up splitting the renderer in 2. I think we may need some similar logic
here
> (see splitAnonymousBoxesAroundChild).

Hmm let's take a look.

[Manuel Rego, 2014-07-01 13:05:11]

I've been testing the new patch and as expected it also fixes the example
(using grid-auto-flow "stack" as explained below) in
https://code.google.com/p/chromium/issues/detail?id=387255

However, if you add the following style "* { position: absolute; }"
(which is reported in
https://code.google.com/p/chromium/issues/detail?id=379460)
it crashes with a completely different backtrace:

b0x00000000028b86f3 in WebCore::RenderObject::assertRendererLaidOut
(this=0x2528bce20220) at
../../third_party/WebKit/Source/core/rendering/RenderObject.h:241
241	        ASSERT_WITH_SECURITY_IMPLICATION(!needsLayout());
#0  0x00000000028b86f3 in WebCore::RenderObject::assertRendererLaidOut
(this=0x2528bce20220) at
../../third_party/WebKit/Source/core/rendering/RenderObject.h:241
#1  0x00000000028b8720 in WebCore::RenderObject::assertSubtreeIsLaidOut
(this=0x2528bce04010) at
../../third_party/WebKit/Source/core/rendering/RenderObject.h:247
#2  0x00000000028ae23d in WebCore::FrameView::layout (this=0x14f978708010,
allowSubtree=true) at
../../third_party/WebKit/Source/core/frame/FrameView.cpp:999
#3  0x00000000021d01b7 in WebCore::Document::updateLayout (this=0x103fada04b90)
at ../../third_party/WebKit/Source/core/dom/Document.cpp:1919
#4  0x00000000021d0412 in
WebCore::Document::updateLayoutIgnorePendingStylesheets (this=0x103fada04b90,
runPostLayoutTasks=WebCore::Document::RunPostLayoutTasksAsyhnchronously)
    at ../../third_party/WebKit/Source/core/dom/Document.cpp:1975
#5  0x0000000002b130c6 in WebCore::DeleteSelectionCommand::fixupWhitespace
(this=0x14f9787b0490) at
../../third_party/WebKit/Source/core/editing/DeleteSelectionCommand.cpp:569
#6  0x0000000002b14a7b in WebCore::DeleteSelectionCommand::doApply
(this=0x14f9787b0490) at
../../third_party/WebKit/Source/core/editing/DeleteSelectionCommand.cpp:826
#7  0x0000000002b023f2 in WebCore::CompositeEditCommand::applyCommandToComposite
(this=0x14f9786f0850, prpCommand=...) at
../../third_party/WebKit/Source/core/editing/CompositeEditCommand.cpp:244
#8  0x0000000002b04915 in WebCore::CompositeEditCommand::deleteSelection
(this=0x14f9786f0850, smartDelete=false, mergeBlocksAfterDelete=false,
expandForSpecialElements=false, sanitizeMarkup=true)
    at ../../third_party/WebKit/Source/core/editing/CompositeEditCommand.cpp:560

https://codereview.chromium.org/358673003/diff/20001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html
(right):

https://codereview.chromium.org/358673003/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html:3:
<div id=grid style="display: grid;">X</div>
After https://codereview.chromium.org/333563003/ you'll need to set
grid-auto-flow to "stack" or the test won't crash (even without your patch).

On the other hand, once "stack" implementation is done. We'll be marking
the grid as dirty if it contains any auto-positioned item.
In any of the auto-placement modes (sparse, dense or stack) we need to
find an empty place to insert the auto-positioned items, so if the grid
contains any of those, we have to dirty it in RenderGrid::addChild().

This means that this kind of tests won't do anything, as the grid is going
to be marked as dirty when dealing with anonymous items (which always
are auto-positioned).

So, I'm not sure what's the best approach to carry on with these issues.

[svillar, 2014-07-07 08:46:33]

Ping reviewers

[Julien - ping for review, 2014-07-07 17:34:41]

lgtm

https://codereview.chromium.org/358673003/diff/20001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html
(right):

https://codereview.chromium.org/358673003/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-item-before-anonymous-child-crash.html:3:
<div id=grid style="display: grid;">X</div>
On 2014/07/01 13:05:11, Manuel Rego wrote:
> After https://codereview.chromium.org/333563003/ you'll need to set
> grid-auto-flow to "stack" or the test won't crash (even without your patch).
> 
> On the other hand, once "stack" implementation is done. We'll be marking
> the grid as dirty if it contains any auto-positioned item.
> In any of the auto-placement modes (sparse, dense or stack) we need to
> find an empty place to insert the auto-positioned items, so if the grid
> contains any of those, we have to dirty it in RenderGrid::addChild().
> 
> This means that this kind of tests won't do anything, as the grid is going
> to be marked as dirty when dealing with anonymous items (which always
> are auto-positioned).
> 
> So, I'm not sure what's the best approach to carry on with these issues.

It's fine to land this change and test case to ensure we don't regress (assuming
it has the grid-auto-flow set to 'stack'). Even if we change the way we handle
positioned elements, it makes sure we have some smoke-tests coverage and ensures
that this very bug doesn't come back.

[svillar, 2014-07-08 08:50:01]

Added the grid-auto-flow: stack

[svillar, 2014-07-08 08:50:45]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2014-07-08 08:51:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/svillar@igalia.com/358673003/40001

[commit-bot: I haz the power, 2014-07-08 09:52:37]

Change committed as 177653