Bug 221382 – Nullptr crash in RenderStyle::shapeOutside()

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

[Ryosuke Niwa]

e.g.

0   com.apple.WebCore             	0x0000000117d6de8d WTF::RawPtrTraits<WebCore::StyleRareNonInheritedData>::unwrap(WebCore::StyleRareNonInheritedData* const&) + 9 (RawPtrTraits.h:43) [inlined]
1   com.apple.WebCore             	0x0000000117d6de8d WTF::Ref<WebCore::StyleRareNonInheritedData, WTF::RawPtrTraits<WebCore::StyleRareNonInheritedData> >::ptr() const + 9 (Ref.h:116) [inlined]
2   com.apple.WebCore             	0x0000000117d6de8d WTF::DataRef<WebCore::StyleRareNonInheritedData>::operator->() const + 9 (DataRef.h:76) [inlined]
3   com.apple.WebCore             	0x0000000117d6de8d WebCore::RenderStyle::shapeOutside() const + 9 (RenderStyle.h:1415) [inlined]
4   com.apple.WebCore             	0x0000000117d6de8d WebCore::ShapeOutsideInfo::isEnabledFor(WebCore::RenderBox const&) + 13 (ShapeOutsideInfo.cpp:311)
5   com.apple.WebCore             	0x0000000117baa16e WebCore::RenderBox::shapeOutsideInfo() const + 14 (RenderBox.h:632)
6   com.apple.WebCore             	0x0000000117bab51e WebCore::ComputeFloatOffsetForLineLayoutAdapter<(WebCore::FloatingObject::Type)1>::updateOffsetIfNeeded(WebCore::FloatingObject const&) + 78 (FloatingObjects.cpp:486)
7   com.apple.WebCore             	0x0000000117bab61e WebCore::ComputeFloatOffsetAdapter<(WebCore::FloatingObject::Type)1>::collectIfNeeded(WebCore::PODInterval<WebCore::LayoutUnit, WebCore::FloatingObject*> const&) + 66 (FloatingObjects.cpp:477) [inlined]
8   com.apple.WebCore             	0x0000000117bab61e void WebCore::PODIntervalTree<WebCore::LayoutUnit, WebCore::FloatingObject*>::searchForOverlapsFrom<WebCore::ComputeFloatOffsetForLineLayoutAdapter<(WebCore::FloatingObject::Type)1> >(WebCore::PODRedBlackTree<WebCore::PODInterval<WebCore::LayoutUnit, WebCore::FloatingObject*>, WebCore::PODIntervalNodeUpdater>::Node*, WebCore::ComputeFloatOffsetForLineLayoutAdapter<(WebCore::FloatingObject::Type)1>&) const + 126 (PODIntervalTree.h:108)
9   com.apple.WebCore             	0x0000000117ba7f3c void WebCore::PODIntervalTree<WebCore::LayoutUnit, WebCore::FloatingObject*>::allOverlapsWithAdapter<WebCore::ComputeFloatOffsetForLineLayoutAdapter<(WebCore::FloatingObject::Type)1> >(WebCore::ComputeFloatOffsetForLineLayoutAdapter<(WebCore::FloatingObject::Type)1>&) const + 12 (PODIntervalTree.h:60) [inlined]
10  com.apple.WebCore             	0x0000000117ba7f3c WebCore::FloatingObjects::logicalLeftOffset(WebCore::LayoutUnit, WebCore::LayoutUnit, WebCore::LayoutUnit) + 204 (FloatingObjects.cpp:424)
11  com.apple.WebCore             	0x0000000117d368a2 WebCore::RenderBlock::logicalLeftOffsetForLine(WebCore::LayoutUnit, WebCore::LayoutUnit, bool, WebCore::LayoutUnit) const + 21 (RenderBlock.h:348) [inlined]
12  com.apple.WebCore             	0x0000000117d368a2 WebCore::RenderBlock::logicalLeftOffsetForLine(WebCore::LayoutUnit, WebCore::IndentTextOrNot, WebCore::LayoutUnit) const + 43 (RenderBlock.h:158) [inlined]
13  com.apple.WebCore             	0x0000000117d368a2 WebCore::RootInlineBox::selectionBottom() const + 562 (RootInlineBox.cpp:693)
14  com.apple.WebCore             	0x0000000117ce1433 WebCore::RootInlineBox::selectionHeight() const + 8 (RootInlineBox.h:78) [inlined]
15  com.apple.WebCore             	0x0000000117ce1433 WebCore::RenderReplaced::localSelectionRect(bool) const + 371 (RenderReplaced.cpp:736)
16  com.apple.WebCore             	0x0000000117ce356d WebCore::RenderReplaced::clippedOverflowRectForRepaint(WebCore::RenderLayerModelObject const*) const + 61 (RenderReplaced.cpp:772)
17  com.apple.WebCore             	0x0000000117cd7d03 WebCore::RenderObject::rectWithOutlineForRepaint(WebCore::RenderLayerModelObject const*, WebCore::LayoutUnit) const + 19 (RenderObject.cpp:952)
18  com.apple.WebCore             	0x0000000117c732cd WebCore::RenderInline::rectWithOutlineForRepaint(WebCore::RenderLayerModelObject const*, WebCore::LayoutUnit) const + 109 (RenderInline.cpp:893)
19  com.apple.WebCore             	0x0000000117c73187 WebCore::RenderInline::clippedOverflowRectForRepaint(WebCore::RenderLayerModelObject const*) const + 679 (RenderInline.cpp:878)
20  com.apple.WebCore             	0x0000000117cceb33 WebCore::RenderObject::repaint() const + 99 (RenderObject.cpp:898)
21  com.apple.WebCore             	0x0000000117c369e1 WebCore::RenderElement::repaintBeforeStyleChange(WebCore::StyleDifference, WebCore::RenderStyle const&, WebCore::RenderStyle const&) + 433 (RenderElement.cpp:446)
22  com.apple.WebCore             	0x0000000117c36c0c WebCore::RenderElement::setStyle(WebCore::RenderStyle&&, WebCore::StyleDifference) + 140 (RenderElement.cpp:485)
23  com.apple.WebCore             	0x0000000117de08d4 WebCore::RenderTreeUpdater::updateRendererStyle(WebCore::RenderElement&, WebCore::RenderStyle&&, WebCore::StyleDifference) + 32 (RenderTreeUpdater.cpp:296) [inlined]
24  com.apple.WebCore             	0x0000000117de08d4 WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdates const&) + 980 (RenderTreeUpdater.cpp:363)
25  com.apple.WebCore             	0x0000000117ddfb30 WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) + 1040 (RenderTreeUpdater.cpp:194)
26  com.apple.WebCore             	0x0000000117ddf62b WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 555 (RenderTreeUpdater.cpp:126)
27  com.apple.WebCore             	0x00000001172d3259 WebCore::Document::updateRenderTree(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 105 (Document.cpp:1968)
28  com.apple.WebCore             	0x00000001172d35e6 WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 822 (Document.cpp:2058)
29  com.apple.WebCore             	0x00000001172d3a67 WebCore::Document::updateStyleIfNeeded() + 311 (Document.cpp:2145)
30  com.apple.WebCore             	0x00000001178a207b WebCore::FrameViewLayoutContext::updateStyleForLayout() + 82 (FrameViewLayoutContext.cpp:545) [inlined]
31  com.apple.WebCore             	0x00000001178a207b WebCore::FrameViewLayoutContext::layout() + 459 (FrameViewLayoutContext.cpp:211)
32  com.apple.WebCore             	0x00000001178b0135 WebCore::FrameView::updateContentsSize() + 165 (FrameView.cpp:2787)
33  com.apple.WebCore             	0x0000000117972a8a WebCore::ScrollView::updateScrollbars(WebCore::IntPoint const&) + 1674 (ScrollView.cpp:710)
34  com.apple.WebCore             	0x0000000117973df2 WebCore::ScrollView::setContentsSize(WebCore::IntSize const&) + 114 (ScrollView.cpp:402)
35  com.apple.WebCore             	0x00000001178a5d7a WebCore::FrameView::setContentsSize(WebCore::IntSize const&) + 58 (FrameView.cpp:589)
36  com.apple.WebCore             	0x000000011789fa74 WebCore::FrameView::adjustViewSize() + 164 (FrameView.cpp:621)
37  com.apple.WebCore             	0x00000001178a254e WebCore::FrameViewLayoutContext::layout() + 1694 (FrameViewLayoutContext.cpp:247)
38  com.apple.WebCore             	0x00000001172d76e3 WebCore::Document::implicitClose() + 947 (Document.cpp:3149)
39  com.apple.WebCore             	0x00000001177c1e07 WebCore::FrameLoader::checkCompleted() + 391 (FrameLoader.cpp:881)

<rdar://problem/73133028>

[Ryosuke Niwa]

Created attachment 419236 [details]
Test

[Frédéric Wang (:fredw)]

Created attachment 420637 [details]
Reduced testcase with similar behavior

So the issue is that in WebCore::ComputeFloatOffsetForLineLayoutAdapter, a RenderBlockFlow (with an associated FloatingObjects) tries to access the dangling WeakPtr of a float descendant (associated to a FloatingObject) that was released in a previous loop step of WebCore::RenderTreeUpdater::updateRenderTree.

For some reason, I'm not able to reproduce the crash on Linux with attachment 419236 [details]. However, here is a simplified testcase exhibiting a similar issue. The destruction of the float renderer still happens in WebCore::FrameViewLayoutContext::updateStyleForLayout() as in attachment 419236 [details], but the bad access is now only detected during the layout of the RenderBlockFlow container:

#0  WTFCrash() () at ../../Source/WTF/wtf/Assertions.cpp:295
#1  0x00007f065482a8cd in CRASH_WITH_INFO(...) ()
    at DerivedSources/ForwardingHeaders/wtf/Assertions.h:713
...
#6  0x00007f0658b45bc3 in WebCore::FloatingObjects::moveAllToFloatInfoMap
    at ../../Source/WebCore/rendering/FloatingObjects.cpp:298
#7  0x00007f0658bc01e1 in WebCore::RenderBlockFlow::rebuildFloatingObjectSetFromIntrudingFloats()
    at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:218
#8  0x00007f0658bc1a4b in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
    at ../../Source/WebCore/rendering/RenderBlockFlow.cpp:483
...
#17 0x00007f0658bb06f7 in WebCore::RenderBlock::layout()
    at ../../Source/WebCore/rendering/RenderBlock.cpp:598
#18 0x00007f0658e0333b in WebCore::RenderView::layout()
    at ../../Source/WebCore/rendering/RenderView.cpp:185
#19 0x00007f065856b809 in WebCore::FrameViewLayoutContext::layout()
    at ../../Source/WebCore/page/FrameViewLayoutContext.cpp:232

[Frédéric Wang (:fredw)]

Created attachment 420638 [details]
Incorrect patch

This patch fixes the crash for attachment 419236 [details] and attachment 420637 [details], however it is a bit too extreme and needs to be refined (e.g. it causes needs-layout assertions on the minibrowser start page).

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #2)
> Created attachment 420637 [details]
> Reduced testcase with similar behavior
> 
> So the issue is that in WebCore::ComputeFloatOffsetForLineLayoutAdapter, a
> RenderBlockFlow (with an associated FloatingObjects) tries to access the
> dangling WeakPtr of a float descendant (associated to a FloatingObject) that
> was released in a previous loop step of
> WebCore::RenderTreeUpdater::updateRenderTree.

Debugging session with more details of the renderer tree as well as the backtrace of when the renderer is removed.

Thread 1 received signal SIGSEGV, Segmentation fault.
(rr) reverse-f
(rr) 
(rr) 
(rr) 
(rr) 
(rr) 
https://webkit-search.igalia.com/webkit/rev/3b16c05731df770efe9fcaa447c5df9ff82c043d/Source/WebCore/rendering/FloatingObjects.cpp#298
(rr) p &renderer
$1 = (WebCore::RenderBox *) 0x0
(rr) p showRenderTree(&this->renderer())

(B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, hasLayer(S)crollableArea, (C)omposited, (+)Dirty style, (+)Dirty layout
B---YGLS- -+  RenderView at (0,0) size 0x600 renderer->(0x7fdd44b4d250) (layout overflow 0,0 803x600) (visual overflow 0,0 785x600) layout->[normal child]
B-----LS- -+    HTML RenderBlock at (0,0) size 18x600 renderer->(0x7fdd44b4d6c0) node->(0x7fdd44b4fa80) (layout overflow -3,0 806x600) (visual overflow -3,0 806x600) layout->[self][normal child]
                   floating object 0x7fdcea4a8420 renderer 0x7fdd44b1c9d0 at (-3.50,0) size 37x14 paintsFloat 1 shouldPaint 1
B---YG--- --      RenderBlock at (0,0) size 18x600 renderer->(0x7fdd44b1c380)
                     floating object 0x7fdcea4a8360 renderer 0x7fdd44b1c9d0 at (-3.50,0) size 37x14 paintsFloat 0 shouldPaint 0
-------- --        Line: (top: 0 bottom: 17) with leading (top: 0 bottom: 18)
-------- --        RootInlineBox at (0,14) size 17x8 (0x7fdd44b1cd30) renderer->(0x7fdd44b1c380)
-------- --          InlineTextBox at (0,14) size 17x8 (0x7fdd44b1ccc0) renderer->(0x7fdd44b1c190) run(0, 1) "b"
-------- --          InlineBox at (3,22) size 0x0 (0x7fdd44b1cdf0) renderer->(0x7fdd44b1c230)
I----G--- --        <pseudo> RenderInline renderer->(0x7fdd44b1c0b0) node->(0x7fdd44b1c010)
B-F-YG--- --          RenderBlock at (-3.50,0) size 37x14 renderer->(0x7fdd44b1c9d0) (layout overflow 0,0 43x14) (visual overflow 0,0 43x14)
-------- --            Line: (top: 8 bottom: 43) with leading (top: 0 bottom: 37)
-------- --            RootInlineBox at (8,0) size 35x14 (0x7fdd44b1cc00) renderer->(0x7fdd44b1c9d0)
-------- --              InlineTextBox at (8,0) size 35x14 (0x7fdd44b1cb90) renderer->(0x7fdd44b1caf0) run(0, 1) "a"
I---YG--- --            RenderText renderer->(0x7fdd44b1caf0)
I---YG--- --          RenderText renderer->(0x7fdd44b1c190)
I---YG--- --          RenderImage at (3,22) size 0x0 renderer->(0x7fdd44b1c230)
B-------- -+*     BODY RenderBody at (26,8) size 769x584 renderer->(0x7fdd44b4d7e0) node->(0x7fdd44b4fef0) (layout overflow -3,0 772x584) (visual overflow -3,0 772x584) layout->[self][normal child]
                     floating object 0x7fdcea4a8120 renderer (nil) at (-3.50,0) size 19x14 paintsFloat 1 shouldPaint 0
I----G--- -+        <pseudo> RenderInline renderer->(0x7fdd44b1c540) node->(0x7fdd44b1c4a0) layout->[self][normal child]
B-F-YG--- -+          RenderBlock at (0,0) size 0x0 renderer->(0x7fdd44b1c810) layout->[self][normal child]
I---YG--- -+            RenderText renderer->(0x7fdd44b1c930) layout->[self]
I---YG--- -+          RenderText renderer->(0x7fdd44b1c620) layout->[self]
I---YG--- -+          RenderImage at (0,0) size 0x0 renderer->(0x7fdd44b1c6c0) layout->[self]

$2 = void
(rr) p it->get()
$3 = (WebCore::FloatingObject *) 0x7fdcea4a8120
(rr) watch -l ((WebCore::FloatingObject *) 0x7fdcea4a8120)->m_renderer
(rr) rc
(rr) reverse-f
(rr) 
(rr) 
(rr) 
https://webkit-search.igalia.com/webkit/rev/3b16c05731df770efe9fcaa447c5df9ff82c043d/Source/WebCore/rendering/FloatingObjects.cpp#52
(rr) p &renderer
$4 = (WebCore::RenderBox *) 0x7fdd44b1c810
(rr) watch -l ((WebCore::RenderBox *) 0x7fdd44b1c810)->m_parent
(rr) c
(rr) 
https://webkit-search.igalia.com/webkit/rev/3b16c05731df770efe9fcaa447c5df9ff82c043d/Source/WebCore/rendering/RenderObject.cpp#271
(rr) bt
#0  WebCore::RenderObject::setParent(WebCore::RenderElement*) (this=0x7fdd44b1c810, parent=0x0) at ../../Source/WebCore/rendering/RenderObject.cpp:272
#1  0x00007fdd61d4984e in WebCore::RenderElement::detachRendererInternal(WebCore::RenderObject&) (this=0x7fdd44b1c540, renderer=...)
    at ../../Source/WebCore/rendering/RenderElement.cpp:588
#2  0x00007fdd6205ef99 in WebCore::RenderTreeBuilder::detachFromRenderElement(WebCore::RenderElement&, WebCore::RenderObject&, WebCore::RenderTreeBuilder::WillBeDestroyed) (this=0x7ffd9209ded8, parent=..., child=..., willBeDestroyed=WebCore::RenderTreeBuilder::WillBeDestroyed::Yes)
    at ../../Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:896
#3  0x00007fdd6205cc34 in WebCore::RenderTreeBuilder::detach(WebCore::RenderElement&, WebCore::RenderObject&, WebCore::RenderTreeBuilder::CanCollapseAnonymousBlock) (this=0x7ffd9209ded8, parent=..., child=..., canCollapseAnonymousBlock=WebCore::RenderTreeBuilder::CanCollapseAnonymousBlock::Yes)
    at ../../Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:397
#4  0x00007fdd6205b713 in WebCore::RenderTreeBuilder::destroy(WebCore::RenderObject&) (this=0x7ffd9209ded8, renderer=...)
    at ../../Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:169
#5  0x00007fdd6205b853 in WebCore::RenderTreeBuilder::destroy(WebCore::RenderObject&) (this=0x7ffd9209ded8, renderer=...)
    at ../../Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:193
#6  0x00007fdd6205ea40 in WebCore::RenderTreeBuilder::destroyAndCleanUpAnonymousWrappers(WebCore::RenderObject&) (this=0x7ffd9209ded8, rendererToDestroy=...)
    at ../../Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:818
#7  0x00007fdd6207443a in operator()(unsigned int) const (__closure=0x7ffd920989c0, depth=0)
    at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:589
#8  0x00007fdd620746b4 in WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType, WebCore::RenderTreeBuilder&) (root=..., teardownType=WebCore::RenderTreeUpdater::TeardownType::RendererUpdate, builder=...)
    at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:612
#9  0x00007fdd62072f38 in WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdates const&)
    (this=0x7ffd9209deb0, element=..., updates=...) at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:325
#10 0x00007fdd62080ef4 in WebCore::RenderTreeUpdater::GeneratedContent::updatePseudoElement(WebCore::Element&, WebCore::Style::ElementUpdates const&, WebCore::PseudoId) (this=0x7fdcea4d2e50, current=..., updates=..., pseudoId=WebCore::PseudoId::Before)
    at https://webkit-search.igalia.com/webkit/rev/3b16c05731df770efe9fcaa447c5df9ff82c043d/Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp#155
#11 0x00007fdd62072961 in WebCore::RenderTreeUpdater::updateBeforeDescendants(WebCore::Element&, WebCore::Style::ElementUpdates const*)
    (this=0x7ffd9209deb0, element=..., updates=0x7fdd44e43e50) at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:254
#12 0x00007fdd62072825 in WebCore::RenderTreeUpdater::pushParent(WebCore::Element&, WebCore::Style::ElementUpdates const*)
    (this=0x7ffd9209deb0, element=..., updates=0x7fdd44e43e50) at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:231
#13 0x00007fdd62072691 in WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) (this=0x7ffd9209deb0, root=...)
    at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:204
#14 0x00007fdd62071fec in WebCore::RenderTreeUpdater::commit(std::unique_ptr<WebCore::Style::Update const, std::default_delete<WebCore::Style::Update const> >)
    (this=0x7ffd9209deb0, styleUpdate=std::unique_ptr<const WebCore::Style::Update> = {...})
    at ../../Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:126
#15 0x00007fdd60b1fd59 in WebCore::Document::updateRenderTree(std::unique_ptr<WebCore::Style::Update const, std::default_delete<WebCore::Style::Update const> >) (this=0x7fdd44b4ec70, styleUpdate=std::unique_ptr<const WebCore::Style::Update> = {...}) at ../../Source/WebCore/dom/Document.cpp:1979
#16 0x00007fdd60b20308 in WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType)
    (this=0x7fdd44b4ec70, type=WebCore::Document::ResolveStyleType::Normal) at ../../Source/WebCore/dom/Document.cpp:2069
#17 0x00007fdd60b208b8 in WebCore::Document::updateStyleIfNeeded() (this=0x7fdd44b4ec70) at ../../Source/WebCore/dom/Document.cpp:2156
#18 0x00007fdd61650d36 in WebCore::FrameViewLayoutContext::updateStyleForLayout() (this=0x7fdd44b4c158)
    at https://webkit-search.igalia.com/webkit/rev/3b16c05731df770efe9fcaa447c5df9ff82c043d/Source/WebCore/page/FrameViewLayoutContext.cpp#545

[Frédéric Wang (:fredw)]

Created attachment 421641 [details]
Patch

This is a better patch than attachment 420638 [details], clearing the float objects at RenderTreeUpdater::GeneratedContent::updatePseudoElement(). It probably can still be refined but I'm already uploading to get feedback and try EWS.

[Ryosuke Niwa]

Comment on attachment 421641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421641&action=review

> Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117
> +        // Clear floating objects to avoid dangling WeakPtr when their renderers are destroyed.
> +        if (is<RenderBlockFlow>(renderer->parent()))

Doesn't this mean that we're also not null-checking where WeakPtr is used?
It seems that we should be adding a nullptr there as well with perhaps a debug-only assertion that it shouldn't be.

[Frédéric Wang (:fredw)]

Comment on attachment 421641 [details]
Patch

removing review flag as I plan to refine the patch.

[Frédéric Wang (:fredw)]

(In reply to Ryosuke Niwa from comment #6)
> Comment on attachment 421641 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421641&action=review
> 
> > Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117
> > +        // Clear floating objects to avoid dangling WeakPtr when their renderers are destroyed.
> > +        if (is<RenderBlockFlow>(renderer->parent()))
> 
> Doesn't this mean that we're also not null-checking where WeakPtr is used?
> It seems that we should be adding a nullptr there as well with perhaps a
> debug-only assertion that it shouldn't be.

Right, FloatingObject::renderer() is just dereferences that way without null-checking:

RenderBox& renderer() const { return *m_renderer; }

Not sure exactly what you are proposing. Add an ASSERT(m_renderer) here, or just null-check / ASSERT in the specific call site involved in this bug, which is FloatingObjects::moveAllToFloatInfoMap?

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #8)
> Right, FloatingObject::renderer() is just dereferences that way without
> null-checking:
> 
> RenderBox& renderer() const { return *m_renderer; }
> 
> Not sure exactly what you are proposing. Add an ASSERT(m_renderer) here, or
> just null-check / ASSERT in the specific call site involved in this bug,
> which is FloatingObjects::moveAllToFloatInfoMap?

mmh, actually since m_renderer is private, we can only perform null checks within FloatingObject anyway...

[Frédéric Wang (:fredw)]

Comment on attachment 421641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421641&action=review

>>> Source/WebCore/rendering/updating/RenderTreeUpdaterGeneratedContent.cpp:117
>>> +        if (is<RenderBlockFlow>(renderer->parent()))
>> 
>> Doesn't this mean that we're also not null-checking where WeakPtr is used?
>> It seems that we should be adding a nullptr there as well with perhaps a debug-only assertion that it shouldn't be.
> 
> Right, FloatingObject::renderer() is just dereferences that way without null-checking:
> 
> RenderBox& renderer() const { return *m_renderer; }
> 
> Not sure exactly what you are proposing. Add an ASSERT(m_renderer) here, or just null-check / ASSERT in the specific call site involved in this bug, which is FloatingObjects::moveAllToFloatInfoMap?

I only added a debug ASSERT in the latest patch. I hope that's fine.

[Frédéric Wang (:fredw)]

Created attachment 422931 [details]
Patch

(In reply to Frédéric Wang (:fredw) from comment #4)
> B-------- -+*     BODY RenderBody at (26,8) size 769x584
> renderer->(0x7fdd44b4d7e0) node->(0x7fdd44b4fef0) (layout overflow -3,0
> 772x584) (visual overflow -3,0 772x584) layout->[self][normal child]
>                      floating object 0x7fdcea4a8120 renderer (nil) at
> (-3.50,0) size 19x14 paintsFloat 1 shouldPaint 0
> I----G--- -+        <pseudo> RenderInline renderer->(0x7fdd44b1c540)
> node->(0x7fdd44b1c4a0) layout->[self][normal child]
> B-F-YG--- -+          RenderBlock at (0,0) size 0x0
> renderer->(0x7fdd44b1c810) layout->[self][normal child]
> I---YG--- -+            RenderText renderer->(0x7fdd44b1c930) layout->[self]
> I---YG--- -+          RenderText renderer->(0x7fdd44b1c620) layout->[self]
> I---YG--- -+          RenderImage at (0,0) size 0x0
> renderer->(0x7fdd44b1c6c0) layout->[self]

So just to make it clear : the <pseudo> element (0x7fdd44b1c540) has a first child (0x7fdd44b1c810) which is a float. That one is wrapped as a WeakPtr in the FloatingObjects of the parent (0x7fdd44b4d7e0). When the pseudo element is updated, the float object is removed, leading to a dangling WeakPtr. The patch addresses that particular case by calling removeFloatingObjects() on the parent of the pseudo element. But I wonder if it could be refined to target more precisely the relevant cases. Not sure what are these situations though.

[zalan]

This looks to be progressed at https://trac.webkit.org/changeset/274278/webkit

[Ryosuke Niwa]

Maybe we can still add tests?

[Frédéric Wang (:fredw)]

Created attachment 423580 [details]
Patch

Thanks Zalan, yes the tests are no longer crashing.

[EWS]

Committed r274645: <https://commits.webkit.org/r274645>

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