Bug 165237 – [GTK] The inspector is broken when AC support is disabled

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

[Carlos Alberto Lopez Perez]

If WebKitGTK+ is built without OpenGL support, or it is started with this support disabled via the environment variable WEBKIT_DISABLE_COMPOSITING_MODE, then the inspector window is broken.

Doing right click, inspect element will show a black or empty window and will end crashing the webprocess.

[Michael Gratton]

*** Bug 167218 has been marked as a duplicate of this bug. ***

[Michael Gratton]

Per the duplicate, this is making debugging CSS and JS issues in the WK2 port of Geary somewhat difficult.

[Carlos Garcia Campos]

This is not specific to GTK+, but I guess it doesn't happen in other ports based on coordinated graphics because they don't allow to disable accelerated compositing. The problem is that PageOverlay depends on accelerated compositing and assumes it's always available. We get also crashes if accelerated compositing is not forced when the roo layer is detached, because the page overlay controller keep its graphics layer alive. In coordinated graphics the layers have a coordinator associated, when the coordinator is destroyed, it expects all the layers to be also destroyed, but that's not true for the page olverlays. So, I think we should do two things here:

 1. Do not use page overlays when accelerated compositing is disabled. The same way RenderLayercompositor doesn't create any GraphicsLayer when AC is disabled. Features depending on page overlays will not work of course, but that's better than crashing. I think the GTK+ port only uses page overlays for the inspector highlights, so not a big deal.

 2. When RenderLayerCompositor detaches the root layer, and we leave accelerated compositor mode, the page overlays should be destroyed to ensure we don't leave any GraphicsLayer pointing to a deleted coordindator.

[Michael Catanzaro]

(Also: Geary should really seriously consider disabling JS if at all possible, like Evolution does.)

[Miguel Gomez]

Created attachment 299601 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 299601 [details]
Patch

Thanks for the patch! This looks good to me, r=me but I would like someone more familiarized with the page overlays and compositor code confirms this is ok, so also setting cq-.

[Tim Horton]

Comment on attachment 299601 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:773
>  void RenderLayerCompositor::appendDocumentOverlayLayers(Vector<GraphicsLayer*>& childList)

Is this sufficient? What about view-relative overlays? Maybe they get installed in a different way, I don't really remember.

> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:110
> +    if (!m_page->corePage()->settings().acceleratedCompositingEnabled())
> +        return;

These are just a few of the many places PageOverlays are used; should we perhaps instead proceed with the installation and just not install layers? (And then if a root layer is attached they'll just start working?)

[Carlos Garcia Campos]

(In reply to comment #7)
> Comment on attachment 299601 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299601&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:773
> >  void RenderLayerCompositor::appendDocumentOverlayLayers(Vector<GraphicsLayer*>& childList)
> 
> Is this sufficient? What about view-relative overlays? Maybe they get
> installed in a different way, I don't really remember.

I think this is enough for GTK+ port, I think we only use overlays for the inspector. But this is not really a GTK+ specific issue, it just happens that GTK+ port is probably the only one that still allows to disable AC.

[Tim Horton]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 299601 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=299601&action=review
> > 
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:773
> > >  void RenderLayerCompositor::appendDocumentOverlayLayers(Vector<GraphicsLayer*>& childList)
> > 
> > Is this sufficient? What about view-relative overlays? Maybe they get
> > installed in a different way, I don't really remember.
> 
> I think this is enough for GTK+ port, I think we only use overlays for the
> inspector. But this is not really a GTK+ specific issue, it just happens
> that GTK+ port is probably the only one that still allows to disable AC.

Right, what I should have said was "this seems like a somewhat haphazard way to fix this, maybe instead of checking this bit at the callsites we should check it at the bottleneck inside PageOverlayController instead, so that we never get it wrong in the future and don't have settings checks scattered in random places"

[Miguel Gomez]

> Right, what I should have said was "this seems like a somewhat haphazard way
> to fix this, maybe instead of checking this bit at the callsites we should
> check it at the bottleneck inside PageOverlayController instead, so that we
> never get it wrong in the future and don't have settings checks scattered in
> random places"

Even moving the check to PageOverlayController, which would be an option as you mention, we want to avoid the creation of GraphicsLayers in the caller as well. This is needed in GTK+ because requesting the GraphicsLayerFactory causes AC mode to be enabled. So in any case the callers need to be modified as well. Or am I missing something?

[Tim Horton]

(In reply to comment #10)
> > Right, what I should have said was "this seems like a somewhat haphazard way
> > to fix this, maybe instead of checking this bit at the callsites we should
> > check it at the bottleneck inside PageOverlayController instead, so that we
> > never get it wrong in the future and don't have settings checks scattered in
> > random places"
> 
> Even moving the check to PageOverlayController, which would be an option as
> you mention, we want to avoid the creation of GraphicsLayers in the caller
> as well. This is needed in GTK+ because requesting the GraphicsLayerFactory
> causes AC mode to be enabled. So in any case the callers need to be modified
> as well. Or am I missing something?

Ah, fair point, I didn't realize the inspector made sublayers, but it does. Carry on!

[WebKit Commit Bot]

Comment on attachment 299601 [details]
Patch

Clearing flags on attachment: 299601

Committed r211141: <http://trac.webkit.org/changeset/211141>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.