39203 2010-05-17 00:48:44 -0700 [DRT/Chromium] Increase the time out value 2010-05-21 11:58:38 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Other All RESOLVED FIXED P2 Normal --- 38023 1 tkent webkit-unassigned abarth dglazkov dpranke eric levin ojan tony webkit.review.bot oldest_to_newest 226658 0 tkent 2010-05-17 00:48:44 -0700 [DRT/Chromium] Increase the time out value 226665 1 56218 tkent 2010-05-17 00:53:08 -0700 Created attachment 56218 Proposed patch 226669 2 tony 2010-05-17 01:01:50 -0700 Ojan would be a good reviewer since he did a lot of work on getting the timeout as low as possible in TestShell. I'm a little confused. I thought when we hit the timeout, we first call notifyDone, and if that fails, we exit(0) the process (which looks like a crash). Is the call to notifyDone failing? 226703 3 tkent 2010-05-17 02:16:52 -0700 (In reply to comment #2) > Ojan would be a good reviewer since he did a lot of work on getting the timeout as low as possible in TestShell. > > I'm a little confused. I thought when we hit the timeout, we first call notifyDone, and if that fails, we exit(0) the process (which looks like a crash). Is the call to notifyDone failing? I think DRT/Chromium works as the same as other platforms DRTs. The problem is that, if a DRT process exits before new-run-webkit-tests detects timeout, new-run-webkit-tests assumes the DRT process crashed. (webkitpy/layout_tests/port/server_process.py _read()) The current DRT/Chromium timeout value is 10 sec and new-run-webkit-tests timeout value is 6 sec. I'm not sure why new-run-webkit-tests can't detect timeout correctly. Anyway, making DRT timeout value longer solves the problem. 226790 4 levin 2010-05-17 09:46:38 -0700 It seems like the DRT timeout should be longer than the script timeout (just like how you mentioned it was 10 seconds vs 6 seconds for test_shell and the script). 228247 5 56218 eric 2010-05-20 01:03:26 -0700 Comment on attachment 56218 Proposed patch OK. 228394 6 56218 tkent 2010-05-20 08:46:17 -0700 Comment on attachment 56218 Proposed patch Clearing flags on attachment: 56218 Committed r59839: <http://trac.webkit.org/changeset/59839> 228395 7 tkent 2010-05-20 08:46:27 -0700 All reviewed patches have been landed. Closing bug. 228427 8 webkit.review.bot 2010-05-20 09:48:08 -0700 http://trac.webkit.org/changeset/59839 might have broken Tiger Intel Release The following changes are on the blame list: http://trac.webkit.org/changeset/59840 http://trac.webkit.org/changeset/59839 228569 9 ojan 2010-05-20 14:25:18 -0700 Dirk is way more familiar with the current state of this code than I am. (In reply to comment #4) > It seems like the DRT timeout should be longer than the script timeout (just like how you mentioned it was 10 seconds vs 6 seconds for test_shell and the script). I actually think it's the other way around. If DRT times out, then it can dump whatever results it has so far and then load the next test. If the script times out, all it can do is kill DRT. new-run-webkit-tests passes the per-test timeout to test_shell when it's running the test, so that value isn't even used for new-run-webkit-tests. (In reply to comment #3) > (In reply to comment #2) > > I'm a little confused. I thought when we hit the timeout, we first call notifyDone, and if that fails, we exit(0) the process (which looks like a crash). Is the call to notifyDone failing? > > The problem is that, if a DRT process exits before new-run-webkit-tests detects timeout, new-run-webkit-tests assumes the DRT process crashed. If this is true, it's a regression or it's a difference between DRT and test_shell. There are two timeout cases: 1. The test times out due to logic error (e.g., forgot to call notifyDone). In this case, DRT can just dump #TIMEOUT and load the next test. 2. The test times out because DRT is hung. In this case, DRT needs to be killed. test_shell has code to also dump #TIMEOUT here in the relevant signal handlers. Does DRT not? #TIMEOUT lets new-run-webkit-tests know that the test timed out instead of crashing, even if DRT/test_shell disapears. Anyways, this patch is fine. We should match Mac DRT. But, I think the underlying bug is still there. Also, FWIW, the conclusion last time that we talked about timeouts on webkit-dev was that all tests would get a long (e.g. 30 seconds) timeout except for tests that are expected to timeout, which will get a much shorter timeout (e.g. a couple seconds). That way, we don't need to manually mark tests as SLOW and timeout tests don't make the test cycle time insanely large. 228621 10 dpranke 2010-05-20 16:16:32 -0700 (In reply to comment #9) > Dirk is way more familiar with the current state of this code than I am. > > (In reply to comment #4) > > It seems like the DRT timeout should be longer than the script timeout (just like how you mentioned it was 10 seconds vs 6 seconds for test_shell and the script). > > I actually think it's the other way around. If DRT times out, then it can dump whatever results it has so far and then load the next test. If the script times out, all it can do is kill DRT. > > new-run-webkit-tests passes the per-test timeout to test_shell when it's running the test, so that value isn't even used for new-run-webkit-tests. > The script logic actually varies by port. The WebKit Mac port of new-run-webkit-tests (and I think the Chromium DRT port) use non-blocking I/O and a timeout to enforce timeouts in the script. This way the script can abort before DRT does, and DRT doesn't have to have a concept of a timeout. The Chromium ports (all platforms) pass the timeout to TestShell, and rely on Test Shell to enforce the timeout. At one point there was code in some configurations of new-run-webkit-tests to try and catch timeouts, but not in the regular code path. We do things this way because there is no way to do a non-blocking read using select on Windows from Python. I think (but am not positive) that you can do this in (cygwin?) Perl on Windows, and so WebKit Win doesn't have this problem. So, DRT and test_shell are definitely different. Separately, I am in the middle of trying to change new-run-webkit-tests to detect wedged threads and recover, in order to work around the Python multi-threading issues we're seeing. So it *may* be feasible to change test_shell to match DRT here, but I can't say so conclusively yet. 228760 11 tkent 2010-05-20 23:08:30 -0700 (In reply to comment #10) > We do things this way because there is no way to do a non-blocking read using select on Windows from Python. I think (but am not positive) that you can do this in (cygwin?) Perl on Windows, and so WebKit Win doesn't have this problem. Do you mean we have no ways to make new-run-webkit-tests work with Chromium DRT for Windows? 228962 12 dpranke 2010-05-21 09:55:14 -0700 (In reply to comment #11) > (In reply to comment #10) > > We do things this way because there is no way to do a non-blocking read using select on Windows from Python. I think (but am not positive) that you can do this in (cygwin?) Perl on Windows, and so WebKit Win doesn't have this problem. > > Do you mean we have no ways to make new-run-webkit-tests work with Chromium DRT for Windows? Well, that's the question. We may have to run it under cygwin. I am looking into other options. 229026 13 ojan 2010-05-21 11:58:38 -0700 (In reply to comment #10) > The WebKit Mac port of new-run-webkit-tests (and I think the Chromium DRT port) use non-blocking I/O and a timeout to enforce timeouts in the script. This way the script can abort before DRT does, and DRT doesn't have to have a concept of a timeout. > > The Chromium ports (all platforms) pass the timeout to TestShell, and rely on Test Shell to enforce the timeout. At one point there was code in some configurations of new-run-webkit-tests to try and catch timeouts, but not in the regular code path. > > We do things this way because there is no way to do a non-blocking read using select on Windows from Python. I think (but am not positive) that you can do this in (cygwin?) Perl on Windows, and so WebKit Win doesn't have this problem. > > So, DRT and test_shell are definitely different. > > Separately, I am in the middle of trying to change new-run-webkit-tests to detect wedged threads and recover, in order to work around the Python multi-threading issues we're seeing. So it *may* be feasible to change test_shell to match DRT here, but I can't say so conclusively yet. I don't quite get all this, but, we should do our best to to maintain the following: For tests that timeout, but don't hang DRT, we first dump the render dump. That way, for timing out tests, we still spit put a -actual.txt file where we can see what the state of the render tree was when it timed out. This is very helpful in debugging timeouts, especially ones that are hard to reproduce. If it's run-webkit-tests that handles the timeout, there's no way for DRT to dump the timeout information. All I'm suggesting really is that both DRT and run-webkit-tests should enforce timeouts. run-webkit-tests's timeout should just be a couple seconds longer than DRTs. 56218 2010-05-17 00:53:08 -0700 2010-05-20 08:46:16 -0700 Proposed patch bug-39203-20100517165307.patch text/plain 1690 tkent ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCBiMjE4YjBjMWE5OTI3MTNmNTJkYWFmZjk0ZGJhMjA3OTg5ZWI0YjljLi4zNmJiMmFl YjQyOGU0ODVkNDhlMmFmMTE4YjNlZGI0MDk4ZDA2ZDlmIDEwMDY0NAotLS0gYS9XZWJLaXRUb29s cy9DaGFuZ2VMb2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkgQEAK KzIwMTAtMDUtMTcgIEtlbnQgVGFtdXJhICA8dGtlbnRAY2hyb21pdW0ub3JnPgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtEUlQvQ2hyb21pdW1dIElu Y3JlYXNlIHRoZSB0aW1lIG91dCB2YWx1ZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MzkyMDMKKworICAgICAgICBDaGFuZ2UgdGhlIHRpbWUgb3V0IHZh bHVlIG9mIENocm9taXVtIERSVCB0byAzMCBzZWNvbmRzLCB3aGljaCBpcworICAgICAgICB0aGUg c2FtZSBhcyBvdGhlciBwb3J0cy4KKyAgICAgICAgSWYgYSBEUlQgcHJvY2VzcyBleGl0cyBiZWZv cmUgbmV3LXJ1bi13ZWJraXQtdGVzdHMgZGV0ZWN0cyB0aW1lCisgICAgICAgIG91dCwgbmV3LXJ1 bi13ZWJraXQtdGVzdHMgYXNzdW1lcyB0aGUgRFJUIHByb2Nlc3MgY3Jhc2hlZC4KKworICAgICAg ICAqIER1bXBSZW5kZXJUcmVlL2Nocm9taXVtL1Rlc3RTaGVsbC5jcHA6CisgICAgICAgIChUZXN0 U2hlbGw6OmxheW91dFRlc3RUaW1lb3V0KToKKyAgICAgICAgICBDaGFuZ2UgdGhlIHRpbWUgb3V0 IHZhbHVlIGZyb20gMTAgc2Vjb25kcyB0byAzMCBzZWNvbmRzLgorCiAyMDEwLTA1LTE3ICBGdW1p dG9zaGkgVWthaSAgPHVrYWlAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEVy aWMgU2VpZGVsLgpkaWZmIC0tZ2l0IGEvV2ViS2l0VG9vbHMvRHVtcFJlbmRlclRyZWUvY2hyb21p dW0vVGVzdFNoZWxsLmNwcCBiL1dlYktpdFRvb2xzL0R1bXBSZW5kZXJUcmVlL2Nocm9taXVtL1Rl c3RTaGVsbC5jcHAKaW5kZXggMjRhODk1YWIxOGJhY2UwMWU1NWY1NTBiZTgzM2MzZGNmOTI3YjYw Zi4uNzg4YzBlZDBmZTRkZGYxNmY5ODRiZTU1ZTQyZWRmNGY2MDJkMWJjMiAxMDA2NDQKLS0tIGEv V2ViS2l0VG9vbHMvRHVtcFJlbmRlclRyZWUvY2hyb21pdW0vVGVzdFNoZWxsLmNwcAorKysgYi9X ZWJLaXRUb29scy9EdW1wUmVuZGVyVHJlZS9jaHJvbWl1bS9UZXN0U2hlbGwuY3BwCkBAIC01NjUs NyArNTY1LDExIEBAIHZvaWQgVGVzdFNoZWxsOjpiaW5kSlNPYmplY3RzVG9XaW5kb3coV2ViRnJh bWUqIGZyYW1lKQogCiBpbnQgVGVzdFNoZWxsOjpsYXlvdXRUZXN0VGltZW91dCgpCiB7Ci0gICAg cmV0dXJuIDEwICogMTAwMDsKKyAgICAvLyAzMCBzZWNvbmQgaXMgdGhlIHNhbWUgYXMgdGhlIHZh bHVlIGluIE1hYyBEUlQuCisgICAgLy8gSWYgd2UgdXNlIGEgdmFsdWUgc21hbGxlciB0aGFuIHRo ZSB0aW1lb3V0IHZhbHVlIG9mCisgICAgLy8gKG5ldy0pcnVuLXdlYmtpdC10ZXN0cywgKG5ldy0p cnVuLXdlYmtpdC10ZXN0cyBtaXN1bmRlcnN0YW5kcyB0aGF0IGEKKyAgICAvLyB0aW1lZC1vdXQg RFJUIHByb2Nlc3Mgd2FzIGNyYXNoZWQuCisgICAgcmV0dXJuIDMwICogMTAwMDsKIH0KIAogV2Vi Vmlld0hvc3QqIFRlc3RTaGVsbDo6Y3JlYXRlV2ViVmlldygpCg==