Bug 64337 – [EFL] Refactor scheduleDispatchFunctionsOnMainThread to fix crash.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

When ecore_timer_add is called in child thread, it worked well.
It's because ecore doesn't check where ecore_timer_add is called, although ecore_timer_add should be called in main thread.

However, some checks were introduced at (E svn r61041)
and I got below error messages.

CRI<27517>: ecore_thread.c:859 _ecore_thread_assert_main_loop_thread() Call to ecore_timer_add from wrong thread!

Program received signal SIGABRT, Aborted.
[Switching to Thread 0xb6765b70 (LWP 27521)]
0x00110832 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
(gdb)

[Ryuan Choi]

Created attachment 100414 [details]
Patch

[Gyuyoung Kim]

In latest ecore library,  there is same crash on EWebLauncher.

CRI<1406>: ecore_thread.c:869 _ecore_thread_assert_main_loop_thread() Call to ecore_timer_add from wrong thread!
Aborted

I look over this patch. It looks good to me.

[Raphael Kubo da Costa (:rakuco)]

The weird part to me is that ecore_time_add is currently called from a method named scheduleDispatchFunctionsOnMainThread, so it should be called from the main thread.

If you get a backtrace when the assert in ecore is hit, where is scheduleDispatchFunctionsOnMainThread currently being called?

[Ryuan Choi]

