Fix crash when establishing an inline continuation inside a block continuation.

Fix crash when establishing an inline continuation inside a block continuation.

When attempting to add a child into a block continuation C
(RenderBlock::addChildToContinuation()), and there's a beforeChild
specified, we used to jump directly to the parent of beforeChild, and
insert it there (by calling addChildIgnoringContinuation() on
beforeChild->parent()). Normal behavior (when block continuations
aren't involved) would be to analyze the renderers in the ancestry
chain between beforeChild and C. This way we perform necessary
adjustments to the tree, such as inserting additional anonymous table
part objects. However, this wasn't done when block continuations were
involved.

What really caused the crash in this case was that there's no override
for addChildIgnoringContinuation() in LayoutTable, so it'd just use
the one in RenderBlock, and that way we'd never get to
LayoutTable::addChild(), which is exactly where we make sure that
children aren't inserted at invalid places (like putting a
RenderInline as a direct child of LayoutTable).

The solution is to find the nearest RenderBlockFlow ancestor of
beforeChild and try to insert it there, and additionally add an
addChildIgnoringContinuation() override in LayoutTable for good
measure (since it's dangerous to use the one in
RenderBlock).

Note that block continuations are only used in the old / current
multicol code, and even then only if there's a spanner that's not a
direct child of a multicol container. Support for block continuations
can be removed when the old multicol code is removed.

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

[mstensho (USE GERRIT), 2015-01-28 10:43:15]

mstensho@opera.com changed reviewers:
+ dsinclair@chromium.org, jchaffraix@chromium.org, leviw@chromium.org

[mstensho (USE GERRIT), 2015-01-28 10:43:16]

Should add some more tests, but I'll wait and see what you guys think first.
Should some parts of this fix be removed? They're not all strictly required to
fix the crash. The one in TableLayout will do, although layout will be wrong-ish
then. The change in RenderBlock both fixes the crashes and ensures correct
layout (as far as I have tested).

Please also see my review comments on patch set 1.

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
Source/core/layout/LayoutTable.h:126: virtual void
addChildIgnoringContinuation(RenderObject* newChild, RenderObject* beforeChild =
0) override;
LayoutTable has its own very special implementation of addChild(), but used to
use RenderBlock's implementation of addChildIgnoringContinuation(), which deals
with funny things like insertion of anonymous block boxes around things that
need it (this is only for block containers, and probably flexboxes and grids).
More importantly, though, calling LayoutTable::addChildIgnoringContinuation()
without a local override here would skip LayoutTable::addChild(), which is where
we take care of redirecting to (or even inserting) the right table object
parent.

This change alone (adding a correct addChildIgnoringContinuation() override)
would actually fix the crash, although we might still end up placing the child
inside a table when we shouldn't (which should be harmless apart from
potentially broken layout).

Even with the fix I have in RenderBlock, this change makes sense, though.

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:453: ASSERT(!beforeChild ||
beforeChild->parent()->isAnonymousColumnSpanBlock() ||
beforeChild->parent()->isRenderBlock());
This was probably here because someone knew that inserting stuff beside stuff
inside tables didn't work. Forgot to check that it wasn't a LayoutTable,
though...

The fix below copes with any type of parent, e.g. table rows and what not.

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

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:456: // Don't attempt to insert into
something that isn't a RenderBlockFlow (block
This is an attempt at a proper fix for the bug, and this chunk of code is really
all it takes. The other fixes are just for safety and sanity.

This change makes sure to look further up in the ancestry chain (of
|beforeChild|), so that we:

1. Don't bypass the table object structure correction machinery (we're calling
addChildIgnoringContinuation(); imagine what that does on LayoutTable, for
instance)
2. Insert it at the right place. We'll now drill down to the right ancestor, and
even insert anonymous table objects if that's required for the new child.

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:465: beforeChildParent =
toRenderBoxModelObject(beforeChildParent->parent());
Could add some assertions here, but I'll wait and see what you guys think of the
general direction first.

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

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderInline.cpp:511:
post->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation();
If we only keep this change but drop the ones in RenderBlock and LayoutTable,
the crash will go away, but we'll still get assertion failures in
RenderInline::layout() in the test case. Which means that it'd be hard to write
a test that doesn't crash in debug.

