179754 2017-11-16 01:34:31 -0800 firstLineBaseline called on non-laid out cell content during HTML table layout 2017-11-16 01:34:31 -0800 1 1 1 Unclassified WebKit Tables WebKit Nightly Build Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=174131 P2 Normal --- 1 fred.wang webkit-unassigned alex jfernandez oldest_to_newest 1372554 0 327053 fred.wang 2017-11-16 01:34:31 -0800 Created attachment 327053 Testcase This was originally reported in bug 174131 for RenderMathMLScripts::firstLineBaseline which is the only implementation asserting !needsLayout(). However, it turns out that this not specific to MathML, so I'm opening it here so that people more familiar with HTML table layout can comment. Actually, the use of firstLineBaseline in MathML is really an implementation detail inherited from the old flexbox-based approach and we might change it in the future, see for example bug 155879. Basically, RenderTableCell::layout() starts with the following line: int oldCellBaseline = cellBaselinePosition(); layoutBlock(cellWidthChanged) Then cellBaselinePosition() calls RenderBlockFlow::firstLineBaseline before the layout of the cell, which in turn may call RenderBlock::firstLineBaseline (if !childrenInline()) which will finally call firstLineBaseline on children. But these children are non laid out either. Here are the firstLineBaseline implementations: find -type f -name '*.h' | xargs grep firstLineBaseline ./RenderBox.h: virtual std::optional<int> firstLineBaseline() const { return std::optional<int>(); } ./RenderBlock.h: std::optional<int> firstLineBaseline() const override; ./RenderTableSection.h: std::optional<int> firstLineBaseline() const override; ./RenderTable.h: std::optional<int> firstLineBaseline() const override; ./RenderMenuList.h: std::optional<int> firstLineBaseline() const override { return RenderBlock::firstLineBaseline(); } ./RenderFlexibleBox.h: std::optional<int> firstLineBaseline() const override; ./RenderTextControl.h: std::optional<int> firstLineBaseline() const override { return RenderBlock::firstLineBaseline(); } ./RenderGrid.h: std::optional<int> firstLineBaseline() const final; ./RenderBlockFlow.h: std::optional<int> firstLineBaseline() const override; ./mathml/RenderMathMLOperator.h: std::optional<int> firstLineBaseline() const final; ./mathml/RenderMathMLRow.h: std::optional<int> firstLineBaseline() const override; ./mathml/RenderMathMLSpace.h: std::optional<int> firstLineBaseline() const final; ./mathml/RenderMathMLToken.h: std::optional<int> firstLineBaseline() const override; ./mathml/RenderMathMLFraction.h: std::optional<int> firstLineBaseline() const final; ./mathml/RenderMathMLScripts.h: std::optional<int> firstLineBaseline() const final; ./mathml/RenderMathMLPadded.h: std::optional<int> firstLineBaseline() const final; ./mathml/RenderMathMLBlock.h: return child.firstLineBaseline().value_or(child.logicalHeight()); ./mathml/RenderMathMLBlock.h: std::optional<int> firstLineBaseline() const final; The attached testcase will hit many of these functions with needsLayout() == false. The question is: is it OK to get firstLineBaseline on boxes that are non laid out yet? Maybe it's fine to return a dummy value for the initial "oldCellBaseline". Or maybe we should explicitly do an early return with a null optional (that's what happen for RenderFlexibleBox and RenderGrid implementations in the testcase) so that cellBaselinePosition() will use the fallback value instead of the wrong value? Maybe this special handling can just happen for RenderTableCell and all the other implementations should ASSERT(!needsLayout())? Any other idea? 327053 2017-11-16 01:34:31 -0800 2017-11-16 01:34:31 -0800 Testcase firstLineBaseline-before-layout.html text/html 3540 fred.wang PCFET0NUWVBFIGh0bWw+CjxodG1sPgogIDxoZWFkPgogICAgPHRpdGxlPmZpcnN0TGluZUJhc2Vs aW5lIGJlZm9yZSBsYXlvdXQ8L3RpdGxlPgogICAgPG1ldGEgY2hhcnNldD0idXRmLTgiLz4KICAg IDxzdHlsZT4KICAgICAgLmZsZXggewogICAgICAgIGRpc3BsYXk6IGZsZXg7CiAgICAgIH0KICAg ICAgLmdyaWQgewogICAgICAgIGRpc3BsYXk6IGdyaWQ7CiAgICAgICAgZ3JpZC10ZW1wbGF0ZS1j b2x1bW5zOiAxMDBweCAxMDBweCAxMDBweDsKICAgICAgICBncmlkLWdhcDogMTBweDsKICAgICAg ICBiYWNrZ3JvdW5kLWNvbG9yOiAjZmZmOwogICAgICAgIGNvbG9yOiAjNDQ0OwogICAgICB9CiAg ICAgIC5ib3ggewogICAgICAgIGJhY2tncm91bmQtY29sb3I6ICM0NDQ7CiAgICAgICAgY29sb3I6 ICNmZmY7CiAgICAgICAgYm9yZGVyLXJhZGl1czogNXB4OwogICAgICAgIHBhZGRpbmc6IDIwcHg7 CiAgICAgICAgZm9udC1zaXplOiAxNTAlOwogICAgICB9CiAgICA8L3N0eWxlPgogIDwvaGVhZD4K ICA8Ym9keT4KCiAgICA8cD5SZW5kZXJUYWJsZToKICAgICAgPHRhYmxlPgogICAgICAgIDx0cj4K ICAgICAgICAgIDx0ZD4KCSAgICA8dGFibGU+CiAgICAgICAgICAgICAgPHRyPjx0ZD5YPC90ZD48 L3RyPgogICAgICAgICAgICA8L3RhYmxlPgogICAgICAgICAgPC90ZD4KICAgICAgICA8L3RyPgog ICAgICA8L3RhYmxlPgogICAgPC9wPgogICAgCiAgICA8cD5SZW5kZXJGbGV4aWJsZUJveDoKICAg ICAgPHRhYmxlPgogICAgICAgIDx0cj4KICAgICAgICAgIDx0ZD4KICAgICAgICAgICAgPGRpdiBj bGFzcz0iZmxleCI+CgkgICAgICA8ZGl2IGNsYXNzPSJib3giPkE8L2Rpdj4KCSAgICAgIDxkaXYg Y2xhc3M9ImJveCI+QjwvZGl2PgoJICAgICAgPGRpdiBjbGFzcz0iYm94Ij5DPC9kaXY+CgkgICAg ICA8ZGl2IGNsYXNzPSJib3giPkQ8L2Rpdj4KCSAgICAgIDxkaXYgY2xhc3M9ImJveCI+RTwvZGl2 PgoJICAgICAgPGRpdiBjbGFzcz0iYm94Ij5GPC9kaXY+CiAgICAgICAgICAgIDwvZGl2PgogICAg ICAgICAgPC90ZD4KICAgICAgICA8L3RyPgogICAgICA8L3RhYmxlPgogICAgPC9wPgoKICAgIDxw PlJlbmRlckdyaWQ6CiAgICAgIDx0YWJsZT4KICAgICAgICA8dHI+CiAgICAgICAgICA8dGQ+Cgkg ICAgPGRpdiBjbGFzcz0iZ3JpZCI+CgkgICAgICA8ZGl2IGNsYXNzPSJib3giPkE8L2Rpdj4KCSAg ICAgIDxkaXYgY2xhc3M9ImJveCI+QjwvZGl2PgoJICAgICAgPGRpdiBjbGFzcz0iYm94Ij5DPC9k aXY+CgkgICAgICA8ZGl2IGNsYXNzPSJib3giPkQ8L2Rpdj4KCSAgICAgIDxkaXYgY2xhc3M9ImJv eCI+RTwvZGl2PgoJICAgICAgPGRpdiBjbGFzcz0iYm94Ij5GPC9kaXY+CgkgICAgPC9kaXY+CiAg ICAgICAgICA8L3RkPgogICAgICAgIDwvdHI+CiAgICAgIDwvdGFibGU+CiAgICA8L3A+CgogICAg PHA+UmVuZGVyTWF0aE1MT3BlcmF0b3I6CiAgICAgIDx0YWJsZT4KICAgICAgICA8dHI+CiAgICAg ICAgICA8dGQ+CiAgICAgICAgICAgIDxtYXRoIGRpc3BsYXk9ImJsb2NrIj4KICAgICAgICAgICAg ICA8bXRleHQ+QTwvbXRleHQ+CiAgICAgICAgICAgIDwvbWF0aD4KICAgICAgICAgIDwvdGQ+CiAg ICAgICAgPC90cj4KICAgICAgPC90YWJsZT4KICAgIDwvcD4KCiAgICA8cD5SZW5kZXJNYXRoTUxS b3c6CiAgICAgIDx0YWJsZT4KICAgICAgICA8dHI+CiAgICAgICAgICA8dGQ+CiAgICAgICAgICAg IDxtYXRoIGRpc3BsYXk9ImJsb2NrIj48L21hdGg+CiAgICAgICAgICA8L3RkPgogICAgICAgIDwv dHI+CiAgICAgIDwvdGFibGU+CiAgICA8L3A+CgogICAgPHA+UmVuZGVyTWF0aE1MU3BhY2U6CiAg ICAgIDx0YWJsZT4KICAgICAgICA8dHI+CiAgICAgICAgICA8dGQ+CiAgICAgICAgICAgIDxtYXRo IGRpc3BsYXk9ImJsb2NrIj4KICAgICAgICAgICAgICA8bXNwYWNlIGhlaWdodD0iMTBweCIgZGVw dGg9IjMwcHgiPjwvbXNwYWNlPgogICAgICAgICAgICA8L21hdGg+CiAgICAgICAgICA8L3RkPgog ICAgICAgIDwvdHI+CiAgICAgIDwvdGFibGU+CiAgICA8L3A+CiAgICAKICAgIDxwPlJlbmRlck1h dGhNTFRva2VuOgogICAgICA8dGFibGU+CiAgICAgICAgPHRyPgogICAgICAgICAgPHRkPgogICAg ICAgICAgICA8bWF0aCBkaXNwbGF5PSJibG9jayI+CiAgICAgICAgICAgICAgPG10ZXh0PkE8L210 ZXh0PgogICAgICAgICAgICA8L21hdGg+CiAgICAgICAgICA8L3RkPgogICAgICAgIDwvdHI+CiAg ICAgIDwvdGFibGU+CiAgICA8L3A+CgogICAgPHA+UmVuZGVyTWF0aE1MRnJhY3Rpb246CiAgICAg IDx0YWJsZT4KICAgICAgICA8dHI+CiAgICAgICAgICA8dGQ+CiAgICAgICAgICAgIDxtYXRoIGRp c3BsYXk9ImJsb2NrIj4KICAgICAgICAgICAgICA8bWZyYWM+CiAgICAgICAgICAgICAgICA8bWk+ QTwvbWk+CiAgICAgICAgICAgICAgICA8bWk+QjwvbWk+CiAgICAgICAgICAgICAgPC9tZnJhYz4K ICAgICAgICAgICAgPC9tYXRoPgogICAgICAgICAgPC90ZD4KICAgICAgICA8L3RyPgogICAgICA8 L3RhYmxlPgogICAgPC9wPgoKICAgIDxwPlJlbmRlck1hdGhNTFNjcmlwdHM6CiAgICAgIDx0YWJs ZT4KICAgICAgICA8dHI+CiAgICAgICAgICA8dGQ+CiAgICAgICAgICAgIDxtYXRoIGRpc3BsYXk9 ImJsb2NrIj4KICAgICAgICAgICAgICA8bXN1Yj4KICAgICAgICAgICAgICAgIDxtaT5BPC9taT4K ICAgICAgICAgICAgICAgIDxtaT5CPC9taT4KICAgICAgICAgICAgICA8L21zdWI+CiAgICAgICAg ICAgIDwvbWF0aD4KICAgICAgICAgIDwvdGQ+CiAgICAgICAgPC90cj4KICAgICAgPC90YWJsZT4K ICAgIDwvcD4KCiAgICA8cD5SZW5kZXJNYXRoTUxQYWRkZWQ6CiAgICAgIDx0YWJsZT4KICAgICAg ICA8dHI+CiAgICAgICAgICA8dGQ+CiAgICAgICAgICAgIDxtYXRoIGRpc3BsYXk9ImJsb2NrIj4K ICAgICAgICAgICAgICA8bXBhZGRlZCBoZWlnaHQ9IjEwcHgiIGRlcHRoPSIzMHB4Ij48L21wYWRk ZWQ+CiAgICAgICAgICAgIDwvbWF0aD4KICAgICAgICAgIDwvdGQ+CiAgICAgICAgPC90cj4KICAg ICAgPC90YWJsZT4KICAgIDwvcD4KCiAgICA8cD5SZW5kZXJNYXRoTUxCbG9jayAoaW52YWxpZCBt YXJrdXApOgogICAgICA8dGFibGU+CiAgICAgICAgPHRyPgogICAgICAgICAgPHRkPgogICAgICAg ICAgICA8bWF0aCBkaXNwbGF5PSJibG9jayI+CiAgICAgICAgICAgICAgPG1mcmFjPgogICAgICAg ICAgICAgICAgPG1pPng8L21pPgogICAgICAgICAgICAgIDwvbWZyYWM+CiAgICAgICAgICAgIDwv bWF0aD4KICAgICAgICAgIDwvdGQ+CiAgICAgICAgPC90cj4KICAgICAgPC90YWJsZT4KICAgIDwv cD4KCiAgICA8cD5SZW5kZXJNYXRoTUxUYWJsZToKICAgICAgPHRhYmxlPgogICAgICAgIDx0cj4K ICAgICAgICAgIDx0ZD4KICAgICAgICAgICAgPG1hdGggZGlzcGxheT0iYmxvY2siPgogICAgICAg ICAgICAgIDxtdGFibGU+PC9tdGFibGU+CiAgICAgICAgICAgIDwvbWF0aD4KICAgICAgICAgIDwv dGQ+CiAgICAgICAgPC90cj4KICAgICAgPC90YWJsZT4KICAgIDwvcD4KCiAgPC9ib2R5Pgo8L2h0 bWw+CgoK