Bug 101241 - [EFL] [WK2] Random crash in Minibrowser
Summary: [EFL] [WK2] Random crash in Minibrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-05 11:45 PST by Viatcheslav Ostapenko
Modified: 2012-11-06 16:17 PST (History)
7 users (show)

See Also:


Attachments
Check that GL context is valid when display timer gets fired. (2.40 KB, patch)
2012-11-05 11:55 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Rebased patch. (2.37 KB, patch)
2012-11-05 12:43 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Do not clear GL surface and context in exitAcceleratedCompositing. (3.10 KB, patch)
2012-11-05 16:02 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Updated patch. (3.07 KB, patch)
2012-11-05 16:14 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Updated patch by Gyuyoung comments. (3.02 KB, patch)
2012-11-05 22:39 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2012-11-05 11:45:56 PST
Assert in debug build or crash in release build on page load with multiple layers:
ASSERTION FAILED: !HashTranslator::equal(KeyTraits::emptyValue(), key)
/home/sl/work/webkit/Source/WTF/wtf/HashTable.h(588) : void WTF::HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::checkKey(const T&) [with HashTranslator = WTF::IdentityHashTranslator<WTF::PtrHash<void*> >, T = void*, Key = void*, Value = WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*>, Extractor = WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*> >, HashFunctions = WTF::PtrHash<void*>, Traits = WTF::HashMapValueTraits<WTF::HashTraits<void*>, WTF::HashTraits<WebCore::TextureMapperGLData::SharedGLData*> >, KeyTraits = WTF::HashTraits<void*>]
1   0xb41b105b void WTF::HashTable<void*, WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*> >, WTF::PtrHash<void*>, WTF::HashMapValueTraits<WTF::HashTraits<void*>, WTF::HashTraits<WebCore::TextureMapperGLData::SharedGLData*> >, WTF::HashTraits<void*> >::checkKey<WTF::IdentityHashTranslator<WTF::PtrHash<void*> >, void*>(void* const&)
2   0xb41b086a WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*>* WTF::HashTable<void*, WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*> >, WTF::PtrHash<void*>, WTF::HashMapValueTraits<WTF::HashTraits<void*>, WTF::HashTraits<WebCore::TextureMapperGLData::SharedGLData*> >, WTF::HashTraits<void*> >::lookup<WTF::IdentityHashTranslator<WTF::PtrHash<void*> >, void*>(void* const&)
3   0xb41afebe WTF::HashTableIterator<void*, WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*> >, WTF::PtrHash<void*>, WTF::HashMapValueTraits<WTF::HashTraits<void*>, WTF::HashTraits<WebCore::TextureMapperGLData::SharedGLData*> >, WTF::HashTraits<void*> > WTF::HashTable<void*, WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*> >, WTF::PtrHash<void*>, WTF::HashMapValueTraits<WTF::HashTraits<void*>, WTF::HashTraits<WebCore::TextureMapperGLData::SharedGLData*> >, WTF::HashTraits<void*> >::find<WTF::IdentityHashTranslator<WTF::PtrHash<void*> >, void*>(void* const&)
4   0xb41af21b WTF::HashTable<void*, WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<void*, WebCore::TextureMapperGLData::SharedGLData*> >, WTF::PtrHash<void*>, WTF::HashMapValueTraits<WTF::HashTraits<void*>, WTF::HashTraits<WebCore::TextureMapperGLData::SharedGLData*> >, WTF::HashTraits<void*> >::find(void* const&)
5   0xb41ae78d WTF::HashMap<void*, WebCore::TextureMapperGLData::SharedGLData*, WTF::PtrHash<void*>, WTF::HashTraits<void*>, WTF::HashTraits<WebCore::TextureMapperGLData::SharedGLData*> >::find(void* const&)
6   0xb41ae0fc WebCore::TextureMapperGLData::SharedGLData::currentSharedGLData(WebCore::GraphicsContext3D*)
7   0xb41ae4fb WebCore::TextureMapperGLData::TextureMapperGLData(WebCore::GraphicsContext3D*)
8   0xb41a9348 WebCore::TextureMapperGL::TextureMapperGL()
9   0xb41ad805 WebCore::TextureMapperGL::create()
10  0xb41ad765 WebCore::TextureMapper::platformCreateAccelerated()
11  0xb3750d4f WebCore::TextureMapper::create(WebCore::TextureMapper::AccelerationMode)
12  0xb71a5e03 WebKit::LayerTreeRenderer::ensureRootLayer()
13  0xb71a5fb5 WebKit::LayerTreeRenderer::syncRemoteContent()
14  0xb7302a56 WebKit::PageViewportControllerClientEfl::display(WebCore::IntRect const&, WebCore::IntPoint const&)
15  0xb72ceb8d EwkViewImpl::displayTimerFired(WebCore::Timer<EwkViewImpl>*)
16  0xb72d5370 WebCore::Timer<EwkViewImpl>::fired()
17  0xb36d24bf WebCore::ThreadTimers::sharedTimerFiredInternal()
18  0xb36d23e3 WebCore::ThreadTimers::sharedTimerFired()
19  0xb4127453
20  0xb6a7d506 _ecore_timer_expired_call
21  0xb6a7d6c4 _ecore_timer_expired_timers_call
22  0xb6a79fdb
23  0xb6a7a6ef ecore_main_loop_begin
24  0xb6911d14 elm_run
25  0x804c46e elm_main
26  0x804c4b1 main
27  0xb65974d3 __libc_start_main
28  0x804a461
Segmentation fault (core dumped)

