Bug 105021 – [EFL] Unit tests process hanging on WK2 Release bots

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Thiago Marcos P. Santos]

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.

[Thiago Marcos P. Santos]

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

[Thiago Marcos P. Santos]

Created attachment 179604 [details]
Patch

[Thiago Marcos P. Santos]

Created attachment 179615 [details]
Patch

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

CC'ing Byungwoo, could you take a look this patch ?

[Kenneth Rohde Christiansen]

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?

[Thiago Marcos P. Santos]

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.

[Byungwoo Lee]

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.

[Thiago Marcos P. Santos]

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.

[Thiago Marcos P. Santos]

Created attachment 179706 [details]
Patch

[Thiago Marcos P. Santos]

(In reply to comment #10)
> Created an attachment (id=179706) [details]
> Patch

Updated the patch. Thanks for reviewing.

[Thiago Marcos P. Santos]

Tracking the misuse of system() at bug 105156.

[WebKit Review Bot]

Comment on attachment 179706 [details]
Patch

Clearing flags on attachment: 179706

Committed r137890: <http://trac.webkit.org/changeset/137890>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

Argh. Not entirely fixed since ::utf8() and ::data() still does memory allocation. Duh.

[Thiago Marcos P. Santos]

Created attachment 179751 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Thiago Marcos P. Santos]

Created attachment 179754 [details]
Patch

Added comments. Thanks for reviewing.

[WebKit Review Bot]

Comment on attachment 179754 [details]
Patch

Clearing flags on attachment: 179754

Committed r137917: <http://trac.webkit.org/changeset/137917>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.