Bug 101241 – [EFL] [WK2] Random crash in Minibrowser

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

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 .

[Viatcheslav Ostapenko]

Created attachment 172374 [details]
Check that GL context is valid when display timer gets fired.

[Viatcheslav Ostapenko]

Created attachment 172381 [details]
Rebased patch.

[Yael]

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.

[Viatcheslav Ostapenko]

(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.

[Chris Dumez]

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

[Viatcheslav Ostapenko]

Created attachment 172425 [details]
Do not clear GL surface and context in exitAcceleratedCompositing.

With this patch Minibrowser also survives webprocess crash.

[Viatcheslav Ostapenko]

Comment on attachment 172425 [details]
Do not clear GL surface and context in exitAcceleratedCompositing.

Spotted some problem myself.

[Viatcheslav Ostapenko]

Created attachment 172428 [details]
Updated patch.

[Kenneth Rohde Christiansen]

Comment on attachment 172428 [details]
Updated patch.

Kalyan is refactoring this code. We should wait for his patch.

[Gyuyoung Kim]

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

[Gyuyoung Kim]

(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 ?

[Kalyan]

(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

[Viatcheslav Ostapenko]

Created attachment 172491 [details]
Updated patch by  Gyuyoung comments.

[WebKit Review Bot]

Comment on attachment 172491 [details]
Updated patch by  Gyuyoung comments.

Clearing flags on attachment: 172491

Committed r133570: <http://trac.webkit.org/changeset/133570>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

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.