Bug 117733 – [WK2][GTK] ASSERTION in WebKit::LayerTreeHostGtk::invalidate

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Sergio Villar Senin]

From time to time I hit this assertion:

ASSERTION FAILED: m_isValid
../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp(171) : virtual void WebKit::LayerTreeHostGtk::invalidate()
1   0x7f130553e7c7 /home/sergio/opt/gnome3/lib64/libjavascriptcoregtk-3.0.so.0(WTFCrash+0x1e) [0x7f130553e7c7]
2   0x7f130034fac3 /home/sergio/opt/gnome3/lib64/libwebkit2gtk-3.0.so.25(+0x81bac3) [0x7f130034fac3]
3   0x7f1300304458 /home/sergio/opt/gnome3/lib64/libwebkit2gtk-3.0.so.25(+0x7d0458) [0x7f1300304458]
4   0x7f1300306ada /home/sergio/opt/gnome3/lib64/libwebkit2gtk-3.0.so.25(+0x7d2ada) [0x7f1300306ada]
5   0x7f13019ac8ce /home/sergio/opt/gnome3/lib64/libwebkit2gtk-3.0.so.25(+0x1e788ce) [0x7f13019ac8ce]
6   0x7f12fcb16b5e /home/sergio/opt/gnome3/lib64/libglib-2.0.so.0(+0x57b5e) [0x7f12fcb16b5e]
7   0x7f12fcb14e01 /home/sergio/opt/gnome3/lib64/libglib-2.0.so.0(+0x55e01) [0x7f12fcb14e01]
8   0x7f12fcb15b66 /home/sergio/opt/gnome3/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x33) [0x7f12fcb15b66]
9   0x7f12fcb15d56 /home/sergio/opt/gnome3/lib64/libglib-2.0.so.0(+0x56d56) [0x7f12fcb15d56]
10  0x7f12fcb16186 /home/sergio/opt/gnome3/lib64/libglib-2.0.so.0(g_main_loop_run+0x1d8) [0x7f12fcb16186]
11  0x7f13019ac46a /home/sergio/opt/gnome3/lib64/libwebkit2gtk-3.0.so.25(+0x1e7846a) [0x7f13019ac46a]
12  0x7f130025da5c /home/sergio/opt/gnome3/lib64/libwebkit2gtk-3.0.so.25(WebProcessMainGtk+0x114) [0x7f130025da5c]
13  0x4009bc /home/sergio/opt/gnome3/libexec/WebKitWebProcess(main+0x20) [0x4009bc]
14  0x7f12fc30fa55 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7f12fc30fa55]
15  0x4008d9 /home/sergio/opt/gnome3/libexec/WebKitWebProcess() [0x4008d9]

[Sergio Villar Senin]

I can reproduce this always just by visiting the URL I pasted above in the URL field.

[Alberto Garcia]

Reproduced, I'll take a look.

[Alberto Garcia]

LayerTreeHostGtk::invalidate() is called from
DrawingAreaImpl::exitAcceleratedCompositingMode() but m_isValid is
false because the call to glContext() fails when LayerTreeHost is
initialized.

[Alberto Garcia]

Created attachment 205891 [details]
Patch

I think that m_isValid is incorrectly set to false if the GL context
cannot be created (which happens if e.g. the display does not support
XDamage or Xcomposite).

That doesn't really do anything other than breaking the assertion in
LayerTreeHostGtk::invalidate().

[Sergio Villar Senin]