What splitInlines() might end up doing on a rainy day is beyond anyone's
anticipation. The crash in this bug happens because |post| is dead after having
called splitInlines(), but that was because the render tree was borked. With
either the fix in RenderBlock or LayoutTable, this won't happen anymore in our
case, but there may be other cases that we're not aware of where it may also
happen. So I was thinking that we might as well order layout with side dishes up
front, before the earthquake.

[dsinclair, 2015-01-28 16:35:01]

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

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderInline.cpp:511:
post->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation();
On 2015/01/28 at 10:43:16, mstensho wrote:
> If we only keep this change but drop the ones in RenderBlock and LayoutTable,
the crash will go away, but we'll still get assertion failures in
RenderInline::layout() in the test case. Which means that it'd be hard to write
a test that doesn't crash in debug.
> 
> What splitInlines() might end up doing on a rainy day is beyond anyone's
anticipation. The crash in this bug happens because |post| is dead after having
called splitInlines(), but that was because the render tree was borked. With
either the fix in RenderBlock or LayoutTable, this won't happen anymore in our
case, but there may be other cases that we're not aware of where it may also
happen. So I was thinking that we might as well order layout with side dishes up
front, before the earthquake.


So, this is moved up just to guard against the case where splitInlines could
delete the pre, block or post renderers? This sounds like it's still going to be
a pretty nasty pit to step in later. Would it be possible to leave this part of
the change out and file a seperate bug to figure out how to make this sane?

Or, immediately after splitInlines, can we null out pre, block and post so we
make sure they aren't touched again in case they go wonky in splitInlines?

[mstensho (USE GERRIT), 2015-01-28 16:51:19]

Any comments on the other parts of the patch?

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

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderInline.cpp:511:
post->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation();
On 2015/01/28 16:35:01, dsinclair wrote:
> On 2015/01/28 at 10:43:16, mstensho wrote:
> > If we only keep this change but drop the ones in RenderBlock and
LayoutTable,
> the crash will go away, but we'll still get assertion failures in
> RenderInline::layout() in the test case. Which means that it'd be hard to
write
> a test that doesn't crash in debug.
> > 
> > What splitInlines() might end up doing on a rainy day is beyond anyone's
> anticipation. The crash in this bug happens because |post| is dead after
having
> called splitInlines(), but that was because the render tree was borked. With
> either the fix in RenderBlock or LayoutTable, this won't happen anymore in our
> case, but there may be other cases that we're not aware of where it may also
> happen. So I was thinking that we might as well order layout with side dishes
up
> front, before the earthquake.
> 
> 
> So, this is moved up just to guard against the case where splitInlines could
> delete the pre, block or post renderers?

Correct. But I don't know if would still be possible for that to happen if the
rest of the patch is taken in. But splitInlines() calls moveChildrenTo(), which
calls addChild(). RenderBlock::addChildIgnoringAnonymousColumnBlocks() ends with
"// this object may be dead here". But it is admittedly weird to worry about
that here in splitFlow(). moveChildrenTo() would also be in trouble if its
|toBoxModelObject| gets killed. In my case it did happen at the last child,
though, so it survived.

> This sounds like it's still going to be
> a pretty nasty pit to step in later. Would it be possible to leave this part
of
> the change out and file a seperate bug to figure out how to make this sane?

I can just remove it. It's a lousy fix anyway, since it depends on the
underlying code to attempt to shoot itself in the foot and miss.

[dsinclair, 2015-01-28 17:08:06]

