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)
*** Bug 87945 has been marked as a duplicate of this bug. ***
ecore_x_init() needs to be called in the ewk_main. Will upload a patch for this.
Created attachment 145041 [details] Patch
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
(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
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.
(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.
Created attachment 145046 [details] Patch Ok, I think I understood what you meant now. Hopefully, this new patch is what you wanted.
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?
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.
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 ?
(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.
(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.
LGTM but might introduce a crash here PluginViewEfl.cpp:238.
(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.
(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. :)
Comment on attachment 145046 [details] Patch rs=me
Comment on attachment 145046 [details] Patch Clearing flags on attachment: 145046 Committed r119228: <http://trac.webkit.org/changeset/119228>
All reviewed patches have been landed. Closing bug.
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
Re-opened since this is blocked by 88098
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.
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.
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.
Created attachment 162007 [details] Patch
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?
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.
(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.
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 ?
(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).
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
Comment on attachment 162007 [details] Patch Clearing flags on attachment: 162007 Committed r127464: <http://trac.webkit.org/changeset/127464>