Bug 145964 – REGRESSION (r182215): Reproducible crash at drawsvg.org due to reentrant layout

• Component: SVG
• Status: RESOLVED FIXED

[Darin Adler]

REGRESSION (r182215): Reproducible crash at drawsvg.org due to reentrant layout

[Darin Adler]

Created attachment 254864 [details]
Patch

[Alexey Proskuryakov]

rdar://problem/21328402

[Simon Fraser (smfr)]

Said and I discussed an alternative approach, which was to not have getBBox force layout when called from the event region code.

[Darin Adler]

(In reply to comment #3)
> Said and I discussed an alternative approach, which was to not have getBBox
> force layout when called from the event region code.

That also sounds OK.

[Simon Fraser (smfr)]

Specifically, Element::boundsInRootViewSpace() should call svgElement.getBoundingBox(localRect, DisallowStyleUpdate).

[Darin Adler]

OK. But what are the pros and cons of the two solutions? I believe my fix deals with other potential problems that could occur if a layout begins in a subframe and then affects the parent frame. I’d really like someone to review+ or review- this patch so we can get on with other things.

[Darin Adler]

Do you prefer the boundsInRootViewSpace fix to this one? If so, please say so.

[Said Abou-Hallawa]

I tried the attached patch and the bug is fixed. I am not sure what side effects can this patch cause since the ChangeLog does not include any explanation on why this fix should be correct.

I tried calling svgElement.getBoundingBox(localRect, SVGLocatable::DisallowStyleUpdate) in Element::absoluteEventBounds but I got the following crash.

#1	0x0000000111126c11 in WebCore::RenderLayerCompositor::requiresCompositingForScrollableFrame() const at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderLayerCompositor.cpp:2451
#2	0x0000000111126864 in WebCore::RenderLayerCompositor::cacheAcceleratedCompositingFlags() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderLayerCompositor.cpp:345
#3	0x000000011018b5ad in WebCore::FrameView::updateCompositingLayersAfterLayout() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:781
#4	0x000000011018d4d0 in WebCore::FrameView::layout(bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1387
#5	0x000000010fdd5797 in WebCore::Document::updateLayout() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/Document.cpp:1864
#6	0x000000010fdd927f in WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/Document.cpp:1896
#7	0x00000001116e0e09 in WebCore::SVGLocatable::getBBox(WebCore::SVGElement*, WebCore::SVGLocatable::StyleUpdateStrategy) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGLocatable.cpp:68
#8	0x00000001116ccb58 in WebCore::SVGGraphicsElement::getBBox(WebCore::SVGLocatable::StyleUpdateStrategy) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGGraphicsElement.cpp:161
#9	0x0000000110afa66f in WebCore::jsSVGGraphicsElementPrototypeFunctionGetBBox(JSC::ExecState*) at /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/JSSVGGraphicsElement.cpp:290

I did what Simon suggested above and I got almost the same crash but from a nest FrameView::layout() call.

Thread 1Queue : com.apple.main-thread (serial)
#0	0x0000000118a50ab7 in ::WTFCrash() at /Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/Assertions.cpp:321
#1	0x000000011bab1c11 in WebCore::RenderLayerCompositor::requiresCompositingForScrollableFrame() const at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderLayerCompositor.cpp:2451
#2	0x000000011bab1864 in WebCore::RenderLayerCompositor::cacheAcceleratedCompositingFlags() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderLayerCompositor.cpp:345
#3	0x000000011ab165ad in WebCore::FrameView::updateCompositingLayersAfterLayout() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:781
#4	0x000000011ab184d0 in WebCore::FrameView::layout(bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1387
#5	0x000000011bc6f381 in WebCore::RenderWidget::updateWidgetPosition() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderWidget.cpp:322
#6	0x000000011ab19dc4 in WebCore::FrameView::updateWidgetPositions() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:4720
#7	0x000000011ab18ee6 in WebCore::FrameView::performPostLayoutTasks() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:3012
#8	0x000000011ab179a4 in WebCore::FrameView::layout(bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1205
#9	0x000000011ab29d79 in WebCore::FrameView::forceLayoutParentViewIfNeeded() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1140
#10	0x000000011ab1826c in WebCore::FrameView::layout(bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1335
#11	0x000000011a760787 in WebCore::Document::updateLayout() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/Document.cpp:1864
#12	0x000000011a76426f in WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/Document.cpp:1896
#13	0x000000011c06be09 in WebCore::SVGLocatable::getBBox(WebCore::SVGElement*, WebCore::SVGLocatable::StyleUpdateStrategy) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGLocatable.cpp:68
#14	0x000000011c057b58 in WebCore::SVGGraphicsElement::getBBox(WebCore::SVGLocatable::StyleUpdateStrategy) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/svg/SVGGraphicsElement.cpp:161
#15	0x000000011b48566f in WebCore::jsSVGGraphicsElementPrototypeFunctionGetBBox(JSC::ExecState*) at /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/JSSVGGraphicsElement.cpp:290

I was looking at a different way to fix this bug. I think the real problem is in calling the SVG model function svgElement.getBoundingBox() from the layout code. It seems to me weird to call a high level function, like the SVG element bounding box, from the layout code. It is the cause of the layout reentry. And it is not consistent with the behavior of other elements during layout.

[Darin Adler]

(In reply to comment #8)
> I tried the attached patch and the bug is fixed. I am not sure what side
> effects can this patch cause since the ChangeLog does not include any
> explanation on why this fix should be correct.

Sorry my ChangeLog wasn’t clear.

The problem I fixed is that forceLayoutParentViewIfNeeded is called by FrameView::layout, not as part of layout but in the post-layout period. It’s not safe to reenter FrameView::layout on a different frame during this period for a variety of reasons. To me the getBoundingBox part seems to only be a part of the problem and to me it’s pretty clear this change gets to the root of the problem. Note that forceLayoutParentViewIfNeeded sounds like a general purpose function but is in fact only for SVG.

> I was looking at a different way to fix this bug. I think the real problem
> is in calling the SVG model function svgElement.getBoundingBox() from the
> layout code. It seems to me weird to call a high level function, like the
> SVG element bounding box, from the layout code. It is the cause of the
> layout reentry. And it is not consistent with the behavior of other elements
> during layout.

Do we call getBoundingBox from layout code? I’m not sure what exactly “layout code” is.

[Simon Fraser (smfr)]

I don't understand why the SVG is dirty, and does layout even though we've just done a updateLayoutIgnorePendingStylesheets().

[Darin Adler]

(In reply to comment #10)
> I don't understand why the SVG is dirty, and does layout even though we've
> just done a updateLayoutIgnorePendingStylesheets().

I think we did updateLayoutIgnorePendingStylesheets on a different frame. There are two frames involved.

[Darin Adler]

But if you like you can do further analysis to look for another fix. I spent a few hours on this over the weekend but I don’t have more time for it now.

[Darin Adler]

Hmm wait I seem to remember the functions call updateLayout unconditionally. So the layout reentrancy assertion at least will fire even if nothing is dirty.

[Darin Adler]

A lot of this has to do with having multiple frames. FrameView::layout returns immediately if isInLayout is true, but it’s not in this case.

[Simon Fraser (smfr)]

I chatted with Said, and we think your fix is the way to to.

[Simon Fraser (smfr)]

Comment on attachment 254864 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1136
> +    ownerRenderer->view().frameView().scheduleRelayout();

I hope this doesn't cause problems like trying to paint before we've laid out.

[Darin Adler]

(In reply to comment #16)
> I hope this doesn't cause problems like trying to paint before we've laid
> out.

It does open the window for the possibility that we might paint twice in cases like this.

[WebKit Commit Bot]

Comment on attachment 254864 [details]
Patch

Clearing flags on attachment: 254864

Committed r185567: <http://trac.webkit.org/changeset/185567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.