Harden column splitting code against bad casts. Make sure that |curr| is a descendant of |fromBlock…

Harden column splitting code against bad casts.

Make sure that |curr| is a descendant of |fromBlock|. We need to check in every iteration of the loop because moveChildrenTo could have moved |curr|. This is a mitigation and not really a fix against a class of tree craziness.

BUG=189089
TEST=none since current fuzzed testcase does not minimize

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

[Julien - ping for review, 2013-04-19 21:21:50]

LGTM but it is probably worth mentioning that this is a mitigation (not really a
fix) against a class of tree craziness.

[inferno, 2013-04-19 21:23:08]

On 2013/04/19 21:21:50, Julien Chaffraix wrote:
> LGTM but it is probably worth mentioning that this is a mitigation (not really
a
> fix) against a class of tree craziness.

Fixed in description.

[esprehn, 2013-04-19 21:24:52]

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

https://codereview.chromium.org/13852041/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlock.cpp:628: while (curr &&
curr->isDescendantOf(fromBlock) && curr != fromBlock) {
This isn't right. The loop is going from curr through ->parent() so you only
need to check this once before the loop. Right now you're walking up through all
the levels of the tree repeatedly.

[inferno, 2013-04-19 21:27:29]

On 2013/04/19 21:24:52, esprehn wrote:
>
https://codereview.chromium.org/13852041/diff/1/Source/core/rendering/RenderB...
> File Source/core/rendering/RenderBlock.cpp (right):
> 
>
https://codereview.chromium.org/13852041/diff/1/Source/core/rendering/RenderB...
> Source/core/rendering/RenderBlock.cpp:628: while (curr &&
> curr->isDescendantOf(fromBlock) && curr != fromBlock) {
> This isn't right. The loop is going from curr through ->parent() so you only
> need to check this once before the loop. Right now you're walking up through
all
> the levels of the tree repeatedly.

This is a mitigation, moveChildrenTo uses appendChild which can sometimes cause
curr to not be the in tree chain at all. That is why i am checking in every
iteration.

[inferno, 2013-04-19 21:29:47]

On 2013/04/19 21:27:29, inferno wrote:
> On 2013/04/19 21:24:52, esprehn wrote:
> >
>
https://codereview.chromium.org/13852041/diff/1/Source/core/rendering/RenderB...
> > File Source/core/rendering/RenderBlock.cpp (right):
> > 
> >
>
https://codereview.chromium.org/13852041/diff/1/Source/core/rendering/RenderB...
> > Source/core/rendering/RenderBlock.cpp:628: while (curr &&
> > curr->isDescendantOf(fromBlock) && curr != fromBlock) {
> > This isn't right. The loop is going from curr through ->parent() so you only
> > need to check this once before the loop. Right now you're walking up through
> all
> > the levels of the tree repeatedly.
> 
> This is a mitigation, moveChildrenTo uses appendChild which can sometimes
cause
> curr to not be the in tree chain at all. That is why i am checking in every
> iteration.

There have been several bug in the past for this and i am tired of fixing each
and every variant of this.
http://trac.webkit.org/changeset/142922
http://trac.webkit.org/changeset/138988, etc

[esprehn, 2013-04-19 21:33:19]

Could you mention in the issue description that you need to check this in every
iteration because moveChildrenTo can move the child?

ex. Make sure that |curr| is a descendant of |fromBlock|. We need to check in
every iteration of the loop because moveChildrenTo could have moved |curr|.

[inferno, 2013-04-19 21:34:17]

On 2013/04/19 21:33:19, esprehn wrote:
> Could you mention in the issue description that you need to check this in
every
> iteration because moveChildrenTo can move the child?
> 
> ex. Make sure that |curr| is a descendant of |fromBlock|. We need to check in
> every iteration of the loop because moveChildrenTo could have moved |curr|.

done!

[commit-bot: I haz the power, 2013-04-19 21:35:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/inferno@chromium.org/13852041/1

[commit-bot: I haz the power, 2013-04-19 21:35:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/inferno@chromium.org/13852041/1

[inferno, 2013-04-19 21:44:58]

Committed patchset #1 manually as r148760 (presubmit successful).