Don't skip a containing VIDEO element when marking for layout.

We have special-code in LayoutObject::Container() for legacy floats. Prevent this from kicking in if it's e.g. a VIDEO element that contains a float ("Wait, what?" you say? Indeed. Wait, what.). LayoutVideo isn't LayoutBlock, so ContainingBlock() would skip it. Another option would be to forbid floated ::-webkit-media-controls, but since this is just about disabling code meant for legacy inline formatting contexts, this fix seems reasonable. Besides, if we were to forbid stuff, we should probably forbid a lot of things, such as out-of-flow positioning, etc. Additionally, skip fragments in pre-paint if IsLayoutObjectDestroyedOrMoved(), just like we already do everywhere else when walking the fragment tree, to prevent such crashers in the future. The test included only failed with DCHECKs enabled (AssertLaidOut()). We don't reach the nullptr deref in pre-paint. I couldn't come up with an automatic testcase to trigger this (unless we set a long timeout). Bug: 1308042, 1308811 Change-Id: I77f75bda23573c623dbd91d50c8dfc7564d5f06a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3545149 Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org> Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Cr-Commit-Position: refs/heads/main@{#984768}
chromium · Mar 24, 2022 · 5ce0c11 · 5ce0c11
1 parent 439ac93
commit 5ce0c11
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 2 deletions.
diff --git a/third_party/blink/renderer/core/layout/layout_object.cc b/third_party/blink/renderer/core/layout/layout_object.cc
@@ -3722,8 +3722,24 @@ LayoutObject* LayoutObject::Container(AncestorSkipInfo* skip_info) const {
     return multicol_container;
   }
 
-  if (IsFloating() && !IsInLayoutNGInlineFormattingContext())
-    return ContainingBlock(skip_info);
+  if (IsFloating() && !IsInLayoutNGInlineFormattingContext()) {
+    // TODO(crbug.com/1229581): Remove this when removing support for legacy
+    // layout.
+    //
+    // In the legacy engine, floats inside non-atomic inlines belong to their
+    // nearest containing block, not the parent non-atomic inline (if any). Skip
+    // past all non-atomic inlines. Note that the reason for not simply using
+    // ContainingBlock() here is that we want to stop at any kind of LayoutBox,
+    // such as LayoutVideo. Otherwise we won't mark the container chain
+    // correctly when marking for re-layout.
+    LayoutObject* walker = Parent();
+    while (walker && walker->IsLayoutInline()) {
+      if (skip_info)
+        skip_info->Update(*walker);
+      walker = walker->Parent();
+    }
+    return walker;
+  }
 
   return Parent();
 }

diff --git a/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc b/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc
@@ -556,6 +556,8 @@ bool PrePaintTreeWalk::CollectMissableChildren(
     const NGPhysicalBoxFragment& parent) {
   bool has_missable_children = false;
   for (const NGLink& child : parent.Children()) {
+    if (UNLIKELY(child->IsLayoutObjectDestroyedOrMoved()))
+      continue;
     if ((child->IsOutOfFlowPositioned() &&
          (context.current_fragmentainer.fragment ||
           child->IsFixedPositioned())) ||
@@ -619,6 +621,8 @@ void PrePaintTreeWalk::WalkMissedChildren(
     return;
 
   for (const NGLink& child : fragment.Children()) {
+    if (UNLIKELY(child->IsLayoutObjectDestroyedOrMoved()))
+      continue;
     if (!child->IsOutOfFlowPositioned() && !child->IsFloating())
       continue;
     if (!pending_missables_.Contains(child.fragment))

diff --git a/third_party/blink/web_tests/media/video-floated-controls-crash.html b/third_party/blink/web_tests/media/video-floated-controls-crash.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<video width="200" height="200" controls src="whatever"></video>
+<style></style>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+<script>
+  test(()=> {
+    document.body.offsetTop;
+    var sheet = document.styleSheets[0];
+    sheet.insertRule("::-webkit-media-controls { float: left; }");
+
+    /* Trigger re-layout of something on the inside of the video AND something
+       on the outside. */
+    document.body.offsetTop;
+    sheet.insertRule("::-webkit-media-controls-enclosure { width: 333px; }");
+    document.body.style.width = "333px";
+  }, "No crash");
+</script>