Bug 86961 – [EFL] Check if ecore_x is initialised before calling ecore_x_bell to avoid crash

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Sudarsana Nagineni (babu)]

editing/execCommand/paste-and-match-style-event.html crashes on EFL port

#0  XBell (dpy=0x0, percent=0) at ../../src/Bell.c:39
39      ../../src/Bell.c: No such file or directory.
(gdb) bt
#0  XBell (dpy=0x0, percent=0) at ../../src/Bell.c:39
#1  0x00007ffe09f69e45 in ecore_x_bell (percent=<optimized out>) at ecore_x.c:987
#2  0x00007ffe0f27313e in WebCore::systemBeep () at /WebKit/Source/WebCore/platform/efl/SoundEfl.cpp:42
#3  0x00007ffe0e514a6f in WebCore::Editor::copy (this=0x24c0c20) at /WebKit/Source/WebCore/editing/Editor.cpp:1002
#4  0x00007ffe0e524118 in WebCore::executeCopy (frame=0x24c06c0) at /WebKit/Source/WebCore/editing/EditorCommand.cpp:287
#5  0x00007ffe0e5282d1 in WebCore::Editor::Command::execute (this=0x7ffff81e2620, parameter="(null)", triggeringEvent=0x0)
    at /WebKit/Source/WebCore/editing/EditorCommand.cpp:1690
#6  0x00007ffe0e3f42ac in WebCore::Document::execCommand (this=0x25d1c00, commandName="Copy", userInterface=false, value="(null)")
    at /WebKit/Source/WebCore/dom/Document.cpp:4472
#7  0x00007ffe0ef3c91b in WebCore::jsDocumentPrototypeFunctionExecCommand (exec=0x7ffdbecd9108)
    at /WebKit/WebKitBuild/Debug/DerivedSources/WebCore/JSDocument.cpp:2504
#8  0x00007ffdc0362265 in ?? ()
#9  0x00007ffff81e27e0 in ?? ()
#10 0x00007ffdc0362f23 in ?? ()
#11 0x00007ffff81e2760 in ?? ()
#12 0x00007ffdbe44f6a0 in ?? ()
#13 0x00000000025cae40 in ?? ()
#14 0x00007ffff81e27b0 in ?? ()
#15 0x00007ffdbe3ccdc0 in ?? ()
#16 0x00007ffe122a6c8d in JSC::Register::Register (this=0x7ffe0a0cd960) at /WebKit/Source/JavaScriptCore/interpreter/Register.h:105
#17 0x00007ffe1240990a in JSC::JITCode::execute (this=0x7ffdbe389558, registerFile=0x25ac518, callFrame=0x7ffdbecd9040, globalData=0x25b5af0)
    at /WebKit/Source/JavaScriptCore/jit/JITCode.h:127
#18 0x00007ffe12406346 in JSC::Interpreter::executeCall (this=0x25ac500, callFrame=0x7ffdbe42fae0, function=0x7ffdbe3ccdc0, callType=JSC::CallTypeJS,
    callData=..., thisValue=..., args=...) at /WebKit/Source/JavaScriptCore/interpreter/Interpreter.cpp:1304
#19 0x00007ffe124ba54c in JSC::call (exec=0x7ffdbe42fae0, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...)
    at /WebKit/Source/JavaScriptCore/runtime/CallData.cpp:39
#20 0x00007ffe0ee4b520 in WebCore::JSMainThreadExecState::call (exec=0x7ffdbe42fae0, functionObject=..., callType=JSC::CallTypeJS, callData=...,
    thisValue=..., args=...) at /WebKit/Source/WebCore/bindings/js/JSMainThreadExecState.h:56
#21 0x00007ffe0ee70baa in WebCore::JSEventListener::handleEvent (this=0x294e940, scriptExecutionContext=0x25d1d30, event=0x250b880)
    at /WebKit/Source/WebCore/bindings/js/JSEventListener.cpp:133