With the mentioned change, this LGTM. It's marked against M42 which hasn't
branched yet, so I don't see any reason to split the change as we don't need to
merge.

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

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderInline.cpp:511:
post->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation();
On 2015/01/28 at 16:51:19, mstensho wrote:
> On 2015/01/28 16:35:01, dsinclair wrote:
> > On 2015/01/28 at 10:43:16, mstensho wrote:
> > > If we only keep this change but drop the ones in RenderBlock and
LayoutTable,
> > the crash will go away, but we'll still get assertion failures in
> > RenderInline::layout() in the test case. Which means that it'd be hard to
write
> > a test that doesn't crash in debug.
> > > 
> > > What splitInlines() might end up doing on a rainy day is beyond anyone's
> > anticipation. The crash in this bug happens because |post| is dead after
having
> > called splitInlines(), but that was because the render tree was borked. With
> > either the fix in RenderBlock or LayoutTable, this won't happen anymore in
our
> > case, but there may be other cases that we're not aware of where it may also
> > happen. So I was thinking that we might as well order layout with side
dishes up
> > front, before the earthquake.
> > 
> > 
> > So, this is moved up just to guard against the case where splitInlines could
> > delete the pre, block or post renderers?
> 
> Correct. But I don't know if would still be possible for that to happen if the
rest of the patch is taken in. But splitInlines() calls moveChildrenTo(), which
calls addChild(). RenderBlock::addChildIgnoringAnonymousColumnBlocks() ends with
"// this object may be dead here". But it is admittedly weird to worry about
that here in splitFlow(). moveChildrenTo() would also be in trouble if its
|toBoxModelObject| gets killed. In my case it did happen at the last child,
though, so it survived.
> 
> > This sounds like it's still going to be
> > a pretty nasty pit to step in later. Would it be possible to leave this part
of
> > the change out and file a seperate bug to figure out how to make this sane?
> 
> I can just remove it. It's a lousy fix anyway, since it depends on the
underlying code to attempt to shoot itself in the foot and miss.


Yea, lets put this back in that case, and file a bug to look into make this
safer (if possible).

[mstensho (USE GERRIT), 2015-01-28 17:19:55]

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

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderInline.cpp:511:
post->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation();
On 2015/01/28 17:08:06, dsinclair wrote:
> On 2015/01/28 at 16:51:19, mstensho wrote:
> > On 2015/01/28 16:35:01, dsinclair wrote:
> > > On 2015/01/28 at 10:43:16, mstensho wrote:
> > > > If we only keep this change but drop the ones in RenderBlock and
> LayoutTable,
> > > the crash will go away, but we'll still get assertion failures in
> > > RenderInline::layout() in the test case. Which means that it'd be hard to
> write
> > > a test that doesn't crash in debug.
> > > > 
> > > > What splitInlines() might end up doing on a rainy day is beyond anyone's
> > > anticipation. The crash in this bug happens because |post| is dead after
> having
> > > called splitInlines(), but that was because the render tree was borked.
With
> > > either the fix in RenderBlock or LayoutTable, this won't happen anymore in
> our
> > > case, but there may be other cases that we're not aware of where it may
also
> > > happen. So I was thinking that we might as well order layout with side
> dishes up
> > > front, before the earthquake.
> > > 
> > > 
> > > So, this is moved up just to guard against the case where splitInlines
could
> > > delete the pre, block or post renderers?
> > 
> > Correct. But I don't know if would still be possible for that to happen if
the
> rest of the patch is taken in. But splitInlines() calls moveChildrenTo(),
which
> calls addChild(). RenderBlock::addChildIgnoringAnonymousColumnBlocks() ends
with
> "// this object may be dead here". But it is admittedly weird to worry about
> that here in splitFlow(). moveChildrenTo() would also be in trouble if its
> |toBoxModelObject| gets killed. In my case it did happen at the last child,
> though, so it survived.
> > 
> > > This sounds like it's still going to be
> > > a pretty nasty pit to step in later. Would it be possible to leave this
part
> of
> > > the change out and file a seperate bug to figure out how to make this
sane?
> > 
> > I can just remove it. It's a lousy fix anyway, since it depends on the
> underlying code to attempt to shoot itself in the foot and miss.
> 
> 
> Yea, lets put this back in that case, and file a bug to look into make this
> safer (if possible).

Done.

Sounds like something to do while working on crbug.com/417556 - added a comment
there.

[Julien - ping for review, 2015-01-28 17:29:53]

