Clear descendant floats when moving children of a multicol container to a flowthread

Clear descendant floats when moving children of a multicol container to a flowthread

We need to call multicolContainer->removeFloatingObjectsFromDescendants() in
LayoutMultiColumnFlowThread::populate(). This is because we can create a new
anonymous multi-col anonymous block and move all its container's children to
it. Any floats nested within the children which are also contained by the new multi-col's
ancestor will become unreachable later on if we attempt to delete them because the
new multi-col flowthread won't have them - and will act like a firewall during the
descent through the tree looking for the float to remove.

BUG=588178
Committed: https://crrev.com/0ab8e6e1a527d322d9e93cba0949208bd2c55550
Cr-Commit-Position: refs/heads/master@{#378862}

[rhogan, 2016-03-01 20:20:42]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com

[rhogan, 2016-03-01 20:20:42]

I have kind of run out of time/energy to reduce the test case on this any
further. Since it's a clusterfuzz crash maybe we can go with what we have.

[rhogan, 2016-03-01 20:22:21]

Description was changed from

==========
Clear descendant floats when moving children of a multicol container to a
flowthread
BUG=588178
==========

to

==========
Clear descendant floats when moving children of a multicol container to a
flowthread
We need to call multicolContainer->removeFloatingObjectsFromDescendants() in
LayoutMultiColumnFlowThread::populate(). This is because we have created a new
anonymous multi-col anonymous block and are moving all the container's children
to
it. Any floats nested within the children which are also contained by the
multi-col's ancestor will become unreachable later on if we attempt to delete
them because the new multi-col flowthread won't have them - and will act like a
firewall during the descent through the tree looking for the float to remove.

BUG=588178
==========

[rhogan, 2016-03-01 20:22:41]

Description was changed from

==========
Clear descendant floats when moving children of a multicol container to a
flowthread
We need to call multicolContainer->removeFloatingObjectsFromDescendants() in
LayoutMultiColumnFlowThread::populate(). This is because we have created a new
anonymous multi-col anonymous block and are moving all the container's children
to
it. Any floats nested within the children which are also contained by the
multi-col's ancestor will become unreachable later on if we attempt to delete
them because the new multi-col flowthread won't have them - and will act like a
firewall during the descent through the tree looking for the float to remove.

BUG=588178
==========

to

==========
Clear descendant floats when moving children of a multicol container to a
flowthread
We need to call multicolContainer->removeFloatingObjectsFromDescendants() in
LayoutMultiColumnFlowThread::populate(). This is because we have created a new
anonymous multi-col anonymous block and are moving all the container's children
to
it. Any floats nested within the children which are also contained by the
multi-col's
ancestor will become unreachable later on if we attempt to delete them because
the
new multi-col flowthread won't have them - and will act like a firewall during
the
descent through the tree looking for the float to remove.

BUG=588178
==========

[rhogan, 2016-03-01 20:23:37]

Description was changed from

==========
Clear descendant floats when moving children of a multicol container to a
flowthread
We need to call multicolContainer->removeFloatingObjectsFromDescendants() in
LayoutMultiColumnFlowThread::populate(). This is because we have created a new
anonymous multi-col anonymous block and are moving all the container's children
to
it. Any floats nested within the children which are also contained by the
multi-col's
ancestor will become unreachable later on if we attempt to delete them because
the
new multi-col flowthread won't have them - and will act like a firewall during
the
descent through the tree looking for the float to remove.

BUG=588178
==========

to

==========
Clear descendant floats when moving children of a multicol container to a
flowthread

We need to call multicolContainer->removeFloatingObjectsFromDescendants() in
LayoutMultiColumnFlowThread::populate(). This is because we can create a new
anonymous multi-col anonymous block and move all the container's children to
it. Any floats nested within the children which are also contained by the
multi-col's
ancestor will become unreachable later on if we attempt to delete them because
the
new multi-col flowthread won't have them - and will act like a firewall during
the
descent through the tree looking for the float to remove.

BUG=588178
==========

[rhogan, 2016-03-01 20:25:04]

Description was changed from

==========
Clear descendant floats when moving children of a multicol container to a
flowthread

We need to call multicolContainer->removeFloatingObjectsFromDescendants() in
LayoutMultiColumnFlowThread::populate(). This is because we can create a new
anonymous multi-col anonymous block and move all the container's children to
it. Any floats nested within the children which are also contained by the
multi-col's
ancestor will become unreachable later on if we attempt to delete them because
the
new multi-col flowthread won't have them - and will act like a firewall during
the
descent through the tree looking for the float to remove.

BUG=588178
==========

to

==========
Clear descendant floats when moving children of a multicol container to a
flowthread

We need to call multicolContainer->removeFloatingObjectsFromDescendants() in
LayoutMultiColumnFlowThread::populate(). This is because we can create a new
anonymous multi-col anonymous block and move all its container's children to
it. Any floats nested within the children which are also contained by the new
multi-col's
ancestor will become unreachable later on if we attempt to delete them because
the
new multi-col flowthread won't have them - and will act like a firewall during
the
descent through the tree looking for the float to remove.

BUG=588178
==========

[mstensho (USE GERRIT), 2016-03-02 10:54:07]

I reduced it to this:

<!DOCTYPE html>
<p>PASS if no crash or assertion failure.</p>
<div id="container">
    <div></div>
    <div id="block2">
        <textarea style="float:left; padding:20px;"></textarea>
        <textarea style="display:block;"></textarea>
    </div>
</div>
<script>
    document.body.offsetTop;
    document.getElementById("container").style.columns = "2";
    document.getElementById("block2").style.webkitWritingMode = "vertical-rl";
</script>

Then further, to this:

<!DOCTYPE html>
<p>PASS if no crash or assertion failure.</p>
<div id="container">
    <div id="child">
        <textarea style="float:left; padding:20px;"></textarea>
        <textarea style="display:block;"></textarea>
    </div>
</div>
<script>
    document.body.offsetTop;
    document.getElementById("container").style.overflow = "hidden";
    document.getElementById("child").style.webkitWritingMode = "vertical-rl";
</script>

No multicol! So it still crashes. But maybe I'm looking at a different problem.
I don't have an asan build at the moment, so I cannot see heap-use-after-free. I
get this assertion failure:

ASSERTION FAILED: interval.high() ==
m_layoutObject->logicalBottomForFloat(floatingObject)
../../third_party/WebKit/Source/core/layout/FloatingObjects.cpp(572) : void
blink::ComputeFloatOffsetAdapter<blink::FloatingObject::Type::FloatLeft>::collectIfNeeded(const
IntervalType &) [FloatTypeValue = blink::FloatingObject::Type::FloatLeft]

[mstensho (USE GERRIT), 2016-03-02 14:53:45]

OK, that assertion failure was something slightly different. Let's not worry
about that for now.

Here's a cleaned up version of the fuzzer test:

<!DOCTYPE html>
<p>PASS if no crash or assertion failure.</p>
<div id="container">
    <div></div>
    <div id="block2">
        <div id="removeMe" style="float:left; width:300px; height:300px;"></div>
        <textarea style="display:block;"></textarea>
    </div>
</div>
<script>
    document.body.offsetTop;
    document.getElementById("container").style.columns = "2";
    document.getElementById("block2").style.webkitWritingMode = "vertical-lr";
    document.getElementById("removeMe").style.display = "none";
</script>

I hope you can add it to the CL.

The thing I don't get when it comes to where (which LayoutBlockFlow objects) we
store FloatingObject instances for a float, is that sometimes we cross BFC
boundaries. In the test above, the float is stored in both HTML and BODY, even
after #container has become multicol, which does establish a new BFC. This is
not specific to multicol; it also happens when changing "overflow" from
"visible" to "hidden", which also injects a new BFC. It's especially bad for
multicol, though, since we have then inserted an anonymous flow thread, breaking
the chain of LayoutBlockFlow objects where the float is present.
LayoutBox::removeFloatingOrPositionedChildFromBlockLists() will go all the way
to the root, walking through fire, ice and over dead bodies (and BFCs). When it
has finally decided that parentBlockFlow needs to be that of the root element
(HTML) (I'd argue that it should have been something in the BFC where the float
actually lives), markAllDescendantsWithFloatsForLayout() will stop at the flow
thread, since it doesn't contain the float (while its descendants might) - just
like you say in the description.

FrameView::layoutOrthogonalWritingModeRoots() should probably be mentioned here
too, since it jumps into arbitrary places in the layout tree to lay out objects
directly. That should generally be legit, though.

Anyway, I'd claim that your code change just accidentally fixes the bug. There
ought to be some mechanism that removes irrelevant FloatingObject objects when a
new BFC is injected. Your change removes all floats, also those that might
rightfully belong in the flow thread, but it's fine, since they will be re-added
if necessary in the next layout pass.

Here's another test, where the float is on the outside of the object that
becomes multicol. Then it's more crucial to remove it inside the flow thread
(but, like I wrote above: there ought to be a generic mechanism for this -
multicol shouldn't have to worry), since it's suddenly in a separate BFC.

<!DOCTYPE html>
<div id="removeMe" style="float:left; width:100px; height:100px;"></div>
<div id="container">
    <!-- Sneak past removeFloatingObjectsFromDescendants() stuff in
         LayoutBoxModelObject::moveChildTo() during
LayoutMultiColumnFlowThread::populate(),
         by having a non-LayoutBlockFlow as the first child: -->
    <div style="display:table;"></div>
    <div>
        <div id="block2">
            <textarea id="block2" style="display:block;"></textarea>
        </div>
    </div>
</div>
<script>
    document.body.offsetTop;
    document.getElementById("container").style.columns = "2";
    getComputedStyle(document.body);
    document.getElementById("removeMe").style.display = "none";
    document.getElementById("block2").style.webkitWritingMode = "vertical-lr";
</script>

Hope you can add this test to the CL too.

lgtm

[rhogan, 2016-03-02 19:42:41]

On 2016/03/02 at 14:53:45, mstensho wrote:
> 
> Anyway, I'd claim that your code change just accidentally fixes the bug. There
ought to be some mechanism that removes irrelevant FloatingObject objects when a
new BFC is injected. Your change removes all floats, also those that might
rightfully belong in the flow thread, but it's fine, since they will be re-added
if necessary in the next layout pass.

Removing floats from all descendants *is* the mechanism. Once you insert a BFC
all bets are off on the floatlists below it (and even above, because some
descendant floats may have previously overhung from what is now the BFC and
therefore be tracked by its containers). The firewall introduced by multicol
feels multicol specific to me here, but would apply under any other mechanism
that can interpolate a wrapper object into a tree.

> 
> Hope you can add this test to the CL too.

Sure.

[mstensho (USE GERRIT), 2016-03-02 19:58:46]

On 2016/03/02 19:42:41, rhogan wrote:
> On 2016/03/02 at 14:53:45, mstensho wrote:
> > 
> > Anyway, I'd claim that your code change just accidentally fixes the bug.
There
> ought to be some mechanism that removes irrelevant FloatingObject objects when
a
> new BFC is injected. Your change removes all floats, also those that might
> rightfully belong in the flow thread, but it's fine, since they will be
re-added
> if necessary in the next layout pass.
> 
> Removing floats from all descendants *is* the mechanism. Once you insert a BFC
> all bets are off on the floatlists below it (and even above, because some
> descendant floats may have previously overhung from what is now the BFC and
> therefore be tracked by its containers).

That's what makes me uneasy. It should be safer to always confine floats to
their BFC, but updating that when a BFC is injected could be expensive, I
suppose.

> The firewall introduced by multicol
> feels multicol specific to me here, but would apply under any other mechanism
> that can interpolate a wrapper object into a tree.

Yes, the firewall thing seems rather multicol specific to me too, but leaving
bogus references to floats around isn't. :)

> > Hope you can add this test to the CL too.
> 
> Sure.

Thanks.

[rhogan, 2016-03-02 21:13:02]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-03-02 21:13:03]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1729663002/#ps20001
(title: "Updated")

[commit-bot: I haz the power, 2016-03-02 21:13:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1729663002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1729663002/20001

[commit-bot: I haz the power, 2016-03-02 22:44:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-02 22:45:43]

Description was changed from

==========
Clear descendant floats when moving children of a multicol container to a
flowthread

We need to call multicolContainer->removeFloatingObjectsFromDescendants() in
LayoutMultiColumnFlowThread::populate(). This is because we can create a new
anonymous multi-col anonymous block and move all its container's children to
it. Any floats nested within the children which are also contained by the new
multi-col's
ancestor will become unreachable later on if we attempt to delete them because
the
new multi-col flowthread won't have them - and will act like a firewall during
the
descent through the tree looking for the float to remove.

BUG=588178
==========

to

==========
Clear descendant floats when moving children of a multicol container to a
flowthread

We need to call multicolContainer->removeFloatingObjectsFromDescendants() in
LayoutMultiColumnFlowThread::populate(). This is because we can create a new
anonymous multi-col anonymous block and move all its container's children to
it. Any floats nested within the children which are also contained by the new
multi-col's
ancestor will become unreachable later on if we attempt to delete them because
the
new multi-col flowthread won't have them - and will act like a firewall during
the
descent through the tree looking for the float to remove.

BUG=588178

Committed: https://crrev.com/0ab8e6e1a527d322d9e93cba0949208bd2c55550
Cr-Commit-Position: refs/heads/master@{#378862}
==========

[commit-bot: I haz the power, 2016-03-02 22:45:44]

Patchset 2 (id:??) landed as
https://crrev.com/0ab8e6e1a527d322d9e93cba0949208bd2c55550
Cr-Commit-Position: refs/heads/master@{#378862}