#22 0x00007ffe0e45fa5a in WebCore::EventTarget::fireEventListeners (this=0x24fdaf0, event=0x250b880, d=0x24fdc28,
    entry=WTF::Vector of length 1, capacity 1 = {...}) at /WebKit/Source/WebCore/dom/EventTarget.cpp:231
#23 0x00007ffe0e45f8b4 in WebCore::EventTarget::fireEventListeners (this=0x24fdaf0, event=0x250b880)
    at /WebKit/Source/WebCore/dom/EventTarget.cpp:198
#24 0x00007ffe0e82ce8c in WebCore::DOMWindow::dispatchEvent (this=0x24fdaf0, prpEvent=..., prpTarget=...)
    at /WebKit/Source/WebCore/page/DOMWindow.cpp:1642
#25 0x00007ffe0e82cc03 in WebCore::DOMWindow::dispatchLoadEvent (this=0x24fdaf0) at /WebKit/Source/WebCore/page/DOMWindow.cpp:1616
---Type <return> to continue, or q <return> to quit---
#26 0x00007ffe0e3f28bf in WebCore::Document::dispatchWindowLoadEvent (this=0x25d1c00) at /WebKit/Source/WebCore/dom/Document.cpp:3983
#27 0x00007ffe0e3ecdfb in WebCore::Document::implicitClose (this=0x25d1c00) at /WebKit/Source/WebCore/dom/Document.cpp:2445
#28 0x00007ffe0e77d583 in WebCore::FrameLoader::checkCallImplicitClose (this=0x24c0758) at /WebKit/Source/WebCore/loader/FrameLoader.cpp:761
#29 0x00007ffe0e77d339 in WebCore::FrameLoader::checkCompleted (this=0x24c0758) at /WebKit/Source/WebCore/loader/FrameLoader.cpp:707
#30 0x00007ffe0e77d11c in WebCore::FrameLoader::loadDone (this=0x24c0758) at /WebKit/Source/WebCore/loader/FrameLoader.cpp:653
#31 0x00007ffe0e7eed43 in WebCore::CachedResourceLoader::loadDone (this=0x250c620)
    at /WebKit/Source/WebCore/loader/cache/CachedResourceLoader.cpp:663
#32 0x00007ffe0e7aea5c in WebCore::SubresourceLoader::releaseResources (this=0x25df550)
    at /WebKit/Source/WebCore/loader/SubresourceLoader.cpp:317
#33 0x00007ffe0e7aa1f2 in WebCore::ResourceLoader::didFinishLoading (this=0x25df550, finishTime=0)
    at /WebKit/Source/WebCore/loader/ResourceLoader.cpp:298
#34 0x00007ffe0e7ae649 in WebCore::SubresourceLoader::didFinishLoading (this=0x25df550, finishTime=0)
    at /WebKit/Source/WebCore/loader/SubresourceLoader.cpp:278
#35 0x00007ffe0e7aa967 in WebCore::ResourceLoader::didFinishLoading (this=0x25df550, finishTime=0)
    at /WebKit/Source/WebCore/loader/ResourceLoader.cpp:435
#36 0x00007ffe0f28b43f in WebCore::readCallback (source=0x24a5ea0, asyncResult=0x24a0920, data=0x25c6960)
    at /WebKit/Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:864
#37 0x00007ffe08996a99 in async_ready_callback_wrapper (source_object=0x24a5ea0, res=0x24a0920, user_data=0x25c6960) at ginputstream.c:470
#38 0x00007ffe089a90e7 in g_simple_async_result_complete (simple=0x24a0920) at gsimpleasyncresult.c:767
#39 0x00007ffe089a9168 in complete_in_idle_cb_for_thread (_data=0x24d13c0) at gsimpleasyncresult.c:835
#40 0x00007ffe09ff5623 in g_main_dispatch (context=0xffff000000000002) at gmain.c:2539
#41 g_main_context_dispatch (context=0xffff000000000002) at gmain.c:3075
#42 0x00007ffe12b275e1 in _ecore_glib_select__locked (ecore_timeout=0x25f5110, efds=<optimized out>, wfds=<optimized out>, rfds=<optimized out>,
    ecore_fds=1, ctx=<optimized out>) at ecore_glib.c:171
