Use the container() chain when locating the containing flow thread.

Use the container() chain when locating the containing flow thread.

Using containingBlock() caused us to miss relatively positioned inlines (that
serve as containing blocks for absolutely positioned descendants), and, more
dangerously (this bug), we'd miss absolutely positioned non-LayoutBlock
ancestors, causing us to find the ancestral flow thread, even though we were
not contained by it. This would in turn make us assume that the multicol
container had column content, which in turn would make us just blindly assume
that there was a column set object there.

A video element establishes a shadow DOM, and if the video was absolutely
positioned, the elements in there would erroneously find the flow thread,
because we skipped the absolutely positioned ancestral video, because it is
not a LayoutBlock.

While bug 496421 is listed as one of the bugs fixed here, it should be pointed
out that the fix is rather speculative in that regard, since since it's fixing
different but rather related crash than the one in the original report. The
crash was triggered by the same TC, though.

BUG=496421, 497524, 497435
R=dsinclair@chromium.org,esprehn@chromium.org

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

[esprehn, 2015-06-08 20:01:52]

esprehn@chromium.org changed reviewers:
+ ojan@chromium.org

[ojan, 2015-06-08 23:51:23]

lgtm

It's so hard to be sure this is correct. containingBlock and container are so
subtly different. We really need to redesign the code so that we don't need both
of these, but that's a very difficult and bigger change obviously.

The primary scary difference is the following in containingBlock:
            if (o->style()->hasInFlowPosition() && o->isInline() &&
!o->isReplaced()) {
                o = o->containingBlock();
                break;
            }

But I can't really tell what that code is doing. I did noticed that the comment
5 lines above it is actually for this code. This was added in
https://chromium.googlesource.com/chromium/blink/+/d27fecb95d6fb60655506f5414...

[mstensho (USE GERRIT), 2015-06-09 07:22:21]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-06-09 07:22:33]

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

[mstensho (USE GERRIT), 2015-06-09 07:38:12]

On 2015/06/08 23:51:23, ojan wrote:
> lgtm
> 
> It's so hard to be sure this is correct. containingBlock and container are so
> subtly different. We really need to redesign the code so that we don't need
both
> of these, but that's a very difficult and bigger change obviously.
> 
> The primary scary difference is the following in containingBlock:
>             if (o->style()->hasInFlowPosition() && o->isInline() &&
> !o->isReplaced()) {
>                 o = o->containingBlock();
>                 break;
>             }

That one, and:

        while (o && ((o->isInline() && !o->isReplaced()) ||
!o->isLayoutBlock()))
            o = o->parent();

... further below. In my case the candidate was a LayoutVideo, i.e. not a
LayoutBlock, so we skipped it (then again, I'm not sure how happy the code would
be about having a non-LayoutBlock as a container, but that's not an issue in my
case anyway, since I just need to walk the proper containing block chain until I
find a flow thread). On a separate note, it's also very confusing that
isReplaced() doesn't mean exactly that. Inline-blocks and inline-tables (and
other inline-foos, I suppose) count as replaced. Other elements that are defined
as replaced in the spec, are not in Blink, because they happen to be rendered by
the layout engine (INPUT, TEXTAREA, etc.).

[commit-bot: I haz the power, 2015-06-09 08:30:40]

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