(In reply to comment #4)
> Created an attachment (id=205891) [details]
> Patch
> 
> I think that m_isValid is incorrectly set to false if the GL context
> cannot be created (which happens if e.g. the display does not support
> XDamage or Xcomposite).

I don't think this is the case at least in my environment. Note that it's set to false also at the end of ::invalidate()
 
> That doesn't really do anything other than breaking the assertion in
> LayerTreeHostGtk::invalidate().

This is not true, in that case ::flushAndRederLayers will early return.

So my guess is that for whatever reason ::invalidate() is incorrectly called twice in a row.

[Alberto Garcia]

(In reply to comment #5)
> > I think that m_isValid is incorrectly set to false if the GL
> > context cannot be created (which happens if e.g. the display does
> > not support XDamage or Xcomposite).
> I don't think this is the case at least in my environment. Note that
> it's set to false also at the end of ::invalidate()

Maybe you're hitting a different problem then, but I could definitely
reproduce it as I explained.

Did you manage to reproduce it again in your environment?

> > That doesn't really do anything other than breaking the assertion in
> > LayerTreeHostGtk::invalidate().
>
> This is not true, in that case ::flushAndRederLayers will early
> return.

It would do it anyway because the call to glContext() immediately
after that would fail as well.

> So my guess is that for whatever reason ::invalidate() is
> incorrectly called twice in a row.

I can't see how that would happen, m_layerTreeHost is set to null
immediately after invalidating it in DrawingAreaImpl.

[Sergio Villar Senin]

(In reply to comment #6)
> (In reply to comment #5)
> > > I think that m_isValid is incorrectly set to false if the GL
> > > context cannot be created (which happens if e.g. the display does
> > > not support XDamage or Xcomposite).
> > I don't think this is the case at least in my environment. Note that
> > it's set to false also at the end of ::invalidate()
> 
> Maybe you're hitting a different problem then, but I could definitely
> reproduce it as I explained.
> 
> Did you manage to reproduce it again in your environment?

What I mean is that my environment does support XDamage and XComposite, but I don't deny that the context creation may fail for some other reasons.

Our implementation is quite close to the Mac one, and it seems that we're maybe misusing m_isValid. The original purpose of that flag is to avoid flushes after an invalidation. We're setting it (incorrectly IMO) to false to track failures in the GLContext creation. So I indeed believe that your change is totally right (the flush won't happen anyway as you say if the context cannot be created).

[Martin Robinson]

(In reply to comment #4)
> Created an attachment (id=205891) [details]
> Patch
> 
> I think that m_isValid is incorrectly set to false if the GL context
> cannot be created (which happens if e.g. the display does not support
> XDamage or Xcomposite).
> 
> That doesn't really do anything other than breaking the assertion in
> LayerTreeHostGtk::invalidate().

Perhaps the solution for this is to check host->m_isValid in LayerTreeHostGtk::create and return null from that factory if it's false.

[Alberto Garcia]

(In reply to comment #8)
> Perhaps the solution for this is to check host->m_isValid in
> LayerTreeHostGtk::create and return null from that factory if it's
> false.

But we still need to invalidate and destroy the object, so we should
probably attempt to create the glContext() at the beginning of the
initialize method then.

[Martin Robinson]

(In reply to comment #9)
> (In reply to comment #8)
> > Perhaps the solution for this is to check host->m_isValid in
> > LayerTreeHostGtk::create and return null from that factory if it's
> > false.
> 
> But we still need to invalidate and destroy the object, so we should
> probably attempt to create the glContext() at the beginning of the
> initialize method then.

I don't think we need to explicitly invalidate the object because when the RefPtr goes out of scope in LayerTreeHostGtk::create, the destructor will clean up its members. The only reason this doesn't happen now is because we call host.release().

[Alberto Garcia]

(In reply to comment #10)
> > But we still need to invalidate and destroy the object, so we
> > should probably attempt to create the glContext() at the beginning
> > of the initialize method then.
> I don't think we need to explicitly invalidate the object because
> when the RefPtr goes out of scope in LayerTreeHostGtk::create, the
> destructor will clean up its members. The only reason this doesn't
> happen now is because we call host.release().

We do because of the ASSERT(!m_rootLayer) in the destructor.

DrawingAreaImpl::enterAcceleratedCompositingMode() is also not
considering the possibility that LayerTreeHost::create() returns 0.

So there's a few things we need to look into.

[Martin Robinson]

(In reply to comment #11)
> (In reply to comment #10)
> > > But we still need to invalidate and destroy the object, so we
> > > should probably attempt to create the glContext() at the beginning
> > > of the initialize method then.
> > I don't think we need to explicitly invalidate the object because
> > when the RefPtr goes out of scope in LayerTreeHostGtk::create, the
> > destructor will clean up its members. The only reason this doesn't
> > happen now is because we call host.release().
> 
> We do because of the ASSERT(!m_rootLayer) in the destructor.

There's no reason the assertion has to remain, if the situation is no longer unexpected. :)

> DrawingAreaImpl::enterAcceleratedCompositingMode() is also not
> considering the possibility that LayerTreeHost::create() returns 0.

This seems to be a bigger issue. It seems the code has changed a bit since I originally wrote it, as returning null would disable AC implicitly. It's probably safer to check during initialization if the system supports XComposite and XDamage and force the page settings to disable AC if not.

[Alberto Garcia]

Created attachment 206264 [details]
Patch

Second attempt.

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alberto Garcia]

Created attachment 206265 [details]
Patch

Oops, there was a hunk missing in the previous patch.

[Martin Robinson]

Comment on attachment 206265 [details]
Patch

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

The bits in WebKitWebView look good to me, but I think any fixes for LayerTreeHost should be split out into another patch. I think given your comments earlier that the approach of returning null isn't a safe one (as the assertion you added attests). I think we are just going to have to make sure that we do not return an invalid LayerTreeHost.

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:70
> -    return host.release();
> +    return host->m_isValid ? host.release() : 0;
>  }
>  

It seems that we could actually dispense with this part of the patch entirely?

[Alberto Garcia]

(In reply to comment #16)
> The bits in WebKitWebView look good to me, but I think any fixes for
> LayerTreeHost should be split out into another patch. I think given
> your comments earlier that the approach of returning null isn't a
> safe one (as the assertion you added attests). I think we are just
> going to have to make sure that we do not return an invalid
> LayerTreeHost.

The situation here is that, with the changes in WebKitWebView, the
scenario where LayerTreeHost fails to get the GL context is not
supposed to happen, ever.

So that change alone would be enough to fix the crash.

The other part of the patch just tries to fix the inconsistencies in
LayerTreeHost.

If we still want to return a non-null LayerTreeHost we can do
something like what I proposed in my first patch (comment 4). The code
already deals with the scenario where glContext() returns null, it was
only the assertion that was causing problems (that's why it only
crashed in debug builds).

Still I think it's a good idea to detect when we're incorrectly
attempting to enter AC mode, because that's not supposed to happen.

[Martin Robinson]

Sure. All that makes sense to me. I think these two things are just turning into two patches as they become two separate topics: fixing non-xcomposite situations and fixing the more general issues.

[Alberto Garcia]

Created attachment 206323 [details]
Patch

Okay, this patch disables AC if the system doesn't support it. This fixes the crash.

Later I can file a separate bug to fix the LayerTreeHost thing.

[Martin Robinson]

Comment on attachment 206323 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:383
> +#if USE(TEXTURE_MAPPER_GL)
> +    if (!webkitWebViewBaseHasRedirectedWindow(WEBKIT_WEB_VIEW_BASE(webView)))
> +        webkitSettingsGetPreferences(settings)->setAcceleratedCompositingEnabled(false);
> +#endif

I'm realizing now that this might be an issue for WebKitTestRunner since it's implemented using WebkitWebViewBase instead of WebKitWebView. Is there any way to accomplish this in WebKitWebViewBase only?

[Alberto Garcia]

(In reply to comment #20)
> I'm realizing now that this might be an issue for WebKitTestRunner
> since it's implemented using WebkitWebViewBase instead of
> WebKitWebView. Is there any way to accomplish this in
> WebKitWebViewBase only?

That would be even better because all the redirectedWindow handling is
made in WebKitWebViewBase already. However I didn't see the way to do
it easily. I'll give it one more try.

[Alberto Garcia]

Created attachment 210362 [details]
Patch using WebKitWebViewBase

Here's the implementation using WebKitWebViewBase.

Still we need to touch code from WebKitWebView to make sure that AC is
disabled if the settings are changed.

[Martin Robinson]

Comment on attachment 210362 [details]
Patch using WebKitWebViewBase

Isn't it sufficient to do this in webkitWebViewGroupAttachSettingsToPageGroup, which is the other place that we disable accelerated compositing? If not, then it seems the Wayland path is wrong...

[Alberto Garcia]

(In reply to comment #23)
> Isn't it sufficient to do this in webkitWebViewGroupAttachSettingsToPageGroup

How is it different from doing it in WebKitWebView (see comment #20)?

[Martin Robinson]

(In reply to comment #24)
> (In reply to comment #23)
> > Isn't it sufficient to do this in webkitWebViewGroupAttachSettingsToPageGroup
> 
> How is it different from doing it in WebKitWebView (see comment #20)?

Oh yes! Good point. I think the Wayland check probably needs to move to WebKitWebViewBase as well.

[Alberto Garcia]

Committed r155066: <http://trac.webkit.org/changeset/155066>