| Summary: | [WK2][GTK] ASSERTION in WebKit::LayerTreeHostGtk::invalidate | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||||||||||
| Component: | WebKitGTK | Assignee: | Alberto Garcia <berto> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | alex, berto, cgarcia, commit-queue, gustavo, mrobinson, svillar | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| URL: | http://praza.com/movementos-sociais/4872/o-pp-rexeita-abolir-as-touradas-porque-non-hai-risco-de-que-proliferen-en-galicia/ | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Sergio Villar Senin
2013-06-18 03:40:47 PDT
I can reproduce this always just by visiting the URL I pasted above in the URL field. Reproduced, I'll take a look. LayerTreeHostGtk::invalidate() is called from DrawingAreaImpl::exitAcceleratedCompositingMode() but m_isValid is false because the call to glContext() fails when LayerTreeHost is initialized. 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().
(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. (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. (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). (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. (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. (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(). (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. (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. Created attachment 206264 [details]
Patch
Second attempt.
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 Created attachment 206265 [details]
Patch
Oops, there was a hunk missing in the previous patch.
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? (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. 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. 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.
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? (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. 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.
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...
(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)? (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. Committed r155066: <http://trac.webkit.org/changeset/155066> |