| Summary: | [EFL] [WK2] Random crash in Minibrowser | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Viatcheslav Ostapenko <ostap73> |
| Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | cdumez, gyuyoung.kim, kalyan.kondapally, lucas.de.marchi, rakuco, webkit.review.bot, yael |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
|
Description
Viatcheslav Ostapenko
2012-11-05 11:45:56 PST
Created attachment 172374 [details]
Check that GL context is valid when display timer gets fired.
Created attachment 172381 [details]
Rebased patch.
Comment on attachment 172381 [details] Rebased patch. View in context: https://bugs.webkit.org/attachment.cgi?id=172381&action=review We should not be switching AC off and on as you are describing. IMO, the proper solution is to move the initialization and cleanup code out of EnterAcceleratedCompositing/ExitAcceleratedCompositing. No other port implements these methods. > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:194 > + Evas_GL* evasGL() const { return m_evasGL.get(); } > + Evas_GL_Context* evasGLContext() const { return m_evasGLContext ? m_evasGLContext->context() : 0; } > + Evas_GL_Surface* evasGLSurface() const { return m_evasGLSurface ? m_evasGLSurface->surface() : 0; } Adding these const is against the latest webkit coding style. (In reply to comment #3) > (From update of attachment 172381 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172381&action=review > > We should not be switching AC off and on as you are describing. It also happens if webprocess crashes. I've spent some time to figure out why it switches of AC in webprocess, but it is difficult to reproduce. I was able to get is several times clicking on demos on this page: http://webdesignerwall.com/trends/47-amazing-css3-animation-demos With debugger attached to webprocess I was not able to reproduce it. >IMO, the proper solution is to move the initialization and cleanup code out of EnterAcceleratedCompositing/ExitAcceleratedCompositing. No other port implements these methods. > > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:194 > > + Evas_GL* evasGL() const { return m_evasGL.get(); } > > + Evas_GL_Context* evasGLContext() const { return m_evasGLContext ? m_evasGLContext->context() : 0; } > > + Evas_GL_Surface* evasGLSurface() const { return m_evasGLSurface ? m_evasGLSurface->surface() : 0; } > > Adding these const is against the latest webkit coding style. Wow. Do you have some reference? At least style checker didn't complain ;) Those "const" mean that method don't change object and optimizer can do something with them. Previously there was single call to those method in function, but now I've added 2nd one, so it is better go give some hint to compiler. Comment on attachment 172381 [details] Rebased patch. View in context: https://bugs.webkit.org/attachment.cgi?id=172381&action=review >>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:194 >>> + Evas_GL_Surface* evasGLSurface() const { return m_evasGLSurface ? m_evasGLSurface->surface() : 0; } >> >> Adding these const is against the latest webkit coding style. > > Wow. > Do you have some reference? > At least style checker didn't complain ;) > Those "const" mean that method don't change object and optimizer can do something with them. Previously there was single call to those method in function, but now I've added 2nd one, so it is better go give some hint to compiler. http://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html Created attachment 172425 [details]
Do not clear GL surface and context in exitAcceleratedCompositing.
With this patch Minibrowser also survives webprocess crash.
Comment on attachment 172425 [details]
Do not clear GL surface and context in exitAcceleratedCompositing.
Spotted some problem myself.
Created attachment 172428 [details]
Updated patch.
Comment on attachment 172428 [details]
Updated patch.
Kalyan is refactoring this code. We should wait for his patch.
Comment on attachment 172428 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=172428&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:550 > + EINA_LOG_DOM_WARN(_ewk_log_dom, "Failed to create GLSurface."); Please use WARN. We already link this one to WARN macro in ewk_private.h (In reply to comment #9) > (From update of attachment 172428 [details]) > Kalyan is refactoring this code. We should wait for his patch. Did he file a bug for the refactoring ? (In reply to comment #9) > (From update of attachment 172428 [details]) > Kalyan is refactoring this code. We should wait for his patch. My changes dont touch this.I have created a bug for my Changes: https://bugs.webkit.org/show_bug.cgi?id=101291 Created attachment 172491 [details]
Updated patch by Gyuyoung comments.
Comment on attachment 172491 [details] Updated patch by Gyuyoung comments. Clearing flags on attachment: 172491 Committed r133570: <http://trac.webkit.org/changeset/133570> All reviewed patches have been landed. Closing bug. Comment on attachment 172491 [details] Updated patch by Gyuyoung comments. View in context: https://bugs.webkit.org/attachment.cgi?id=172491&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:502 > bool EwkViewImpl::createGLSurface(const IntSize& viewSize) This function is no longer just for creating a surface. It should have been renamed accordingly. |