Would you have a dump before the issue to see what's wrong? Without it, it is
difficult to say if the direction is correct.

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
Source/core/layout/LayoutTable.h:125: virtual void addChild(RenderObject* child,
RenderObject* beforeChild = 0) override final;
This is redundant, LayoutTable is marked as final already.

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
Source/core/layout/LayoutTable.h:126: virtual void
addChildIgnoringContinuation(RenderObject* newChild, RenderObject* beforeChild =
0) override;
On 2015/01/28 at 10:43:16, mstensho wrote:
> LayoutTable has its own very special implementation of addChild(), but used to
use RenderBlock's implementation of addChildIgnoringContinuation(), which deals
with funny things like insertion of anonymous block boxes around things that
need it (this is only for block containers, and probably flexboxes and grids).
More importantly, though, calling LayoutTable::addChildIgnoringContinuation()
without a local override here would skip LayoutTable::addChild(), which is where
we take care of redirecting to (or even inserting) the right table object
parent.
> 
> This change alone (adding a correct addChildIgnoringContinuation() override)
would actually fix the crash, although we might still end up placing the child
inside a table when we shouldn't (which should be harmless apart from
potentially broken layout).
> 
> Even with the fix I have in RenderBlock, this change makes sense, though.

We didn't need the override AFAICT as we never expected a continuation in a
table (thus the ASSERT you're removing). It may be an oversight.

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

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:464: while (beforeChildParent &&
!beforeChildParent->isRenderBlockFlow())
This code can go past |this| while walking up the tree, not sure if it's of any
significance.

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:466: ASSERT(beforeChildParent);
I don't know how:

beforeChildParent->addChildIgnoringContinuation(newChild, beforeChild);

is correct knowing that |beforeChildParent| is not the direct parent of
|beforeChild| but an ancestor now.

[mstensho (USE GERRIT), 2015-01-28 17:51:26]

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
Source/core/layout/LayoutTable.h:125: virtual void addChild(RenderObject* child,
RenderObject* beforeChild = 0) override final;
On 2015/01/28 17:29:53, Julien Chaffraix - CET wrote:
> This is redundant, LayoutTable is marked as final already.

Done.

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
Source/core/layout/LayoutTable.h:126: virtual void
addChildIgnoringContinuation(RenderObject* newChild, RenderObject* beforeChild =
0) override;
On 2015/01/28 17:29:53, Julien Chaffraix - CET wrote:
> On 2015/01/28 at 10:43:16, mstensho wrote:
> > LayoutTable has its own very special implementation of addChild(), but used
to
> use RenderBlock's implementation of addChildIgnoringContinuation(), which
deals
> with funny things like insertion of anonymous block boxes around things that
> need it (this is only for block containers, and probably flexboxes and grids).
> More importantly, though, calling LayoutTable::addChildIgnoringContinuation()
> without a local override here would skip LayoutTable::addChild(), which is
where
> we take care of redirecting to (or even inserting) the right table object
> parent.
> > 
> > This change alone (adding a correct addChildIgnoringContinuation() override)
> would actually fix the crash, although we might still end up placing the child
> inside a table when we shouldn't (which should be harmless apart from
> potentially broken layout).
> > 
> > Even with the fix I have in RenderBlock, this change makes sense, though.
> 
> We didn't need the override AFAICT as we never expected a continuation in a
> table (thus the ASSERT you're removing). It may be an oversight.

The parent of |beforeChild| in RenderBlock::addChildToContinuation() may be an
anonymous table. Well, not after my fix, but I added this override here anyway,
in case there are others who call addChildIgnoringContinuation() on tables and
expect them to behave. I could have called
RenderObject::addChildIgnoringContinuation(newChild, beforeChild) here instead,
BTW, because that method does the exact same thing (except that then you'd
definitely end up with another vtable lookup).

Should I assert !isLayoutTable() in RenderBlock::addChildIgnoringContinuation()
instead?

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

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:464: while (beforeChildParent &&
!beforeChildParent->isRenderBlockFlow())
On 2015/01/28 17:29:53, Julien Chaffraix - CET wrote:
> This code can go past |this| while walking up the tree, not sure if it's of
any
> significance.

Yeah, I added an assert against that in the second patch set.

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:466: ASSERT(beforeChildParent);
On 2015/01/28 17:29:53, Julien Chaffraix - CET wrote:
> I don't know how:
> 
> beforeChildParent->addChildIgnoringContinuation(newChild, beforeChild);
> 
> is correct knowing that |beforeChildParent| is not the direct parent of
> |beforeChild| but an ancestor now.

One of the first things RenderBlock::addChildIgnoringAnonymousColumnBlocks()
does is to do stuff if |beforeChildParent| isn't a direct child, so this should
be fine.

[Julien - ping for review, 2015-01-29 10:28:31]

lgtm

The bug has several cases, it seems like we should land them for the sake of
extra code coverage.

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
Source/core/layout/LayoutTable.h:126: virtual void
addChildIgnoringContinuation(RenderObject* newChild, RenderObject* beforeChild =
0) override;
On 2015/01/28 17:51:26, mstensho wrote:
> On 2015/01/28 17:29:53, Julien Chaffraix - CET wrote:
> > On 2015/01/28 at 10:43:16, mstensho wrote:
> > > LayoutTable has its own very special implementation of addChild(), but
used
> to
> > use RenderBlock's implementation of addChildIgnoringContinuation(), which
> deals
> > with funny things like insertion of anonymous block boxes around things that
> > need it (this is only for block containers, and probably flexboxes and
grids).
> > More importantly, though, calling
LayoutTable::addChildIgnoringContinuation()
> > without a local override here would skip LayoutTable::addChild(), which is
> where
> > we take care of redirecting to (or even inserting) the right table object
> > parent.
> > > 
> > > This change alone (adding a correct addChildIgnoringContinuation()
override)
> > would actually fix the crash, although we might still end up placing the
child
> > inside a table when we shouldn't (which should be harmless apart from
> > potentially broken layout).
> > > 
> > > Even with the fix I have in RenderBlock, this change makes sense, though.
> > 
> > We didn't need the override AFAICT as we never expected a continuation in a
> > table (thus the ASSERT you're removing). It may be an oversight.
> 
> The parent of |beforeChild| in RenderBlock::addChildToContinuation() may be an
> anonymous table. Well, not after my fix, but I added this override here
anyway,
> in case there are others who call addChildIgnoringContinuation() on tables and
> expect them to behave. I could have called
> RenderObject::addChildIgnoringContinuation(newChild, beforeChild) here
instead,
> BTW, because that method does the exact same thing (except that then you'd
> definitely end up with another vtable lookup).
> 
> Should I assert !isLayoutTable() in
RenderBlock::addChildIgnoringContinuation()
> instead?

I think it's safer to implement the function (think defense-in-depth pattern)
but we should ASSERT that we don't arrive in the override (with a comment as to
why).

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

