Bug 105021 - [EFL] Unit tests process hanging on WK2 Release bots
Summary: [EFL] Unit tests process hanging on WK2 Release bots
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: Thiago Marcos P. Santos
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-14 07:06 PST by Thiago Marcos P. Santos
Modified: 2012-12-17 09:52 PST (History)
7 users (show)

See Also:


Attachments
Patch (5.10 KB, patch)
2012-12-15 12:04 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (5.10 KB, patch)
2012-12-15 14:07 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (5.10 KB, patch)
2012-12-17 02:10 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (3.54 KB, patch)
2012-12-17 08:41 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (3.79 KB, patch)
2012-12-17 08:57 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Marcos P. Santos 2012-12-14 07:06:34 PST
Unit tests that are passing on Debug bots are randomly timeouting on the Release bot and not being killed by CMake, turning into dangling process in a busy loop eating all the CPU available.
Comment 1 Thiago Marcos P. Santos 2012-12-14 09:31:44 PST
Stack trace of the process in a busy wait:

[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/i386-linux-gnu/libthread_db.so.1".
0xb76e41b2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2

Thread 1 (Thread 0xafeddb40 (LWP 7783)):
#0  0xb76e41b2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#1  0xb69c5d1c in sched_yield () at ../sysdeps/unix/syscall-template.S:82
#2  0x080562d0 in WTF::TCMalloc_Central_FreeList::FetchFromSpansSafe() ()
#3  0x080567f5 in WTF::TCMalloc_Central_FreeList::RemoveRange(void**, void**, int*) ()
#4  0x08057828 in WTF::fastMalloc(unsigned int) ()
#5  0xb7608adf in WTF::CStringBuffer::createUninitialized(unsigned int) () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libjavascriptcore_efl.so.0
#6  0xb7608b1f in WTF::CString::init(char const*, unsigned int) () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libjavascriptcore_efl.so.0
#7  0xb7608bcb in WTF::CString::CString(char const*, unsigned int) () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libjavascriptcore_efl.so.0
#8  0xb761508c in WTF::String::utf8(WTF::String::ConversionMode) const () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libjavascriptcore_efl.so.0
#9  0xb70cf27a in WebKit::findProcessPath(char const*) () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libewebkit2.so.0
#10 0xb70cf498 in WebKit::executablePathOfWebProcess() () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libewebkit2.so.0
#11 0xb7106108 in WebKit::ProcessLauncher::launchProcess() () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libewebkit2.so.0
#12 0xb7020b20 in WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebKit::ProcessLauncher::*)()>, void (WebKit::ProcessLauncher*)>::operator()() () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libewebkit2.so.0
#13 0xb70cc0b8 in WorkQueue::performWork() () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libewebkit2.so.0
#14 0xb70cc873 in WorkQueue::workQueueThread(WorkQueue*) () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libewebkit2.so.0
#15 0xb75fdd25 in WTF::threadEntryPoint(void*) () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libjavascriptcore_efl.so.0
#16 0xb761b775 in WTF::wtfThreadEntryPoint(void*) () from /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Release/lib/libjavascriptcore_efl.so.0
#17 0xb6c17d4c in start_thread (arg=0xafeddb40) at pthread_create.c:308
#18 0xb69efd3e in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:130
Comment 2 Thiago Marcos P. Santos 2012-12-15 12:04:22 PST
Created attachment 179604 [details]
Patch
Comment 3 Thiago Marcos P. Santos 2012-12-15 14:07:19 PST
Created attachment 179615 [details]
Patch
Comment 4 Gyuyoung Kim 2012-12-17 00:48:54 PST
Comment on attachment 179615 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:61
> +    String prefixedExecutablePath;

Could you explain why you use "prefixedExecutablePath* instead of cmd though I prefer to use full name ?

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:63
> +        prefixedExecutablePath = makeString(m_launchOptions.processCmdPrefix, ' ', executablePath, ' ', socket);

makeString is deprecated. it would be good if you follow "Efficient String" guidance.
Comment 5 Gyuyoung Kim 2012-12-17 00:50:04 PST
CC'ing Byungwoo, could you take a look this patch ?
Comment 6 Kenneth Rohde Christiansen 2012-12-17 01:31:11 PST
Comment on attachment 179615 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:52
> +        break;
>  #if ENABLE(PLUGIN_PROCESS)
> -        case PluginProcess:
> -            executablePath = executablePathOfPluginProcess();
> -            break;
> +    case PluginProcess:
> +        executablePath = executablePathOfPluginProcess();
> +        break;
>  #endif

at some point we should handle the network process here

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:71
> +            // FIXME: This is not correct because invokes the shell

because *it*

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:75
> +            // something like execvp().
> +            if (system(prefixedExecutablePath.utf8().data()) == -1) {
>                  ASSERT_NOT_REACHED();

why not do it now?
Comment 7 Thiago Marcos P. Santos 2012-12-17 01:32:58 PST
Comment on attachment 179615 [details]
Patch

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

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:61
>> +    String prefixedExecutablePath;
> 
> Could you explain why you use "prefixedExecutablePath* instead of cmd though I prefer to use full name ?

We don't use abbreviations like this on WebKit. I made it more explicit.

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:63
>> +        prefixedExecutablePath = makeString(m_launchOptions.processCmdPrefix, ' ', executablePath, ' ', socket);
> 
> makeString is deprecated. it would be good if you follow "Efficient String" guidance.

Should have fixed this too. It is coming from the existing code I moved.
Comment 8 Byungwoo Lee 2012-12-17 01:41:14 PST
Comment on attachment 179615 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:70
> +        if (!prefixedExecutablePath.isEmpty()) {

Can I ask why the condition is changed?
This has no problem but checking processCmdPrefix looks clear to me.
Comment 9 Thiago Marcos P. Santos 2012-12-17 02:07:43 PST
Comment on attachment 179615 [details]
Patch

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

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:70
>> +        if (!prefixedExecutablePath.isEmpty()) {
> 
> Can I ask why the condition is changed?
> This has no problem but checking processCmdPrefix looks clear to me.

I'm using prefixedExecutablePath because we get a more logical pattern:

if (prefixedExecutablePath)
   do something with prefixedExecutablePath

instead of:

if (processCmdPrefix)
   do something with prefixedExecutablePath

>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:75
>>                  ASSERT_NOT_REACHED();
> 
> why not do it now?

It would make this patch way bigger. I think it is out of the scope of this fix.
Comment 10 Thiago Marcos P. Santos 2012-12-17 02:10:03 PST
Created attachment 179706 [details]
Patch
Comment 11 Thiago Marcos P. Santos 2012-12-17 02:11:49 PST
(In reply to comment #10)
> Created an attachment (id=179706) [details]
> Patch

Updated the patch. Thanks for reviewing.
Comment 12 Thiago Marcos P. Santos 2012-12-17 02:21:38 PST
Tracking the misuse of system() at bug 105156.
Comment 13 WebKit Review Bot 2012-12-17 02:50:24 PST
Comment on attachment 179706 [details]
Patch

Clearing flags on attachment: 179706

Committed r137890: <http://trac.webkit.org/changeset/137890>
Comment 14 WebKit Review Bot 2012-12-17 02:50:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Thiago Marcos P. Santos 2012-12-17 08:32:00 PST
Argh. Not entirely fixed since ::utf8() and ::data() still does memory allocation. Duh.
Comment 16 Thiago Marcos P. Santos 2012-12-17 08:41:41 PST
Created attachment 179751 [details]
Patch
Comment 17 Kenneth Rohde Christiansen 2012-12-17 08:45:33 PST
Comment on attachment 179751 [details]
Patch

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

maybe we should add some comments to the code?

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:43
> -    String executablePath;
> +    const char* executablePath = 0;

comment?
Comment 18 Thiago Marcos P. Santos 2012-12-17 08:57:59 PST
Created attachment 179754 [details]
Patch

Added comments. Thanks for reviewing.
Comment 19 WebKit Review Bot 2012-12-17 09:52:46 PST
Comment on attachment 179754 [details]
Patch

Clearing flags on attachment: 179754

Committed r137917: <http://trac.webkit.org/changeset/137917>
Comment 20 WebKit Review Bot 2012-12-17 09:52:52 PST
All reviewed patches have been landed.  Closing bug.