REGRESSION (r182215): Reproducible crash at drawsvg.org due to reentrant layout
Created attachment 254864 [details] Patch
rdar://problem/21328402
Said and I discussed an alternative approach, which was to not have getBBox force layout when called from the event region code.
(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.
Specifically, Element::boundsInRootViewSpace() should call svgElement.getBoundingBox(localRect, DisallowStyleUpdate).
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.
Do you prefer the boundsInRootViewSpace fix to this one? If so, please say so.
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.
(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.
I don't understand why the SVG is dirty, and does layout even though we've just done a updateLayoutIgnorePendingStylesheets().
(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.
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.
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.
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.
I chatted with Said, and we think your fix is the way to to.
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.
(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.
Comment on attachment 254864 [details] Patch Clearing flags on attachment: 254864 Committed r185567: <http://trac.webkit.org/changeset/185567>
All reviewed patches have been landed. Closing bug.