Bug 64337 - [EFL] Refactor scheduleDispatchFunctionsOnMainThread to fix crash.
Summary: [EFL] Refactor scheduleDispatchFunctionsOnMainThread to fix crash.
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: Ryuan Choi
URL:
Keywords:
Depends on: 64515
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-11 18:57 PDT by Ryuan Choi
Modified: 2011-07-17 19:59 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.44 KB, patch)
2011-07-11 19:04 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
pipe_approach (2.21 KB, patch)
2011-07-12 17:32 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
pipe_approach2 (3.67 KB, patch)
2011-07-12 20:41 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (2.26 KB, patch)
2011-07-14 15:11 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (1.97 KB, patch)
2011-07-15 18:30 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (2.13 KB, patch)
2011-07-16 19:01 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2011-07-11 18:57:17 PDT
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)
Comment 1 Ryuan Choi 2011-07-11 19:04:44 PDT
Created attachment 100414 [details]
Patch
Comment 2 Gyuyoung Kim 2011-07-11 21:04:27 PDT
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.
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-07-12 07:08:52 PDT
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?
Comment 4 Ryuan Choi 2011-07-12 15:39:58 PDT
(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
Comment 5 Leandro Pereira 2011-07-12 16:19:41 PDT
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.
Comment 6 Ryuan Choi 2011-07-12 16:29:14 PDT
(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?
Comment 7 Ryuan Choi 2011-07-12 17:32:45 PDT
Created attachment 100596 [details]
pipe_approach
Comment 8 Leandro Pereira 2011-07-12 18:06:07 PDT
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".
Comment 9 Ryuan Choi 2011-07-12 20:41:53 PDT
Created attachment 100617 [details]
pipe_approach2
Comment 10 WebKit Review Bot 2011-07-12 20:44:35 PDT
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.
Comment 11 Ryuan Choi 2011-07-12 20:52:27 PDT
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"
Comment 12 Gyuyoung Kim 2011-07-12 20:55:43 PDT
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]
Comment 13 Raphael Kubo da Costa (:rakuco) 2011-07-13 07:06:45 PDT
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).
Comment 14 Ryuan Choi 2011-07-14 15:11:10 PDT
Created attachment 100871 [details]
Patch
Comment 15 Raphael Kubo da Costa (:rakuco) 2011-07-15 09:35:49 PDT
> 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?
Comment 16 Ryuan Choi 2011-07-15 18:30:55 PDT
Created attachment 101082 [details]
Patch
Comment 17 Ryuan Choi 2011-07-15 18:35:28 PDT
(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.
Comment 18 Raphael Kubo da Costa (:rakuco) 2011-07-16 18:27:25 PDT
> 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).
Comment 19 Ryuan Choi 2011-07-16 19:01:18 PDT
Created attachment 101110 [details]
Patch
Comment 20 Ryuan Choi 2011-07-16 19:05:24 PDT
(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.
Comment 21 Raphael Kubo da Costa (:rakuco) 2011-07-16 21:54:53 PDT
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).
Comment 22 Ryuan Choi 2011-07-16 22:00:05 PDT
(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.
Comment 23 WebKit Review Bot 2011-07-17 19:59:01 PDT
Comment on attachment 101110 [details]
Patch

Clearing flags on attachment: 101110

Committed r91171: <http://trac.webkit.org/changeset/91171>
Comment 24 WebKit Review Bot 2011-07-17 19:59:07 PDT
All reviewed patches have been landed.  Closing bug.