It seems that from time to time accelerated compositing get switched off and immediately on. GL context variables gets cleared in EwkViewImpl::exitAcceleratedCompositingMode and recreated in EwkViewImpl::enterAcceleratedCompositingMode , but display timer continues to run and causes crash because of 0 GL context in EwkViewImpl::displayTimerFired .
Comment 1 Viatcheslav Ostapenko 2012-11-05 11:55:47 PST
Created attachment 172374 [details]
Check that GL context is valid when display timer gets fired.
Comment 2 Viatcheslav Ostapenko 2012-11-05 12:43:20 PST
Created attachment 172381 [details]
Rebased patch.
Comment 3 Yael 2012-11-05 13:47:49 PST
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.
Comment 4 Viatcheslav Ostapenko 2012-11-05 14:09:14 PST
(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 5 Chris Dumez 2012-11-05 14:18:49 PST
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
Comment 6 Viatcheslav Ostapenko 2012-11-05 16:02:49 PST
Created attachment 172425 [details]
Do not clear GL surface and context in exitAcceleratedCompositing.

With this patch Minibrowser also survives webprocess crash.
Comment 7 Viatcheslav Ostapenko 2012-11-05 16:04:19 PST
Comment on attachment 172425 [details]
Do not clear GL surface and context in exitAcceleratedCompositing.

Spotted some problem myself.
Comment 8 Viatcheslav Ostapenko 2012-11-05 16:14:10 PST
Created attachment 172428 [details]
Updated patch.
Comment 9 Kenneth Rohde Christiansen 2012-11-05 16:31:01 PST
Comment on attachment 172428 [details]
Updated patch.

Kalyan is refactoring this code. We should wait for his patch.
Comment 10 Gyuyoung Kim 2012-11-05 17:55:55 PST
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
Comment 11 Gyuyoung Kim 2012-11-05 17:57:11 PST
(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 ?
Comment 12 Kalyan 2012-11-05 18:45:49 PST
(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
Comment 13 Viatcheslav Ostapenko 2012-11-05 22:39:01 PST
Created attachment 172491 [details]
Updated patch by  Gyuyoung comments.
Comment 14 WebKit Review Bot 2012-11-06 01:35:15 PST
Comment on attachment 172491 [details]
Updated patch by  Gyuyoung comments.

Clearing flags on attachment: 172491

Committed r133570: <http://trac.webkit.org/changeset/133570>
Comment 15 WebKit Review Bot 2012-11-06 01:35:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Yael 2012-11-06 16:17:44 PST
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.