Bug 249942 - [IFC] Infinite recursion in Layout::LineBoxVerticalAligner::layoutBoundsForInlineBoxSubtree
Summary: [IFC] Infinite recursion in Layout::LineBoxVerticalAligner::layoutBoundsForIn...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-12-29 07:55 PST by Michael Catanzaro
Modified: 2023-01-15 11:08 PST (History)
6 users (show)

See Also:


Attachments
Render dump (457.85 KB, text/plain)
2023-01-13 15:22 PST, Michael Catanzaro
no flags Details
Test case (198 bytes, text/html)
2023-01-14 19:43 PST, zalan
no flags Details
Patch (10.98 KB, patch)
2023-01-15 07:38 PST, zalan
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2022-12-29 07:55:27 PST
I found some infinite recursion in WebCore::Layout::LineBoxVerticalAligner::layoutBoundsForInlineBoxSubtree leading to a stack overflow. Unfortunately I do not have a shareable reproducer, but the backtrace for the crash is pretty straightforward, so perhaps it won't be needed?

#0  0x00007fbff9fe6ab8 in WebCore::Layout::InlineFormattingGeometry::inlineLevelBoxAffectsLineBox(WebCore::Layout::InlineLevelBox const&) const (this=this@entry=0x7ffd64ff3f88, inlineLevelBox=...)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebCore/layout/formattingContexts/inline/InlineFormattingGeometry.cpp:123
#1  0x00007fbff9ff52c0 in WebCore::Layout::LineBoxVerticalAligner::layoutBoundsForInlineBoxSubtree(WebCore::Layout::LineBox const&, WebCore::Layout::InlineLevelBox const&) const
    (this=0x7ffd64ff3f80, lineBox=..., inlineBox=<optimized out>)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebCore/layout/formattingContexts/inline/InlineLineBoxVerticalAligner.cpp:386
#2  0x00007fbff9ff530e in WebCore::Layout::LineBoxVerticalAligner::layoutBoundsForInlineBoxSubtree(WebCore::Layout::LineBox const&, WebCore::Layout::InlineLevelBox const&) const
    (this=0x7ffd64ff3f80, lineBox=..., inlineBox=<optimized out>)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebCore/layout/formattingContexts/inline/InlineLineBoxVerticalAligner.cpp:391
#3  0x00007fbff9ff530e in WebCore::Layout::LineBoxVerticalAligner::layoutBoundsForInlineBoxSubtree(WebCore::Layout::LineBox const&, WebCore::Layout::InlineLevelBox const&) const
    (this=0x7ffd64ff3f80, lineBox=..., inlineBox=<optimized out>)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebCore/layout/formattingContexts/inline/InlineLineBoxVerticalAligner.cpp:391
#4  0x00007fbff9ff530e in WebCore::Layout::LineBoxVerticalAligner::layoutBoundsForInlineBoxSubtree(WebCore::Layout::LineBox const&, WebCore::Layout::InlineLevelBox const&) const
    (this=0x7ffd64ff3f80, lineBox=..., inlineBox=<optimized out>)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebCore/layout/formattingContexts/inline/InlineLineBoxVerticalAligner.cpp:391
#5  0x00007fbff9ff530e in WebCore::Layout::LineBoxVerticalAligner::layoutBoundsForInlineBoxSubtree(WebCore::Layout::LineBox const&, WebCore::Layout::InlineLevelBox const&) const
    (this=0x7ffd64ff3f80, lineBox=..., inlineBox=<optimized out>)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebCore/layout/formattingContexts/inline/InlineLineBoxVerticalAligner.cpp:391

That repeats for at least a thousand more frames before I got bored and hit Ctrl+C. Here are the top frames from a different slightly backtrace (just the first two frames are different here):

#0  0x00007fe548bf516f in WebCore::Layout::Traversal::firstChild<WebCore::Layout::ElementBox, WebCore::Layout::ElementBox const>(WebCore::Layout::ElementBox const&) (current=<optimized out>)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebCore/layout/layouttree/LayoutIterator.h:106
        object = 0x7fe483a13e40
        childLayoutBox = <optimized out>

                    formattingGeometry = @0x7fff7d438e68: {<WebCore::Layout::FormattingGeometry> = {m_formattingContext = @0x7fff7d439c60}, <No data fields>}
        enclosingLayoutBounds = {ascent = 0, descent = 0}