https://codereview.chromium.org/880113002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:466: ASSERT(beforeChildParent);
On 2015/01/28 17:51:26, mstensho wrote:
> On 2015/01/28 17:29:53, Julien Chaffraix - CET wrote:
> > I don't know how:
> > 
> > beforeChildParent->addChildIgnoringContinuation(newChild, beforeChild);
> > 
> > is correct knowing that |beforeChildParent| is not the direct parent of
> > |beforeChild| but an ancestor now.
> 
> One of the first things RenderBlock::addChildIgnoringAnonymousColumnBlocks()
> does is to do stuff if |beforeChildParent| isn't a direct child, so this
should
> be fine.

Acknowledged.

https://codereview.chromium.org/880113002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/880113002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:467: ASSERT(beforeChildParent != this);
RELEASE_ASSERT as this could yield to more madness?

[mstensho (USE GERRIT), 2015-01-29 12:08:42]

Also added one additional interesting test.

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
Source/core/layout/LayoutTable.h:126: virtual void
addChildIgnoringContinuation(RenderObject* newChild, RenderObject* beforeChild =
0) override;
On 2015/01/29 10:28:31, Julien Chaffraix - CET wrote:
> On 2015/01/28 17:51:26, mstensho wrote:
> > On 2015/01/28 17:29:53, Julien Chaffraix - CET wrote:
> > > On 2015/01/28 at 10:43:16, mstensho wrote:
> > > > LayoutTable has its own very special implementation of addChild(), but
> used
> > to
> > > use RenderBlock's implementation of addChildIgnoringContinuation(), which
> > deals
> > > with funny things like insertion of anonymous block boxes around things
that
> > > need it (this is only for block containers, and probably flexboxes and
> grids).
> > > More importantly, though, calling
> LayoutTable::addChildIgnoringContinuation()
> > > without a local override here would skip LayoutTable::addChild(), which is
> > where
> > > we take care of redirecting to (or even inserting) the right table object
> > > parent.
> > > > 
> > > > This change alone (adding a correct addChildIgnoringContinuation()
> override)
> > > would actually fix the crash, although we might still end up placing the
> child
> > > inside a table when we shouldn't (which should be harmless apart from
> > > potentially broken layout).
> > > > 
> > > > Even with the fix I have in RenderBlock, this change makes sense,
though.
> > > 
> > > We didn't need the override AFAICT as we never expected a continuation in
a
> > > table (thus the ASSERT you're removing). It may be an oversight.
> > 
> > The parent of |beforeChild| in RenderBlock::addChildToContinuation() may be
an
> > anonymous table. Well, not after my fix, but I added this override here
> anyway,
> > in case there are others who call addChildIgnoringContinuation() on tables
and
> > expect them to behave. I could have called
> > RenderObject::addChildIgnoringContinuation(newChild, beforeChild) here
> instead,
> > BTW, because that method does the exact same thing (except that then you'd
> > definitely end up with another vtable lookup).
> > 
> > Should I assert !isLayoutTable() in
> RenderBlock::addChildIgnoringContinuation()
> > instead?
> 
> I think it's safer to implement the function (think defense-in-depth pattern)
> but we should ASSERT that we don't arrive in the override (with a comment as
to
> why).

