Bug 249942

Summary: [IFC] Infinite recursion in Layout::LineBoxVerticalAligner::layoutBoundsForInlineBoxSubtree
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, koivisto, mcatanzaro, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Render dump
none
Test case
none
Patch ews-feeder: commit-queue-

Michael Catanzaro
Reported 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.
Attachments
Render dump (457.85 KB, text/plain)
2023-01-13 15:22 PST, Michael Catanzaro
no flags
Test case (198 bytes, text/html)
2023-01-14 19:43 PST, alan
no flags
Patch (10.98 KB, patch)
2023-01-15 07:38 PST, alan
ews-feeder: commit-queue-
Radar WebKit Bug Importer
Comment 1 2022-12-29 07:55:39 PST
alan
Comment 2 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?
Michael Catanzaro
Comment 3 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....
alan
Comment 4 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.
Michael Catanzaro
Comment 5 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.
alan
Comment 6 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 :(
Michael Catanzaro
Comment 7 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.
Michael Catanzaro
Comment 8 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. :)
alan
Comment 9 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.
Michael Catanzaro
Comment 10 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.)
alan
Comment 11 2023-01-13 08:30:29 PST
Let's use <rdar://104223956> instead
Michael Catanzaro
Comment 12 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?
alan
Comment 13 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.
Michael Catanzaro
Comment 14 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.
Michael Catanzaro
Comment 15 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.
alan
Comment 16 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!
Michael Catanzaro
Comment 17 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.
alan
Comment 18 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" :)
alan
Comment 19 2023-01-14 19:43:50 PST
Created attachment 464504 [details] Test case
alan
Comment 20 2023-01-15 07:38:27 PST
EWS
Comment 21 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].
Note You need to log in before you can comment on or make changes to this bug.