Here is a stack trace: #0 0x00007f2b3c1d5597 in WebCore::GraphicsLayer::~GraphicsLayer (this=0x7f2b2e8fc400, __in_chrg=<optimized out>) at ../../third_party/WebKit/Source/WebCore/platform/graphics/GraphicsLayer.cpp:99 #1 0x00007f2b3c1f7ad7 in WebCore::GraphicsLayerChromium::~GraphicsLayerChromium (this=0x7f2b2e8fc400, __in_chrg=<optimized out>) at ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:107 #2 0x00007f2b3b576f08 in WTF::deleteOwnedPtr<WebCore::GraphicsLayer> (ptr=0x7f2b2e8fc400) at ../../third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtrCommon.h:55 #3 0x00007f2b3b57e434 in WTF::OwnPtr<WebCore::GraphicsLayer>::clear (this=0x7f2b2e87c298) at ../../third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtr.h:100 #4 0x00007f2b3b57e252 in WTF::OwnPtr<WebCore::GraphicsLayer>::operator= (this=0x7f2b2e87c298) at ../../third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtr.h:73 #5 0x00007f2b3bbbd2b6 in WebCore::RenderLayerCompositor::destroyRootLayer (this=0x7f2b2e87c210) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:2021 #6 0x00007f2b3bbb68fb in WebCore::RenderLayerCompositor::enableCompositingMode (this=0x7f2b2e87c210, enable=false) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:184 #7 0x00007f2b3bbb906b in WebCore::RenderLayerCompositor::computeCompositingRequirements (this=0x7f2b2e87c210, layer=0x7f2b2e8792d8, overlapMap=0x7fffbc3bc6a0, compositingState=..., layersChanged=@0x7fffbc3bc69c) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:856 #8 0x00007f2b3bbb70ec in WebCore::RenderLayerCompositor::updateCompositingLayers (this=0x7f2b2e87c210, updateType=WebCore::CompositingUpdateAfterLayoutOrStyleChange, updateRoot=0x7f2b2e8792d8) at ../../third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp:361 #9 0x00007f2b3c8de2a5 in WebCore::FrameView::updateCompositingLayers (this=0x7f2b2f90a700) at ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:646 #10 0x00007f2b3c8dfa35 in WebCore::FrameView::layout (this=0x7f2b2f90a700, allowSubtree=true) at ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:1128 #11 0x00007f2b3c8e6f89 in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive (this=0x7f2b2f90a700) at ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:3079 #12 0x00007f2b3b5c1336 in WebKit::WebFrameImpl::layout (this=0x7f2b2e844a00) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:2053 #13 0x00007f2b3b61becd in WebKit::WebViewImpl::layout (this=0x7f2b320a7600) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:1391 #14 0x00007f2b39d2d9cc in webkit::WebViewPlugin::paint (this=0x7f2b2e87a270, canvas=0x7f2b2f8e1c40, rect=...) at ../../webkit/plugins/webview_plugin.cc:140 #15 0x00007f2b3b5f9883 in WebKit::WebPluginContainerImpl::paint (this=0x7f2b2e827640, gc=0x7f2b2e896b40, damageRect=...) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:138 #16 0x00007f2b3bc4de6a in WebCore::RenderWidget::paint (this=0x7f2b2e849b58, paintInfo=..., paintOffset=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderWidget.cpp:299 #17 0x00007f2b3bb5c1e7 in WebCore::RenderEmbeddedObject::paint (this=0x7f2b2e849b58, paintInfo=..., paintOffset=...) at ../../third_party/WebKit/Source/WebCore/rendering/RenderEmbeddedObject.cpp:156 The simplest solution seems to be to get rid of the ASSERT(!s_inPaintContents) in GraphicsLayer::~GraphicsLayer. Is there a better solution?
This isn't really a crash, it's an ASSERT(). Debug only. Why is a WebViewPlugin (what is that?) calling layout() during paint()? Why can't it layout its child when layout() is called on itself?
(In reply to comment #1) > This isn't really a crash, it's an ASSERT(). Debug only. > Yes, updated title to better represent this. > Why is a WebViewPlugin (what is that?) calling layout() during paint()? Why can't it layout its child when layout() is called on itself? A WebViewPlugin is, more or less, what its name suggests, it's a PluginContainer holds a WebView. This is used for things like the "Plugin not found" box. The crash is in WebKit but this is more of a chromium bug. Will post it there.
To expand a bit, WebViewPlugin is a WebPlugin implementation that forwards drawing and input event handling to a separate WebView, to allow us to draw plug-in placeholders in HTML. The inner WebView is by design completely independent from the outer one (from the point of view of the outer one, it's just a plug-in), so it shouldn't be a problem that we call layout() on the inner one during paint() in the outer one. WebPlugin also doesn't have a layout() method, so we can't call layout() on the inner WebView when the outer one is being layouted. Is the crash caused by the two RenderViews somehow sharing graphics layers? If so, maybe that's what we should prevent.
(In reply to comment #3) > To expand a bit, WebViewPlugin is a WebPlugin implementation that forwards drawing and input event handling to a separate WebView, to allow us to draw plug-in placeholders in HTML. The inner WebView is by design completely independent from the outer one (from the point of view of the outer one, it's just a plug-in), so it shouldn't be a problem that we call layout() on the inner one during paint() in the outer one. WebPlugin also doesn't have a layout() method, so we can't call layout() on the inner WebView when the outer one is being layouted. > > Is the crash caused by the two RenderViews somehow sharing graphics layers? If so, maybe that's what we should prevent. The problem here is s_inPaintContents is a static variable that's shared between the two WebView instances. One WebView is painting while the other is laying out.
(In reply to comment #4) > (In reply to comment #3) > > To expand a bit, WebViewPlugin is a WebPlugin implementation that forwards drawing and input event handling to a separate WebView, to allow us to draw plug-in placeholders in HTML. The inner WebView is by design completely independent from the outer one (from the point of view of the outer one, it's just a plug-in), so it shouldn't be a problem that we call layout() on the inner one during paint() in the outer one. WebPlugin also doesn't have a layout() method, so we can't call layout() on the inner WebView when the outer one is being layouted. > > > > Is the crash caused by the two RenderViews somehow sharing graphics layers? If so, maybe that's what we should prevent. > > The problem here is s_inPaintContents is a static variable that's shared between the two WebView instances. One WebView is painting while the other is laying out. How important is it that we keep this assert? It would be nice to debug in force-compositing-mode and not occasionally fire this assert when there's an old or missing plugin.
(In reply to comment #4) > (In reply to comment #3) > > To expand a bit, WebViewPlugin is a WebPlugin implementation that forwards drawing and input event handling to a separate WebView, to allow us to draw plug-in placeholders in HTML. The inner WebView is by design completely independent from the outer one (from the point of view of the outer one, it's just a plug-in), so it shouldn't be a problem that we call layout() on the inner one during paint() in the outer one. WebPlugin also doesn't have a layout() method, so we can't call layout() on the inner WebView when the outer one is being layouted. > > > > Is the crash caused by the two RenderViews somehow sharing graphics layers? If so, maybe that's what we should prevent. > > The problem here is s_inPaintContents is a static variable that's shared between the two WebView instances. One WebView is painting while the other is laying out. Perhaps we can move this flag (which seems to be for debugging purposes only) to Page or some other "one per WebView" object?
It's very important for the normal case where there's one compositing tree in the process and mutating it is a sign of either concurrent access from multiple threads (always bad) or something mutating layout inside a WebView while that WebView is being painted. You could make this restriction per-WebView if you wanted isolation here. I would not be OK with turning the ASSERT() off completely, it's helped us catch many other bugs. Or just do layout inside the WebView at the same time inside the WebViewPlugin as outside the WebView if you are painting them in lockstep, which it appears from this callstack that you are.
Discussed this offline, here's my suggestion. What do you think James? GraphicsLayer::~GraphicsLayer { m_client->verifyNotPainting() } RenderLayerBacking::verifyNotPainting() { ASSERT(!toRenderView(renderer())->frameView()->frame()->page()->isPainting()); } RenderLayerBacking::paintContents() { toRenderView(renderer())->frameView()->frame()->page()->setIsPainting(true)); ... toRenderView(renderer())->frameView()->frame()->page()->setIsPainting(false)); }
Created attachment 133492 [details] Patch
Created attachment 133493 [details] Patch
Comment on attachment 133493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133493&action=review This seems like a good compromise to me, as long as renderer->frame and frame->page always exist! :) > Source/WebCore/page/Page.h:437 > +#ifndef NDEBG typo: NDEBUG > Source/WebCore/platform/graphics/GraphicsLayer.cpp:91 > + if (m_client) > + m_client->verifyNotPainting(); ifndef NDEBUG? > Source/WebCore/platform/graphics/GraphicsLayer.cpp:97 > + if (m_client) > + m_client->verifyNotPainting(); ifndef NDEBUG? > Source/WebCore/platform/graphics/GraphicsLayerClient.h:78 > + virtual void verifyNotPainting() { } ifndef NDEBUG? > Source/WebCore/rendering/RenderLayerBacking.cpp:1179 > + toRenderView(renderer())->frameView()->frame()->page()->setIsPainting(false); should this be renderer()->frame()->page()-> like below?
Comment on attachment 133493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133493&action=review >> Source/WebCore/page/Page.h:437 >> +#ifndef NDEBG > > typo: NDEBUG Thanks, fixed. Reuploading, but will do a sanity check by building release locally. >> Source/WebCore/platform/graphics/GraphicsLayer.cpp:91 >> + m_client->verifyNotPainting(); > > ifndef NDEBUG? Seems unnecessary? This will just do nothing. >> Source/WebCore/platform/graphics/GraphicsLayer.cpp:97 >> + m_client->verifyNotPainting(); > > ifndef NDEBUG? Unnecessary. This will just do nothing. >> Source/WebCore/platform/graphics/GraphicsLayerClient.h:78 >> + virtual void verifyNotPainting() { } > > ifndef NDEBUG? Is that necessary? One extra pointer in the vtable... >> Source/WebCore/rendering/RenderLayerBacking.cpp:1179 >> + toRenderView(renderer())->frameView()->frame()->page()->setIsPainting(false); > > should this be renderer()->frame()->page()-> like below? Yup, thanks.
(In reply to comment #12) > (From update of attachment 133493 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133493&action=review > > >> Source/WebCore/page/Page.h:437 > >> +#ifndef NDEBG > > > > typo: NDEBUG > > Thanks, fixed. Reuploading, but will do a sanity check by building release locally. > > >> Source/WebCore/platform/graphics/GraphicsLayer.cpp:91 > >> + m_client->verifyNotPainting(); > > > > ifndef NDEBUG? > > Seems unnecessary? This will just do nothing. > > >> Source/WebCore/platform/graphics/GraphicsLayer.cpp:97 > >> + m_client->verifyNotPainting(); > > > > ifndef NDEBUG? > > Unnecessary. This will just do nothing. > > >> Source/WebCore/platform/graphics/GraphicsLayerClient.h:78 > >> + virtual void verifyNotPainting() { } > > > > ifndef NDEBUG? > > Is that necessary? One extra pointer in the vtable... > > >> Source/WebCore/rendering/RenderLayerBacking.cpp:1179 > >> + toRenderView(renderer())->frameView()->frame()->page()->setIsPainting(false); > > > > should this be renderer()->frame()->page()-> like below? > > Yup, thanks. Nevermind, I'll guard calls in GraphicsLayer too. We probably don't want to do an extra virtual method call in GraphicsLayer construction/destruction on release builds.
Created attachment 133504 [details] Patch
Comment on attachment 133504 [details] Patch lgtm. Does this work for you James?
Comment on attachment 133504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133504&action=review > Source/WebCore/platform/graphics/GraphicsLayerClient.h:78 > + virtual void verifyNotPainting() { } why is this not #ifndef NDEBUG guarded? I think you need to document what this is or give it a better name, it's not very obvious what it means from the name along (what exactly is it verifying? what happens if the "verification" fails? who is it checking painting on - this client, or others?)
Created attachment 133580 [details] Patch
(In reply to comment #16) > (From update of attachment 133504 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133504&action=review > > > Source/WebCore/platform/graphics/GraphicsLayerClient.h:78 > > + virtual void verifyNotPainting() { } > > why is this not #ifndef NDEBUG guarded? > > I think you need to document what this is or give it a better name, it's not very obvious what it means from the name along (what exactly is it verifying? what happens if the "verification" fails? who is it checking painting on - this client, or others?) Done. Thanks.
Comment on attachment 133580 [details] Patch OK
Comment on attachment 133580 [details] Patch Clearing flags on attachment: 133580 Committed r112178: <http://trac.webkit.org/changeset/112178>
All reviewed patches have been landed. Closing bug.