#43 _ecore_glib_select (ecore_fds=1, rfds=<optimized out>, wfds=<optimized out>, efds=<optimized out>, ecore_timeout=0x25f5110) at ecore_glib.c:205
#44 0x00007ffe12b219ad in _ecore_main_select (timeout=<optimized out>) at ecore_main.c:1419
#45 0x00007ffe12b22445 in _ecore_main_loop_iterate_internal (once_only=0) at ecore_main.c:1835
#46 0x00007ffe12b22727 in ecore_main_loop_begin () at ecore_main.c:906
#47 0x00000000004620c5 in runTest (cTestPathOrURL=0x7ffff81e3770 "/WebKit/LayoutTests/editing/execCommand/paste-and-match-style-event.html")
    at /WebKit/Tools/DumpRenderTree/efl/DumpRenderTree.cpp:249
#48 0x000000000046223e in runTestingServerLoop () at /WebKit/Tools/DumpRenderTree/efl/DumpRenderTree.cpp:275
#49 0x000000000046288e in main (argc=2, argv=0x7ffff81e48c8) at /WebKit/Tools/DumpRenderTree/efl/DumpRenderTree.cpp:411
(gdb)

[Chris Dumez]

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

[Chris Dumez]

ecore_x_init() needs to be called in the ewk_main. Will upload a patch for this.

[Chris Dumez]

Created attachment 145041 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 145041 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_main.cpp:40
> +#ifdef HAVE_ECORE_X

Generally, WebKit doesn't puts #ifdef ~ #endif statement in include file list.

- http://trac.webkit.org/browser/trunk/Source/WebCore/platform/efl/WidgetEfl.cpp#L49
- http://trac.webkit.org/browser/trunk/Source/WebCore/platform/efl/EflScreenUtilities.cpp#L22

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 145041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145041&action=review
> 
> > Source/WebKit/efl/ewk/ewk_main.cpp:40
> > +#ifdef HAVE_ECORE_X
> 
> Generally, WebKit doesn't puts #ifdef ~ #endif statement in include file list.
> 
> - http://trac.webkit.org/browser/trunk/Source/WebCore/platform/efl/WidgetEfl.cpp#L49
> - http://trac.webkit.org/browser/trunk/Source/WebCore/platform/efl/EflScreenUtilities.cpp#L22

Gyuyoung, I don't understand because in the 2 examples you provide, #ifdef is used for includes and this is not my code. For the record, I used Source/WebCore/platform/efl/SoundEfl.cpp as reference which does the following:

#ifdef HAVE_ECORE_X
#include <Ecore_X.h>
#endif

[Chris Dumez]

Created attachment 145042 [details]
Patch

Small fix in #ifdef placement. I'm still keeping the #ifdef for the include though. This is a header that is external to WebKit.

[Gyuyoung Kim]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 145041 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=145041&action=review
> > 
> > > Source/WebKit/efl/ewk/ewk_main.cpp:40
> > > +#ifdef HAVE_ECORE_X
> > 
> > Generally, WebKit doesn't puts #ifdef ~ #endif statement in include file list.
> > 
> > - http://trac.webkit.org/browser/trunk/Source/WebCore/platform/efl/WidgetEfl.cpp#L49
> > - http://trac.webkit.org/browser/trunk/Source/WebCore/platform/efl/EflScreenUtilities.cpp#L22
> 
> Gyuyoung, I don't understand because in the 2 examples you provide, #ifdef is used for includes and this is not my code. For the record, I used Source/WebCore/platform/efl/SoundEfl.cpp as reference which does the following:
> 
> #ifdef HAVE_ECORE_X
> #include <Ecore_X.h>
> #endif

