Skip to content

GitLab

T
tangled
Commit 17766b88 authored by Morten Stenshorne's avatar Morten Stenshorne Committed by Commit Bot
Browse files
Options

Line boxes hold potentially dangling pointers to FloatingObject. 

And that's how it's supposed to work, apparently. We're just supposed to mark
such lines dirty when the objects get deleted, and the dangling pointers will
be cleaned up "soon enough".

When removing a FloatingObject, we need to mark the RootInlineBox object that
points to it as dirty. The case we were missing was the one where we remove ALL
floating objects. When removing just one, on the other hand, we use
LayoutBlockFlow::RemoveFloatingObject(), which already takes care of this.

Bug: 835613
Change-Id: I2b978f1bb4c9a496a5dad4b097ef598630059a9d
Reviewed-on: https://chromium-review.googlesource.com/1108936
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: default avatarEmil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569223}
parent 8a119eb7
Hide whitespace changes
Inline Side-by-side
Showing with 68 additions and 2 deletions
third_party/WebKit/LayoutTests/fast/block/float/remove-float-after-anonymous-block-insert-crash.html 0 → 100644
View file @ 17766b88
<!DOCTYPE html>
<div id="container">
<span id="inlineBlock" style="display:inline-block; width:50px;">
<div>
<br>
<span style="padding-left:30px;"><span style="padding-left:30px;"><div id="victim" style="float:left;"></div>
</span></span>
</div>
</span>
<div id="intruder" style="display:none;"></div>
</div>
<script src="../../../resources/testharness.js"></script>
<script src="../../../resources/testharnessreport.js"></script>
<script>
test(() => {
// The comments here assume that crbug.com/835613 is unfixed.
// Trigger layout.
document.body.offsetTop;
// Make two specific tree changes in the same style
// update. Inserting #intruder will require #container to have block
// children (it had inline children up until this point). It will
// have to insert an anonymous block around #inlineBlock. As part of
// inserting an anonymous block, all the children that get wrapped
// inside it will have their FloatingObjects cleared. The code that
// does this is in LayoutBoxModelObject::MoveChildTo(). It calls
// RemoveFloatingObjectsFromDescendants() (although that's really
// there to cope with a completely different scenario than this, and
// it's actually pretty unnecessary to clear the float in this case
// (the float is even inside a new formatting context)). Anyway, the
// bug is that we forget about RootInlineBox objects that may hold
// pointers to the FloatingObjects that it's removing.
//
// Then #intruder can be inserted after the new anonymous block.
intruder.style.display = "block";
// When we move on and remove #victim (the float), it will attempt
// to remove its FloatingObject(s).
// LayoutBlockFlow::MarkAllDescendantsWithFloatsForLayout() will
// call RemoveFloatingObject() to look for the FloatingObject, but
// it's already gone! So we won't get to the part where we'd
// otherwise mark the line that contains the FloatingObject dirty.
// It requires a rather special tree [1] to actually trigger a
// read-after-free, though.
//
// [1] The ex-float must be adjacent to an inline that's inside
// another inline, and the sum of the inline-start border/padding of
// those inlines must be larger than the inline size of the
// containing block. Aaand we need a line in front of all this
// (hence the BR).
victim.style.display = "none";
}, "PASS if no assertion failure or heap-use-after-free");
</script>
third_party/blink/renderer/core/layout/layout_block_flow.cc
View file @ 17766b88
...@@ -2741,9 +2741,21 @@ void LayoutBlockFlow::RemoveFloatingObjectsFromDescendants() { ...@@ -2741,9 +2741,21 @@ void LayoutBlockFlow::RemoveFloatingObjectsFromDescendants() {
// If our children are inline, then the only boxes which could contain floats // If our children are inline, then the only boxes which could contain floats
// are atomic inlines (e.g. inline-block, float etc.) and these create // are atomic inlines (e.g. inline-block, float etc.) and these create
// formatting contexts, so can't pick up intruding floats from // formatting contexts, so can't pick up intruding floats from
// ancestors/siblings - making them safe to skip. // ancestors/siblings - making them safe to skip. We do need to examine the
if (ChildrenInline()) // lines, though, as there may be pointers to the any of the objects that we
// are going to remove. Mark those lines dirty, to avoid accessing dangling
// pointers. Also, yikes!
if (ChildrenInline()) {
for (auto* line = FirstRootBox(); line; line = line->NextRootBox()) {
if (!line->IsDirty()) {
if (const auto* floats = line->FloatsPtr()) {
if (floats->size())
line->MarkDirty();
}
}
}
return; return;
}
for (LayoutObject* child = FirstChild(); child; for (LayoutObject* child = FirstChild(); child;
child = child->NextSibling()) { child = child->NextSibling()) {
// We don't skip blocks that create formatting contexts as they may have // We don't skip blocks that create formatting contexts as they may have
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment