new-run-webkit-tests can deadlock with Chromium's TestShell

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

new-run-webkit-tests can deadlock with Chromium's TestShell

TestShell waits in fgets for a new URL from NRWT.

NRWT waits in .readline() for a line which looks like #EOF.

If TestShell never sends an #EOF, or hangs itself before sending the #EOF, then the two will deadlock.

See attached samples.

[Eric Seidel (no email)]

Created attachment 54645 [details]
Sample from new-run-webkit-tests when hung waiting on two TestShells

[Eric Seidel (no email)]

Created attachment 54646 [details]
sample from first TestShell

[Eric Seidel (no email)]

Created attachment 54647 [details]
sample from second TestShell

[Eric Seidel (no email)]

I'm not sure what exact conditions caused this deadlock, but it's clear from reading the code it's possible.  We may have had this for a long time, or it's possible (but unlikely) that this recently regressed.

[Eric Seidel (no email)]

The hang in TestShell happens at this fgets:
http://src.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=webkit/tools/test_shell/test_shell_main.cc;h=37c26bcff0bc9475f051cc0025b688109ed816ca;hb=HEAD#l316

If for any reason the first char of: filenameBuffer is 0 we can hang:

 324           if (!*filenameBuffer)
 325             continue;

This started happening more with my unicode changes, because we started sending unicode bytestreams to test_shell on some systems by mistake.  If the first byte left in the buffer is ever 00, then it hangs forever.  That could happen if IO got interupted due to process switching, etc.

[Eric Seidel (no email)]

I have a fix for the added frequency.

Note, this is STILL a bug in test_shell/NRWT that they can deadlock like this.  It's been this way forever.  But it should occur less often now.

[Eric Seidel (no email)]

Created attachment 54674 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:77:  expected 1 blank line, found 0  [pep8/E301] [5]
Total errors found: 1 in 3 files


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

[Eric Seidel (no email)]

pep8.py doesn't like my inline function definition, but I wasn't sure what it really wanted either.

[Adam Barth]

Comment on attachment 54674 [details]
Patch

Bow down before our stylebot overlords.

[Eric Seidel (no email)]

Stylebot is wrong.

PEP8 says nothing about inline methods:
http://www.python.org/dev/peps/pep-0008/

See the "blank lines" section.  pep8 is treating this like a "method in a class" which is wrong, this is a method in a method in a class, which is not covered in pep8.

[Adam Barth]

Comment on attachment 54674 [details]
Patch

I'm suspicious, but ok.

[Eric Seidel (no email)]

Comment on attachment 54674 [details]
Patch

Thank you for the review.  If you have a suggested style fix, I'm happy to make one.  But I think adding blank lines around the inline function is silly in this case, and certainly not covered by pep8.

[WebKit Commit Bot]

Comment on attachment 54674 [details]
Patch

Clearing flags on attachment: 54674

Committed r58503: <http://trac.webkit.org/changeset/58503>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58503 might have broken Qt Linux Release