Look at Internals.cpp file or webkitwebview.cpp.  #ifdef ~ #else in include list is independently placed.

 - http://trac.webkit.org/browser/trunk/Source/WebCore/testing/Internals.cpp#L66
 - http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/webkit/webkitwebview.cpp#L117

Though it seems this is not rule, I prefer this to mix #ifdef. In addition, it seems to me WebKit tends to use my suggestion. Of course, this is trivial issue.

[Chris Dumez]

Created attachment 145046 [details]
Patch

Ok, I think I understood what you meant now. Hopefully, this new patch is what you wanted.

[Gyuyoung Kim]

Comment on attachment 145046 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_main.cpp:50
> +#ifdef HAVE_ECORE_X

Sorry for my poor comment. This is what I want exactly.

> Source/WebKit/efl/ewk/ewk_main.cpp:115
> +error_ecore_x:

BTW, doesn't ecore_x_shutdown() needs to be called in *error_ecore_x* case instead of *error_edje* case?

[Chris Dumez]

Comment on attachment 145046 [details]
Patch

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

>> Source/WebKit/efl/ewk/ewk_main.cpp:115
>> +error_ecore_x:
> 
> BTW, doesn't ecore_x_shutdown() needs to be called in *error_ecore_x* case instead of *error_edje* case?

No, I believe this is correct. Look at the other labels under, they do the same. There is no need to shutdown ecore x if it did not init.

[Gyuyoung Kim]

Comment on attachment 145046 [details]
Patch

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

>>> Source/WebKit/efl/ewk/ewk_main.cpp:115
>>> +error_ecore_x:
>> 
>> BTW, doesn't ecore_x_shutdown() needs to be called in *error_ecore_x* case instead of *error_edje* case?
> 
> No, I believe this is correct. Look at the other labels under, they do the same. There is no need to shutdown ecore x if it did not init.

I don't understand yet. If ecore_x_init(0) fails to initialize, go to error_ecore_x label, right? Then, It seems to me error_ecore_x label needs to invoke ecore_x_shutdown() as below,

error_ecore_x:
    ecore_x_shutdown();

But, current patch calls ecore_evas_shutdown() instead of ecore_x_shutdown() when HAVE_ECORE_X is enabled. 

Let's assume there is no #ifdef HAVE_ECORE_X,

error_edje:
    ecore_x_shutdown();
error_ecore_x:
    ecore_evas_shutdown();

Is above correct ?

[Chris Dumez]