#1  WebCore::Layout::LayoutChildIteratorAdapter<WebCore::Layout::ElementBox>::begin() const (this=<optimized out>)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebCore/layout/layouttree/LayoutChildIterator.h:87
        childLayoutBox = <optimized out>

                    formattingGeometry = @0x7fff7d438e68: {<WebCore::Layout::FormattingGeometry> = {m_formattingContext = @0x7fff7d439c60}, <No data fields>}
        enclosingLayoutBounds = {ascent = 0, descent = 0}
#2  WebCore::Layout::LineBoxVerticalAligner::layoutBoundsForInlineBoxSubtree(WebCore::Layout::LineBox const&, WebCore::Layout::InlineLevelBox const&) const (this=0x7fff7d438e60, lineBox=..., inlineBox=...)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebCore/layout/formattingContexts/inline/InlineLineBoxVerticalAligner.cpp:382
        childLayoutBox = <optimized out>

                    formattingGeometry = @0x7fff7d438e68: {<WebCore::Layout::FormattingGeometry> = {m_formattingContext = @0x7fff7d439c60}, <No data fields>}
        enclosingLayoutBounds = {ascent = 0, descent = 0}
#3  0x00007fe548bf530e in WebCore::Layout::LineBoxVerticalAligner::layoutBoundsForInlineBoxSubtree(WebCore::Layout::LineBox const&, WebCore::Layout::InlineLevelBox const&) const
    (this=0x7fff7d438e60, lineBox=..., inlineBox=<optimized out>)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebCore/layout/formattingContexts/inline/InlineLineBoxVerticalAligner.cpp:391

                  inlineLevelBox = @0x7fe458463380: {m_layoutBox = {m_ptr = 0x7fe483a13d80}, m_logicalRect = {m_rect = {m_location = {m_x = 0, m_y = 0}, m_size = {m_width = 198.339783, m_height = 14}}}, m_layoutBounds = std::optional<WebCore::Layout::InlineLevelBox::LayoutBounds> = {[contained value] = {ascent = 11, descent = 3}}, m_inlineBoxContentOffsetForLeadingTrim = 0, m_ascent = 11, m_descent = std::optional<float> = {[contained value] = 3}, m_hasContent = false, m_isFirstWithinLayoutBox = true, m_isLastWithinLayoutBox = false, m_type = WebCore::Layout::InlineLevelBox::Type::InlineBox, m_style = {primaryFontMetrics = @0x7fe4dec26c04, lineHeight = @0x7fe44efd541c, textEdge = {over = WebCore::TextEdgeType::Leading, under = WebCore::TextEdgeType::Leading}, leadingTrim = WebCore::LeadingTrim::Normal, lineBoxContain = {m_storage = 19}, primaryFontSize = 12, verticalAlignment = {type = WebCore::VerticalAlign::Baseline, baselineOffset = std::optional<float> [no contained value]}}, m_annotation = std::optional<WebCore::Layout::InlineLevelBox::Annotation> [no contained value]}
        ascent = <optimized out>
        descent = <optimized out>

                    childLayoutBox = @0x7fe483a203c0: {<WebCore::Layout::Box> = {<WTF::CanMakeCheckedPtrBase<WTF::SingleThreadIntegralWrapper<unsigned int>, unsigned int>> = {m_count = {m_value = 2}}, _vptr.Box = 0x7fe54a8a8fe0 <vtable for WebCore::Layout::ElementBox+16>, m_style = {static allTransformOperations = {m_storage = 31 '\037'}, static individualTransformOperations = <same as static member of an already seen type>, m_boxData = {m_data = {m_ptr = 0x7fe447bc36a0}}, m_visualData = {m_data = {m_ptr = 0x7fe5260f3870}}, m_backgroundData = {m_data = {m_ptr = 0x7fe526015240}}, m_surroundData = {m_data = {m_ptr = 0x7fe526074830}}, m_rareNonInheritedData = {m_data = {m_ptr = 0x7fe52602d440}}, m_nonInheritedFlags = {effectiveDisplay = 0, originalDisplay = 0, overflowX = 0, overflowY = 0, verticalAlign = 0, clear = 0, position = 0, unicodeBidi = 0, floating = 0, tableLayout = 0, hasExplicitlySetBorderBottomLeftRadius = 0, hasExplicitlySetBorderBottomRightRadius = 0, hasExplicitlySetBorderTopLeftRadius = 0, hasExplicitlySetBorderTopRightRadius = 0, hasExplicitlySetDirection = 0, hasExplicitlySetWritingMode = 0, hasExplicitlySetColorScheme = 0, usesViewportUnits = 0, usesContainerUnits = 0,--Type <RET> for more, q to quit, c to continue without paging--
 hasExplicitlyInheritedProperties = 0, disallowsFastPathInheritance = 0, isUnique = 0, emptyState = 0, firstChildState = 0, lastChildState = 0, isLink = 0, hasContentNone = 0, styleType = 0, pseudoBits = 136}, m_rareInheritedData = {m_data = {m_ptr = 0x7fe4641c8b60}}, m_inheritedData = {m_data = {m_ptr = 0x7fe44efd5410}}, m_inheritedFlags = {emptyCells = 0, captionSide = 0, listStyleType = 0, listStylePosition = 0, visibility = 0, textAlign = 0, textTransform = 3, textDecorationLines = 0, cursor = 0, cursorVisibility = 0, direction = 0, whiteSpace = 0, borderCollapse = 0, boxDirection = 0, rtlOrdering = 0, printColorAdjust = 0, pointerEvents = 1, insideLink = 0, insideDefaultButton = 0, writingMode = 0}, m_cachedPseudoStyles = std::unique_ptr<WTF::Vector<std::unique_ptr<WebCore::RenderStyle, std::default_delete<WebCore::RenderStyle> >, 4, WTF::CrashOnOverflow, 16, WTF::FastMalloc>> = {get() = 0x0}, m_svgStyle = {m_data = {m_ptr = 0x7fe526035df0}}}, m_nodeType = WebCore::Layout::Box::NodeType::GenericElement, m_isAnonymous = false, m_baseTypeFlags = 2, m_hasRareData = false, m_isInlineIntegrationRoot = false, m_isFirstChildForIntegration = false, m_parent = {m_ptr = 0x7fe483a13d80}, m_nextSibling = std::unique_ptr<WebCore::Layout::Box> = {get() = 0x0}, m_previousSibling = {m_ptr = 0x7fe483a13e40}, m_cachedLayoutState = {m_impl = {m_ptr = 0x7fe464460e80}}, m_cachedGeometryForLayoutState = std::unique_ptr<WebCore::Layout::BoxGeometry> = {get() = 0x7fe483a15760}}, m_firstChild = std::unique_ptr<WebCore::Layout::Box> = {get() = 0x0}, m_lastChild = {m_ptr = 0x0}, m_baselineForIntegration = std::optional<WebCore::LayoutUnit> [no contained value], m_replacedData = std::unique_ptr<WebCore::Layout::ElementBox::ReplacedData> = {get() = 0x0}}

                    formattingGeometry = @0x7fff7d438e68: {<WebCore::Layout::FormattingGeometry> = {m_formattingContext = @0x7fff7d439c60}, <No data fields>}
        enclosingLayoutBounds = {ascent = <optimized out>, descent = <optimized out>}

