Don't add %-height descendants outside the containing block chain.

A video element creates some children (flexboxes, etc.), and the video ought to be their containing "block" for all purposes, but LayoutVideo is not a LayoutBlock (it's LayoutReplaced), so it doesn't really work. Make sure that we don't skip upwards and escape from the innards of the video element and start adding percentage descendants to blocks that are not in the containing block chain of the video (the video may be out-of-flow positioned). Bug: 965032 Change-Id: Ic9fdb7b9dcff5724b9488d85dfafc9c87f4bee62 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1622806 Reviewed-by: Emil A Eklund <eae@chromium.org> Commit-Queue: Christian Biesinger <cbiesinger@chromium.org> Cr-Commit-Position: refs/heads/master@{#662821}
Denger-Network · May 23, 2019 · 39402a8 · 39402a8
1 parent 764fc6e
commit 39402a8
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 1 deletion.
diff --git a/third_party/blink/renderer/core/layout/layout_block.cc b/third_party/blink/renderer/core/layout/layout_block.cc
@@ -1114,6 +1114,15 @@ void LayoutBlock::RemovePositionedObjects(
 }
 
 void LayoutBlock::AddPercentHeightDescendant(LayoutBox* descendant) {
+  // A replaced object is incapable of properly acting as a containing block for
+  // its children (this is an issue with VIDEO elements, for instance, which
+  // inserts some percentage height flexbox children). Assert that the
+  // descendant hasn't escaped from within a replaced object. Registering the
+  // percentage height descendant further up in the tree is only going to cause
+  // trouble, especially if the replaced object is out-of-flow positioned (and
+  // we failed to notice).
+  DCHECK(!descendant->Container()->IsLayoutReplaced());
+
   if (descendant->PercentHeightContainer()) {
     if (descendant->PercentHeightContainer() == this) {
       DCHECK(HasPercentHeightDescendant(descendant));

diff --git a/third_party/blink/renderer/core/layout/layout_box.cc b/third_party/blink/renderer/core/layout/layout_box.cc
@@ -3796,7 +3796,13 @@ LayoutUnit LayoutBox::ComputePercentageLogicalHeight(
           &cb, &skipped_auto_height_containing_block);
 
   DCHECK(cb);
-  cb->AddPercentHeightDescendant(const_cast<LayoutBox*>(this));
+
+  // If the container of the descendant is a replaced element (a VIDEO, for
+  // instance), |cb| (which uses ContainingBlock()) may actually not be in the
+  // containing block chain for the descendant.
+  const LayoutObject* container = Container();
+  if (!container->IsLayoutReplaced())
+    cb->AddPercentHeightDescendant(const_cast<LayoutBox*>(this));
 
   if (available_height == -1)
     return available_height;

diff --git a/...d_party/blink/web_tests/fast/replaced/abspos-percentage-height-video-inline-fc-crash.html b/...d_party/blink/web_tests/fast/replaced/abspos-percentage-height-video-inline-fc-crash.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<div contenteditable></div> <!-- Trigger legacy layout for the entire document. -->
+<div id="container" style="position:relative; width:110px;">
+  <div style="float:left; width:100px; height:100px;">
+    <div></div>
+    <video style="position:absolute; height:1%;"></video>
+  </div>
+</div>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<script>
+  test(()=> {
+    document.documentElement.offsetTop;
+    container.style.width = "111px";
+  }, "did not crash");
+</script>