Bug 81954 – [Chromium] Using WebViewPlugins with --force-compositing-mode causes an ASSERT to fail

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Fady Samuel]

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?

[James Robinson]

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?

[Fady Samuel]

(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.

[Bernhard Bauer]

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.

[Fady Samuel]

(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.

[Fady Samuel]

(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.

[Fady Samuel]

(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?

[James Robinson]

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.

[Dana Jansens]

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));
}

[Fady Samuel]

Created attachment 133492 [details]
Patch

[Fady Samuel]

Created attachment 133493 [details]
Patch

[Dana Jansens]

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?

[Fady Samuel]

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.

[Fady Samuel]

(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.

[Fady Samuel]

Created attachment 133504 [details]
Patch

[Dana Jansens]

Comment on attachment 133504 [details]
Patch

lgtm. Does this work for you James?

[James Robinson]

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?)

[Fady Samuel]

Created attachment 133580 [details]
Patch

[Fady Samuel]

(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.

[James Robinson]

Comment on attachment 133580 [details]
Patch

OK

[WebKit Review Bot]

Comment on attachment 133580 [details]
Patch

Clearing flags on attachment: 133580

Committed r112178: <http://trac.webkit.org/changeset/112178>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.