I can modify WebKit with extra WTFLogAlways or other debugging if desired.
Comment 1 Radar WebKit Bug Importer 2022-12-29 07:55:39 PST
<rdar://problem/103760798>
Comment 2 zalan 2022-12-29 08:19:18 PST
Thanks, will take a look. Attaching the render tree dump would help a lot. Is that something you could share?
Comment 3 Michael Catanzaro 2022-12-29 08:28:00 PST
(In reply to zalan from comment #2)
> Thanks, will take a look. Attaching the render tree dump would help a lot.

What's the easiest way to get that?

> Is that something you could share?

Not the whole thing, but *maybe* I'll be able to reduce the HTML input down to something shareable. I've never been good at this, though....
Comment 4 zalan 2022-12-29 08:33:06 PST
(In reply to Michael Catanzaro from comment #3)
> (In reply to zalan from comment #2)
> > Thanks, will take a look. Attaching the render tree dump would help a lot.
> 
> What's the easiest way to get that?
> 
> > Is that something you could share?
> 
> Not the whole thing, but *maybe* I'll be able to reduce the HTML input down
> to something shareable. I've never been good at this, though....
the easiest would be to 
(In reply to Michael Catanzaro from comment #3)
> (In reply to zalan from comment #2)
> > Thanks, will take a look. Attaching the render tree dump would help a lot.
> 
> What's the easiest way to get that?

diff --git a/Source/WebCore/page/FrameViewLayoutContext.cpp b/Source/WebCore/page/FrameViewLayoutContext.cpp
index 36c600c48c7c..c5d7105c9c1b 100644
--- a/Source/WebCore/page/FrameViewLayoutContext.cpp
+++ b/Source/WebCore/page/FrameViewLayoutContext.cpp
@@ -247,7 +247,11 @@ void FrameViewLayoutContext::performLayout()
 #ifndef NDEBUG
         RenderTreeNeedsLayoutChecker checker(*layoutRoot);
 #endif
+        WTFLogAlways("before");
+        showRenderTree(layoutRoot.get());
         layoutRoot->layout();
+        WTFLogAlways("after");
+        showRenderTree(layoutRoot.get());
         layoutUsingFormattingContext();
         ++m_layoutCount;
 #if ENABLE(TEXT_AUTOSIZING)

This needs ENABLE(TREE_DEBUGGING) which in on by default in DEBUG.

> > Is that something you could share?
> 
> Not the whole thing, but *maybe* I'll be able to reduce the HTML input down
> to something shareable. I've never been good at this, though....
Thank you! Very much appreciated.
Comment 5 Michael Catanzaro 2022-12-29 10:29:56 PST
(In reply to zalan from comment #4)
> Thank you! Very much appreciated.

It looks like a lot of things crash and go wrong before this point when using a debug build, so I'll add this to my post-holiday to-do list.
Comment 6 zalan 2022-12-29 10:46:31 PST
(In reply to Michael Catanzaro from comment #5)
> (In reply to zalan from comment #4)
> > Thank you! Very much appreciated.
> 
> It looks like a lot of things crash and go wrong before this point when
> using a debug build, so I'll add this to my post-holiday to-do list.
ok, thanks. You could enable render tree dump in release builds with this ->

diff --git a/Source/WTF/wtf/PlatformEnable.h b/Source/WTF/wtf/PlatformEnable.h
index 4cb5cdfb3da2..22d22f983122 100644
--- a/Source/WTF/wtf/PlatformEnable.h
+++ b/Source/WTF/wtf/PlatformEnable.h
@@ -875,7 +875,7 @@
 #define ENABLE_BINDING_INTEGRITY 1
 #endif
 
-#if !defined(ENABLE_TREE_DEBUGGING) && !defined(NDEBUG)
+#if !defined(ENABLE_TREE_DEBUGGING)
 #define ENABLE_TREE_DEBUGGING 1
 #endif

let me check if this actually complies (from time to time we introduce dependencies behind DEBUG :(
Comment 7 Michael Catanzaro 2022-12-29 11:17:41 PST
Well I wound up tracking down and resolving two recently-introduced bugs, one in Epiphany that was causing a web process crash, then bug #249945 that causes a UI process crash after the web process crash. Neither of these are related to debug builds.

But now I can't reproduce the infinite recursion anymore, so it's either (a) not happening in debug builds (possible but weird), or (b) fixed sometime in the past two weeks (I was testing WebKitGTK 2.39.3 when I reported this, but built today's WebKit main just now). Will investigate more later, probably after holidays. I might try a release build with ENABLE_TREE_DEBUGGING force enabled simply because debug builds are super slow.
Comment 8 Michael Catanzaro 2022-12-29 11:26:00 PST
(In reply to Michael Catanzaro from comment #7)
> But now I can't reproduce the infinite recursion anymore, so it's either (a)
> not happening in debug builds (possible but weird), or (b) fixed sometime in
> the past two weeks (I was testing WebKitGTK 2.39.3 when I reported this, but
> built today's WebKit main just now).

Oh nope, I was just testing the wrong page. :)
Comment 9 zalan 2022-12-29 12:06:39 PST
(In reply to zalan from comment #6)
> (In reply to Michael Catanzaro from comment #5)
> > (In reply to zalan from comment #4)
> > > Thank you! Very much appreciated.
> > 
> > It looks like a lot of things crash and go wrong before this point when
> > using a debug build, so I'll add this to my post-holiday to-do list.
> ok, thanks. You could enable render tree dump in release builds with this ->
> 
> diff --git a/Source/WTF/wtf/PlatformEnable.h
> b/Source/WTF/wtf/PlatformEnable.h
> index 4cb5cdfb3da2..22d22f983122 100644
> --- a/Source/WTF/wtf/PlatformEnable.h
> +++ b/Source/WTF/wtf/PlatformEnable.h
> @@ -875,7 +875,7 @@
>  #define ENABLE_BINDING_INTEGRITY 1
>  #endif
>  
> -#if !defined(ENABLE_TREE_DEBUGGING) && !defined(NDEBUG)
> +#if !defined(ENABLE_TREE_DEBUGGING)
>  #define ENABLE_TREE_DEBUGGING 1
>  #endif
> 
> let me check if this actually complies (from time to time we introduce
> dependencies behind DEBUG :(
It does.
Comment 10 Michael Catanzaro 2023-01-04 08:26:19 PST
Is there any way to make WebKit render a page using the dumped render tree? (I'm wondering if I might be able to reduce the render tree to something shareable more easily than I could reduce the original web content. I don't think I've ever done that successfully before.)
Comment 11 zalan 2023-01-13 08:30:29 PST
Let's use <rdar://104223956> instead
Comment 12 Michael Catanzaro 2023-01-13 12:28:21 PST
(In reply to zalan from comment #11)
> Let's use <rdar://104223956> instead

Is there anything I need to know from that issue? Did someone else find a reproducer, perhaps?
Comment 13 zalan 2023-01-13 14:03:32 PST
(In reply to Michael Catanzaro from comment #12)
> (In reply to zalan from comment #11)
> > Let's use <rdar://104223956> instead
> 
> Is there anything I need to know from that issue? Did someone else find a
> reproducer, perhaps?
No unfortunately not. I just found a better radar in our system to track this issue.

>Is there any way to make WebKit render a page using the dumped render tree?
oops, I totally missed this. Sorry about that.
The answer is no, we don't have such tool.

Any chance of sharing the render tree dump here? If it has some sensitive content, go ahead and obfuscate it, I don't really need any of the inline content. However the tree structure/geometry would be a great help to move this bugzilla.
Comment 14 Michael Catanzaro 2023-01-13 14:51:22 PST
I'll see how hard it is to do. It's a financial page, so probably I can just remove the numbers and such, depending on how big the render tree dump is.
Comment 15 Michael Catanzaro 2023-01-13 15:02:36 PST
Yeah I think this will work. It will take a little while to go through since it's kinda big, but I'll do so and upload it soon, probably next week.
Comment 16 zalan 2023-01-13 15:08:59 PST
(In reply to Michael Catanzaro from comment #15)
> Yeah I think this will work. It will take a little while to go through since
> it's kinda big, but I'll do so and upload it soon, probably next week.
Very much appreciated!
Comment 17 Michael Catanzaro 2023-01-13 15:22:40 PST
Created attachment 464492 [details]
Render dump

Aha, regex are our friends... sometimes. I just removed the strings since you probably don't need them?

This is the dump before the call to layoutRoot->layout(). Looks like it crashes inside that call so there is no after dump.
Comment 18 zalan 2023-01-13 16:21:27 PST
(In reply to Michael Catanzaro from comment #17)
> Created attachment 464492 [details]
> Render dump
> 
> Aha, regex are our friends... sometimes. I just removed the strings since
> you probably don't need them?
> 
> This is the dump before the call to layoutRoot->layout(). Looks like it
> crashes inside that call so there is no after dump.

"censored by mcatanzaro" :)
Comment 19 zalan 2023-01-14 19:43:50 PST
Created attachment 464504 [details]
Test case
Comment 20 zalan 2023-01-15 07:38:27 PST
Created attachment 464507 [details]
Patch
Comment 21 EWS 2023-01-15 10:43:13 PST
Committed 258933@main (1cf5f56743f9): <https://commits.webkit.org/258933@main>

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