Bug 234654 – null ptr deref in WebCore::LayoutIntegration::LineLayout::collectOverflow()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Gabriel Nava Marino]

null ptr deref in WebCore::LayoutIntegration::LineLayout::collectOverflow()

[Gabriel Nava Marino]

<rdar://problem/86571571>

[Gabriel Nava Marino]

Created attachment 447915 [details]
Patch

[Gabriel Nava Marino]

Created attachment 447920 [details]
Patch

[Gabriel Nava Marino]

Created attachment 447929 [details]
Patch

[zalan]

Normally preferred width computation is followed by normal flow layout (e.g. IFC) which constructs the InlineContent for painting/hittesting/overflow etc.
However in this case while the block is shrink-fit sizing (and we use IFC to compute its preferred width), it is also candidate to simplified layout which does not call IFC at all (it assumes 1, the content is already laid out (not dirty), or its content does not need normal flow layout).
What happens is
1. we construct the LineLayout object at tryComputePreferredWidthsUsingModernPath (this is the preferred width computation codepath)
2. but since we don't run layout through the normal codepath, we don't generate any InlineContent and
3. later (still in the simplified layout codepath) addOverflowFromInlineChildren() uses the LineLayout object (constructed at the preferred width codepath) to retrieve the (never computed) geometry.

While the patch fixes the crash, it does not address the asymmetric behavior explained above (e.g how we end up in the simplified layout codepath when the block container clearly has a text renderer and we even run preferred width for it).

[zalan]

B---YGLSC -+  RenderView at (0,0) size 1356x743 renderer->(0x16c00cff0) layout->[positioned child]
B-----LS- --    HTML RenderBlock at (0,0) size 1356x0 renderer->(0x16c0084d0) node->(0x16c014140)
BA----L-- -+      BODY RenderBody at (8,8) size 7.11x18 renderer->(0x16c0085f0) node->(0x16c0144a0) layout->[normal child][simplified]
B-----L-C --        DIV RenderBlock at (0,0) size 7.11x0 renderer->(0x16c008e90) node->(0x16c014d80)
B-------- --          TABLE RenderTable at (0,0) size 0x0 renderer->(0x16c0090d0) node->(0x16c014eb0)
BA----L-- --          IMG RenderImage at (0,0) size 0x0 renderer->(0x16c008d70) node->(0x16c009290)
B-----L-C -+*       DIV RenderBlock at (0,0) size 7.11x18 renderer->(0x16c008a10) node->(0x16c014c60) layout->[normal child][positioned child]
I-------- -+          BR RenderBR renderer->(0x16c015620) node->(0x16c015470) layout->[self]
I-------- --          #text RenderText renderer->(0x16c011020) node->(0x16c010dd0) length->(1) "a"
B-----L-C --        DIV RenderBlock at (0,18) size 7.11x0 renderer->(0x16c008b30) node->(0x16c014cf0)

1. The out-of-flow positioned <br> triggers a setPosChildNeedsLayoutBit() on its containing block (DIV). It tells the <div>(0x16c008a10) that it has a positioned child and it needs a layout.
2. This <br> then gets removed from the tree (RenderObject::willBeRemovedFromTree 0x16c015620)
3. At this point we should probably adjust the dirty layout bit but we don't.
4. When we finally run layout, all we see is a (clean) RenderText so we (incorrectly) end up choosing the simplified layout codepath.

B---YGLSC -+  RenderView at (0,0) size 1356x743 renderer->(0x16c00cff0) layout->[self][positioned child]
B-----LS- --    HTML RenderBlock at (0,0) size 1356x0 renderer->(0x16c0084d0) node->(0x16c014140)
BA----L-- -+      BODY RenderBody at (8,8) size 7.11x36 renderer->(0x16c0085f0) node->(0x16c0144a0) layout->[normal child][simplified]
B-----L-C --        DIV RenderBlock at (0,0) size 7.11x0 renderer->(0x16c008e90) node->(0x16c014d80)
B-------- --          TABLE RenderTable at (0,0) size 0x0 renderer->(0x16c0090d0) node->(0x16c014eb0)
BA----L-- --          IMG RenderImage at (0,0) size 0x0 renderer->(0x16c008d70) node->(0x16c009290)
B-----L-C -+*       DIV RenderBlock at (0,0) size 7.11x36 renderer->(0x16c008a10) node->(0x16c014c60) layout->[positioned child]
I-------- --          #text RenderText renderer->(0x16c011020) node->(0x16c010dd0) length->(1) "a"
B-----L-C --        DIV RenderBlock at (0,36) size 7.11x0 renderer->(0x16c008b30) node->(0x16c014cf0)

[zalan]

ok, so while having the posChildNeedsLayout bit set on the block is misleading since the block does not even have a positioned child (anymore), it is _technically_ correct. It is correct because removing out-of-flow boxes does not impact the inflow content (ie the rest of of the content still has valid (clean) geometry) so the posChildNeedsLayout bit simply means that we don't need to run layout on the inflow boxes, instead take the simplified layout codepath and recompute overflow.
However, when the out-of-flow box is removed, we also invalidate the line layout path on the block (*first bug) which means that all the associated geometry information is thrown away. In order to recover it, we mark the block renderer dirty by calling setNeedsLayout() unless the renderer is already dirty (see RenderBlockFlow::invalidateLineLayoutPath). 
...
        m_lineLayout = std::monostate();
        setLineLayoutPath(path);
        if (selfNeedsLayout())
            return;
        setNeedsLayout();
...
Sadly this needsLayout() check is insufficient for modern line layout.
m_lineLayout = std::monostate() does not only destroy the line layout object but it also nukes all the IFC geometries. It is equivalent to having all the child boxes dirty, since in order to re-generate the geometry information, we have to layout _all_ the boxes (while nuking the legacy line layout object does not destroy the inline tree -where we store the geometry information). The (second) bug is that needsLayout() returns true for cases (e.g. posChildNeedsLayout) when while the geometry is all gone, we are going to take a special layout codepath which expects pre-computed geometries. 

We have a few options here 
1. do not invalidate the line layout path when the out-of-flow box is removed (since it does not impact the inflow content) -first bug
2. decouple the IFC line layout object and the IFC geometries (like legacy line layout has)
2. check against selfNeedsLayout() instead. -second bug

fix ->
@@ -3712,7 +3712,7 @@ void RenderBlockFlow::invalidateLineLayoutPath()
 #endif
         m_lineLayout = std::monostate();
         setLineLayoutPath(path);
-        if (needsLayout())
+        if (selfNeedsLayout())
             return;
         // FIXME: We should just kick off a subtree layout here (if needed at all) see webkit.org/b/172947.
         setNeedsLayout();

[zalan]

Created attachment 447963 [details]
Patch

[zalan]

The missing 'a' in the -expected.txt (previous patch) also an indication that while the crash is gone, the layout is still in an invalid state ('a' has no computed geometry). It shows up now.

[zalan]

Comment on attachment 447963 [details]
Patch

Apparently in some cases we invalidate the line layout path _during_ layout and we can't just setNeedsLayout the ancestor chain.

[zalan]

Created attachment 447966 [details]
Patch

[EWS]

Committed r287867 (245911@main): <https://commits.webkit.org/245911@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447966 [details].