Done.

https://codereview.chromium.org/880113002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/880113002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:467: ASSERT(beforeChildParent != this);
On 2015/01/29 10:28:31, Julien Chaffraix - CET wrote:
> RELEASE_ASSERT as this could yield to more madness?

Done.

[mstensho (USE GERRIT), 2015-01-29 12:10:02]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-01-29 12:10:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/880113002/60001

[commit-bot: I haz the power, 2015-01-29 13:34:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-29 13:34:30]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/48301)

[mstensho (USE GERRIT), 2015-01-29 17:49:48]

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
Source/core/layout/LayoutTable.h:126: virtual void
addChildIgnoringContinuation(RenderObject* newChild, RenderObject* beforeChild =
0) override;
On 2015/01/29 12:08:42, mstensho wrote:
> On 2015/01/29 10:28:31, Julien Chaffraix - CET wrote:
> > On 2015/01/28 17:51:26, mstensho wrote:
> > > On 2015/01/28 17:29:53, Julien Chaffraix - CET wrote:
> > > > On 2015/01/28 at 10:43:16, mstensho wrote:
> > > > > LayoutTable has its own very special implementation of addChild(), but
> > used
> > > to
> > > > use RenderBlock's implementation of addChildIgnoringContinuation(),
which
> > > deals
> > > > with funny things like insertion of anonymous block boxes around things
> that
> > > > need it (this is only for block containers, and probably flexboxes and
> > grids).
> > > > More importantly, though, calling
> > LayoutTable::addChildIgnoringContinuation()
> > > > without a local override here would skip LayoutTable::addChild(), which
is
> > > where
> > > > we take care of redirecting to (or even inserting) the right table
object
> > > > parent.
> > > > > 
> > > > > This change alone (adding a correct addChildIgnoringContinuation()
> > override)
> > > > would actually fix the crash, although we might still end up placing the
> > child
> > > > inside a table when we shouldn't (which should be harmless apart from
> > > > potentially broken layout).
> > > > > 
> > > > > Even with the fix I have in RenderBlock, this change makes sense,
> though.
> > > > 
> > > > We didn't need the override AFAICT as we never expected a continuation
in
> a
> > > > table (thus the ASSERT you're removing). It may be an oversight.
> > > 
> > > The parent of |beforeChild| in RenderBlock::addChildToContinuation() may
be
> an
> > > anonymous table. Well, not after my fix, but I added this override here
> > anyway,
> > > in case there are others who call addChildIgnoringContinuation() on tables
> and
> > > expect them to behave. I could have called
> > > RenderObject::addChildIgnoringContinuation(newChild, beforeChild) here
> > instead,
> > > BTW, because that method does the exact same thing (except that then you'd
> > > definitely end up with another vtable lookup).
> > > 
> > > Should I assert !isLayoutTable() in
> > RenderBlock::addChildIgnoringContinuation()
> > > instead?
> > 
> > I think it's safer to implement the function (think defense-in-depth
pattern)
> > but we should ASSERT that we don't arrive in the override (with a comment as
> to
> > why).
> 
> Done.