(In reply to comment #11)
> (From update of attachment 145046 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145046&action=review
> 
> >>> Source/WebKit/efl/ewk/ewk_main.cpp:115
> >>> +error_ecore_x:
> >> 
> >> BTW, doesn't ecore_x_shutdown() needs to be called in *error_ecore_x* case instead of *error_edje* case?
> > 
> > No, I believe this is correct. Look at the other labels under, they do the same. There is no need to shutdown ecore x if it did not init.
> 
> I don't understand yet. If ecore_x_init(0) fails to initialize, go to error_ecore_x label, right? Then, It seems to me error_ecore_x label needs to invoke ecore_x_shutdown() as below,
> 
> error_ecore_x:
>     ecore_x_shutdown();
> 
> But, current patch calls ecore_evas_shutdown() instead of ecore_x_shutdown() when HAVE_ECORE_X is enabled. 
> 
> Let's assume there is no #ifdef HAVE_ECORE_X,
> 
> error_edje:
>     ecore_x_shutdown();
> error_ecore_x:
>     ecore_evas_shutdown();
> 
> Is above correct ?

Yes, it is.

Looks at other labels:
error_ecore:
    evas_shutdown();
error_evas: // NOT evas_shutdown()
    eina_log_domain_unregister(_ewk_log_dom);
    _ewk_log_dom = -1;
error_log_domain:
    eina_shutdown(); // NOT eina_log_domain_unregister()
error_eina:
    return 0; // NOT eina_shutdown().

All the cleanup functions are shifted by 1 compared the the labels.  The reason for that is:
If the code jumps to "error_evas" label, this means that evas_init() failed. As a consequence, there is no point in calling evas_shutdown(). However, we need to call the shutdown functions for all the previous init calls (i.e. eina_log_domain_register and eina init.

[Gyuyoung Kim]

(In reply to comment #12)

> All the cleanup functions are shifted by 1 compared the the labels.  The reason for that is:
> If the code jumps to "error_evas" label, this means that evas_init() failed. As a consequence, there is no point in calling evas_shutdown(). However, we need to call the shutdown functions for all the previous init calls (i.e. eina_log_domain_register and eina init.

Ok, I understand this. Thank you for your explanation. But, I don't like to goto ~ case statement. As in this case, it is a little difficult to understand algorithm logic. I would like to remove this goto statement in future. Anyway, looks good to me now. Thanks.

[Thiago Marcos P. Santos]

LGTM but might introduce a crash here PluginViewEfl.cpp:238.

[Chris Dumez]

(In reply to comment #14)
> LGTM but might introduce a crash here PluginViewEfl.cpp:238.

Would you care to elaborate? How could initializing ecore_x library cause a crash in a file I'm not touching?

PluginViewEfl.cpp:238 *would* likely crash *without* without my patch since the ecore_x library was not initialized before calling ecore_x_display_get().

Also note that in ecore_x_init(0), 0 mean default display according to the doc. In case this is related to your comment.

[Thiago Marcos P. Santos]

(In reply to comment #15)
> (In reply to comment #14)
> > LGTM but might introduce a crash here PluginViewEfl.cpp:238.
> 
> Would you care to elaborate? How could initializing ecore_x library cause a crash in a file I'm not touching?
> 
> PluginViewEfl.cpp:238 *would* likely crash *without* without my patch since the ecore_x library was not initialized before calling ecore_x_display_get().
> 
> Also note that in ecore_x_init(0), 0 mean default display according to the doc. In case this is related to your comment.

Sorry, I thought that you were conditioning the ecore_x initialization to HAVE_ECORE_X flag. But it is not being initialized at all.

PluginViewEfl.cpp:238 is a separated bug then.

LGTM with no remarks. :)

[Csaba Osztrogonác]

Comment on attachment 145046 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 145046 [details]
Patch

Clearing flags on attachment: 145046

Committed r119228: <http://trac.webkit.org/changeset/119228>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sudarsana Nagineni (babu)]

DRT crashes on the test startup with this patch. The EFL Buildbots are red exiting early after 20 crashes.

crash log for DumpRenderTree (pid 24858):
STDOUT: <empty>
STDERR: CRI<24918>:ewebkit /home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Source/WebKit/efl/ewk/ewk_main.cpp:96 ewk_init() could not init ecore_x

[WebKit Review Bot]

Re-opened since this is blocked by 88098

[Chris Dumez]

Dominik, gyuyoung: This patch seems valid but it causes crashes on the build bot. The reason for that is that calling "ecore_x_init(0)" on a machine which does not have X running causes a crash :(

I believe we have 2 possible solutions:
1. Make sure the ecore_x library is not build on the build bot. This way, the ECORE_X flag will not be set and ecore_x_init(0) will never be called.
2. Install a headless X on the build bots

I think 1. is probably easier however, 2. would allow to enable/run the same tests as on a regular desktop machine.

[Chris Dumez]

Created attachment 145987 [details]
Patch

Improved patch which runs each DRT in its own xfvb. This will avoid crashes on the bot due to X not running and the ecore_x library being used.

[Chris Dumez]

Comment on attachment 145987 [details]
Patch

Clearing flags for now as it seems to cause trouble on my machine.
The tests run fine for a while but then I get this message:

stopping ImageDiff timed out, killing it
killed
Can not get image diff. ImageDiff program might not work correctly.

After that, it keeps going but extremely slowly and apparently with a single Xvfb running. I'm not sure what's going on yet.

[Alexander Shalamov]

Created attachment 162007 [details]
Patch

[Dominik Röttsches (drott)]

Comment on attachment 162007 [details]
Patch

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

Did you test this with DISPLAY unset?

> Source/WebCore/platform/efl/SoundEfl.cpp:42
> +    if (ecore_x_init(0)) {

This potentially initializes ecoreX multiple times, right? How about a function level static variable here to remember whether initialization completed?

[Chris Dumez]

Comment on attachment 162007 [details]
Patch

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

LGTM. Thanks.

>> Source/WebCore/platform/efl/SoundEfl.cpp:42
>> +    if (ecore_x_init(0)) {
> 
> This potentially initializes ecoreX multiple times, right? How about a function level static variable here to remember whether initialization completed?

No it does not. It increments a ref count.

[Alexander Shalamov]

(In reply to comment #26)
> (From update of attachment 162007 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162007&action=review
> 
> Did you test this with DISPLAY unset?

Yes, I tested with DISPLAY unset.


> > Source/WebCore/platform/efl/SoundEfl.cpp:42
> > +    if (ecore_x_init(0)) {
> 
> This potentially initializes ecoreX multiple times, right? How about a function level static variable here to remember whether initialization completed?

ecore_x_* has static variable for ref counting, all init / shutdown calls would increment / decrement ref count value.

[Kenneth Rohde Christiansen]

Comment on attachment 162007 [details]
Patch

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

>>>> Source/WebCore/platform/efl/SoundEfl.cpp:42
>>>> +    if (ecore_x_init(0)) {
>>> 
>>> This potentially initializes ecoreX multiple times, right? How about a function level static variable here to remember whether initialization completed?
>> 
>> No it does not. It increments a ref count.
> 
> ecore_x_* has static variable for ref counting, all init / shutdown calls would increment / decrement ref count value.

why not use the new WITH_ECORE_X ?

[Alexander Shalamov]

(In reply to comment #29)
> (From update of attachment 162007 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162007&action=review
> 
> >>>> Source/WebCore/platform/efl/SoundEfl.cpp:42
> >>>> +    if (ecore_x_init(0)) {
> >>> 
> >>> This potentially initializes ecoreX multiple times, right? How about a function level static variable here to remember whether initialization completed?
> >> 
> >> No it does not. It increments a ref count.
> > 
> > ecore_x_* has static variable for ref counting, all init / shutdown calls would increment / decrement ref count value.
> 
> why not use the new WITH_ECORE_X ?

The macro was fetching screen to get UI related properties.
Screen or display is not needed to call a bell (ecore_x_bell).

[Kenneth Rohde Christiansen]

Comment on attachment 162007 [details]
Patch

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

>>>>>> Source/WebCore/platform/efl/SoundEfl.cpp:42
>>>>>> +    if (ecore_x_init(0)) {
>>>>> 
>>>>> This potentially initializes ecoreX multiple times, right? How about a function level static variable here to remember whether initialization completed?
>>>> 
>>>> No it does not. It increments a ref count.
>>> 
>>> ecore_x_* has static variable for ref counting, all init / shutdown calls would increment / decrement ref count value.
>> 
>> why not use the new WITH_ECORE_X ?
> 
> The macro was fetching screen to get UI related properties.
> Screen or display is not needed to call a bell (ecore_x_bell).

I guess it should be renamed then to ECORE_X_CALL_WITH_DISPLAY or so then to make that more clear

[WebKit Review Bot]

Comment on attachment 162007 [details]
Patch

Clearing flags on attachment: 162007

Committed r127464: <http://trac.webkit.org/changeset/127464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.