(In reply to comment #3)
> The weird part to me is that ecore_time_add is currently called from a method named scheduleDispatchFunctionsOnMainThread, so it should be called from the main thread.
> 
> If you get a backtrace when the assert in ecore is hit, where is scheduleDispatchFunctionsOnMainThread currently being called?

scheduleDispatchFunctionsOnMainThread is called in child thread.
So, we should change it to prevent assert.

(gdb) bt
#0  0x00110832 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#1  0x021ca651 in *__GI_raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0x021cda82 in *__GI_abort () at abort.c:92
#3  0x002f61cf in _ecore_thread_assert_main_loop_thread (function=0x2fd0b8 "ecore_timer_add") at ecore_thread.c:860
#4  0x002f46d3 in ecore_timer_add (in=0, func=0x1886240 <WTF::timeoutFired(void*)>, data=0x0) at ecore_timer.c:144
#5  0x0188629e in WTF::scheduleDispatchFunctionsOnMainThread() ()
   from /workspace/webkits/efl-webkit/WebKitBuild/Release/WebKit/libewebkit.so.0
#6  0x01866e62 in WTF::callOnMainThread(void (*)(void*), void*) ()
   from /workspace/webkits/efl-webkit/WebKitBuild/Release/WebKit/libewebkit.so.0
#7  0x009c6a74 in WebCore::IconDatabase::performURLImport() ()
   from /workspace/webkits/efl-webkit/WebKitBuild/Release/WebKit/libewebkit.so.0
#8  0x009c86a8 in WebCore::IconDatabase::iconDatabaseSyncThread() ()
   from /workspace/webkits/efl-webkit/WebKitBuild/Release/WebKit/libewebkit.so.0
#9  0x01868c0f in WTF::threadEntryPoint(void*) ()
   from /workspace/webkits/efl-webkit/WebKitBuild/Release/WebKit/libewebkit.so.0
#10 0x01ceb96e in start_thread (arg=0xb6765b70) at pthread_create.c:300
#11 0x0226da4e in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:130

[Leandro Pereira]

Comment on attachment 100414 [details]
Patch

I don't like this approach. This will constantly wake the process up (every time the main loop is idle) to check a flag and sleep again.

[Ryuan Choi]

(In reply to comment #5)
> (From update of attachment 100414 [details])
> I don't like this approach. This will constantly wake the process up (every time the main loop is idle) to check a flag and sleep again.

I agree. we need better idea if possible.
Does we have any good alternative to wake main_thread in child thread?

[Ryuan Choi]

Created attachment 100596 [details]
pipe_approach

[Leandro Pereira]

Comment on attachment 100596 [details]
pipe_approach

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

This looks better, but I'm not familiar with Ecore_Pipe; I'll check the documentation (and code) tomorrow and do a proper informal review. In the meantime, informal r- for the reasons below.

> Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:43
> +static void deletePipe();
> +static Ecore_Pipe* pipe;

You could implement the needed OwnPtrEfl function for Ecore_Pipe, and get rid of deletePipe() (and the associated atexit() call below). Also, you should use a better name than simply "pipe".

[Ryuan Choi]

Created attachment 100617 [details]
pipe_approach2

[WebKit Review Bot]

Attachment 100617 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/OwnPtrCommon.h:56:  Ecore_Pipe is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryuan Choi]

The style bot complained about some struct names, but they are false positives.

(In reply to comment #8)
> (From update of attachment 100596 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100596&action=review
> 
> This looks better, but I'm not familiar with Ecore_Pipe; I'll check the documentation (and code) tomorrow and do a proper informal review. In the meantime, informal r- for the reasons below.
> 
> > Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:43
> > +static void deletePipe();
> > +static Ecore_Pipe* pipe;
> 
> You could implement the needed OwnPtrEfl function for Ecore_Pipe, and get rid of deletePipe() (and the associated atexit() call below). Also, you should use a better name than simply "pipe".

I fixed almost what you mentioned. but I can't give good name for "pipe"
Now I just changed it to "pipeObject"

[Gyuyoung Kim]

Comment on attachment 100617 [details]
pipe_approach2

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

>> Source/JavaScriptCore/wtf/OwnPtrCommon.h:56
>> +typedef struct _Ecore_Pipe Ecore_Pipe;
> 
> Ecore_Pipe is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

I think we need to let style checker not check "readability/naming rule". However, we are obliged to ignore this style error because this file is common file. Existed codes already have same style error.

~/webkit/WebKit$ Tools/Scripts/check-webkit-style Source/JavaScriptCore/wtf/OwnPtrCommon.h 
Source/JavaScriptCore/wtf/OwnPtrCommon.h:54:  Ecore_Evas is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/OwnPtrCommon.h:55:  Evas_Object is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

[Raphael Kubo da Costa (:rakuco)]

Doesn't it make more sense to put the OwnPtr stuff in a separate patch? I think it's easier for it to get reviewed and integrated.

>>> Source/JavaScriptCore/wtf/OwnPtrCommon.h:56
>>> +typedef struct _Ecore_Pipe Ecore_Pipe;

BTW, I'd rather keep these typedefs sorted alphabetically (here, below and in the implementation).

[Ryuan Choi]

Created attachment 100871 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

> Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:43
> +static Eina_Bool timeoutFired(void*);

Why not implement it here and save one declaration?

> Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:44
> +static OwnPtr<Ecore_Pipe> pipeObject;

Hmm, I've got some reservations with this idiom.

It is creating a global static non-POD object, so its destructor will be called after the program finishes, and in an undefined order. If your main() calls ecore_shutdown(), pipeObject's destructor will call ecore_pipe_del() after ecore_shutdown() has been called.

In this case, what I can see that people sometimes do is use the DEFINE_STATIC_LOCAL macro: the pointer will end up being leaked, but as it is only one object that will stay alive during the whole lifetime of the program people don't seem to care much.

> Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:48
> +    ecore_timer_add(0, timeoutFired, 0);

Is it necessary to have both a timer and a pipe or could monitorDispatchFunctions() call dispatchFunctionsFromMainThread() right away?

> Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:-45
> -static Eina_Bool timeoutFired(void*)

Why?

[Ryuan Choi]

Created attachment 101082 [details]
Patch

[Ryuan Choi]

(In reply to comment #15)
> > Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:43
> > +static Eina_Bool timeoutFired(void*);
> 
> Why not implement it here and save one declaration?
> 
> > Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:44
> > +static OwnPtr<Ecore_Pipe> pipeObject;
> 
> Hmm, I've got some reservations with this idiom.
> 
> It is creating a global static non-POD object, so its destructor will be called after the program finishes, and in an undefined order. If your main() calls ecore_shutdown(), pipeObject's destructor will call ecore_pipe_del() after ecore_shutdown() has been called.
> 
> In this case, what I can see that people sometimes do is use the DEFINE_STATIC_LOCAL macro: the pointer will end up being leaked, but as it is only one object that will stay alive during the whole lifetime of the program people don't seem to care much.
> 

Thank you for your comment.
You make me and this patch smarter.

I tried like you mentioned. 

> > Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:48
> > +    ecore_timer_add(0, timeoutFired, 0);
> 
> Is it necessary to have both a timer and a pipe or could monitorDispatchFunctions() call dispatchFunctionsFromMainThread() right away?
> 
> > Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:-45
> > -static Eina_Bool timeoutFired(void*)
> 
> Why?

I don't know why I can't catch it.
You're right. It's stupid.

I fixed.

[Raphael Kubo da Costa (:rakuco)]

> Source/JavaScriptCore/ChangeLog:8
> +        but in a main thread.

"the" main thread, as there is only one.

> Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:44
> +DEFINE_STATIC_LOCAL(OwnPtr<Ecore_Pipe>, pipeObject, ());

The usual idiom is to use this inside a function (see, for example, wtf/gtk/ThreadingGtk.cpp or, for something which looks like what you will have to do here, WebCore/dom/UserTypingGestureIndicator.cpp).

[Ryuan Choi]

Created attachment 101110 [details]
Patch

[Ryuan Choi]

(In reply to comment #18)
> > Source/JavaScriptCore/ChangeLog:8
> > +        but in a main thread.
> 
> "the" main thread, as there is only one.

fixed.

> 
> > Source/JavaScriptCore/wtf/efl/MainThreadEfl.cpp:44
> > +DEFINE_STATIC_LOCAL(OwnPtr<Ecore_Pipe>, pipeObject, ());
> 
> The usual idiom is to use this inside a function (see, for example, wtf/gtk/ThreadingGtk.cpp or, for something which looks like what you will have to do here, WebCore/dom/UserTypingGestureIndicator.cpp).

Although I didn't understand fully,
I prepared new patch like you mentioned.
Thanks.

[Raphael Kubo da Costa (:rakuco)]

LGTM, informal r+.

(In reply to comment #20)
> Although I didn't understand fully,
> I prepared new patch like you mentioned.
> Thanks.

What part was not clear? Technically speaking, the only difference between the last two patches is that the object only gets created when it is first invoked; the moving of the macro to a function was mostly to keep the code consistent with the other uses of DEFINE_STATIC_LOCAL (if you grep the source tree, you can see that there are no uses of it in the global scope).

[Ryuan Choi]

(In reply to comment #21)
> LGTM, informal r+.
> 
> (In reply to comment #20)
> > Although I didn't understand fully,
> > I prepared new patch like you mentioned.
> > Thanks.
> 
> What part was not clear? Technically speaking, the only difference between the last two patches is that the object only gets created when it is first invoked; the moving of the macro to a function was mostly to keep the code consistent with the other uses of DEFINE_STATIC_LOCAL (if you grep the source tree, you can see that there are no uses of it in the global scope).

Ah, I didn't consider when it is created.
Thanks for kind explanation.

[WebKit Review Bot]

Comment on attachment 101110 [details]
Patch

Clearing flags on attachment: 101110

Committed r91171: <http://trac.webkit.org/changeset/91171>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.