FYI, removing the assertion again. It got hit by
fast/table/crash-bad-child-table-continuation.html

[mstensho (USE GERRIT), 2015-01-29 17:50:59]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-01-29 17:52:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/880113002/80001

[mstensho (USE GERRIT), 2015-01-29 18:05:41]

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
File Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/880113002/diff/1/Source/core/layout/LayoutTab...
Source/core/layout/LayoutTable.h:126: virtual void
addChildIgnoringContinuation(RenderObject* newChild, RenderObject* beforeChild =
0) override;
On 2015/01/29 17:49:48, mstensho wrote:
> On 2015/01/29 12:08:42, mstensho wrote:
> > On 2015/01/29 10:28:31, Julien Chaffraix - CET wrote:
> > > On 2015/01/28 17:51:26, mstensho wrote:
> > > > On 2015/01/28 17:29:53, Julien Chaffraix - CET wrote:
> > > > > On 2015/01/28 at 10:43:16, mstensho wrote:
> > > > > > LayoutTable has its own very special implementation of addChild(),
but
> > > used
> > > > to
> > > > > use RenderBlock's implementation of addChildIgnoringContinuation(),
> which
> > > > deals
> > > > > with funny things like insertion of anonymous block boxes around
things
> > that
> > > > > need it (this is only for block containers, and probably flexboxes and
> > > grids).
> > > > > More importantly, though, calling
> > > LayoutTable::addChildIgnoringContinuation()
> > > > > without a local override here would skip LayoutTable::addChild(),
which
> is
> > > > where
> > > > > we take care of redirecting to (or even inserting) the right table
> object
> > > > > parent.
> > > > > > 
> > > > > > This change alone (adding a correct addChildIgnoringContinuation()
> > > override)
> > > > > would actually fix the crash, although we might still end up placing
the
> > > child
> > > > > inside a table when we shouldn't (which should be harmless apart from
> > > > > potentially broken layout).
> > > > > > 
> > > > > > Even with the fix I have in RenderBlock, this change makes sense,
> > though.
> > > > > 
> > > > > We didn't need the override AFAICT as we never expected a continuation
> in
> > a
> > > > > table (thus the ASSERT you're removing). It may be an oversight.
> > > > 
> > > > The parent of |beforeChild| in RenderBlock::addChildToContinuation() may
> be
> > an
> > > > anonymous table. Well, not after my fix, but I added this override here
> > > anyway,
> > > > in case there are others who call addChildIgnoringContinuation() on
tables
> > and
> > > > expect them to behave. I could have called
> > > > RenderObject::addChildIgnoringContinuation(newChild, beforeChild) here
> > > instead,
> > > > BTW, because that method does the exact same thing (except that then
you'd
> > > > definitely end up with another vtable lookup).
> > > > 
> > > > Should I assert !isLayoutTable() in
> > > RenderBlock::addChildIgnoringContinuation()
> > > > instead?
> > > 
> > > I think it's safer to implement the function (think defense-in-depth
> pattern)
> > > but we should ASSERT that we don't arrive in the override (with a comment
as
> > to
> > > why).
> > 
> > Done.
> 
> FYI, removing the assertion again. It got hit by
> fast/table/crash-bad-child-table-continuation.html

Reported crbug.com/453488 for the
fast/table/crash-bad-child-table-continuation.html misbehavior.

[commit-bot: I haz the power, 2015-01-29 19:03:53]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189206