68026 2011-09-13 14:21:50 -0700 chromium.py's KILL_TIMEOUT is too short for valgrind 2011-09-23 15:46:36 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 tony thestig abarth dpranke eric mrobinson ossy thestig timurrrr webkit.review.bot xan.lopez oldest_to_newest 466524 0 tony 2011-09-13 14:21:50 -0700 In WebKit/Tools/Scripts/webkitpy/layout_tests/port/chromium.py, there's a 3 second timeout when closing the DRT process pipe where we wait for the DRT process to shutdown. When running DRT in valgrind, this takes longer than 3 seconds and we kill the process before getting memory leak information. Maybe we can somehow share the --time-out-ms flag to increase this value? I'm not sure if there's some other way to detect that we're running under valgrind to increase this timeout. Maybe thestig knows. 466533 1 eric 2011-09-13 14:29:16 -0700 I mean, we could also add back the TOOL_WRAPPER environment if valgrind really needs it. :) But yes, it seems like a reasonable heuristic that we could just double that value when time_out_ms is larger than default. 466538 2 thestig 2011-09-13 14:33:33 -0700 I already fixed the script on the Chromium side to use --wrapper. Just need to fix this bit here. Can we just use --time-out-ms as the timer value here? The script on the Chromium side is already passing --time-out-ms=200000. Doubling the timeout value is not enough. I set to 180 seconds locally just to be on the safe side , but I can see what's a good, lower value if needed. 466550 3 eric 2011-09-13 14:40:02 -0700 Is this generally true when trying to use valgrind with run-webkit-tests on linux/mac? Will other ports want something like this? 466555 4 thestig 2011-09-13 14:42:22 -0700 I don't know if other ports run Valgrind with --time-out-ms=$large_number, but I imagine they would have to given Valgrind's slowness. 466909 5 timurrrr 2011-09-14 01:11:36 -0700 > I'm not sure if there's some other way to detect > that we're running under valgrind to increase this timeout. Use RUNNING_ON_VALGRIND ? http://src.chromium.org/viewvc/chrome/trunk/src/base/third_party/valgrind/valgrind.h?revision=72632&view=markup 466911 6 timurrrr 2011-09-14 01:13:27 -0700 Also, you can add RunningOnValgrind() from http://src.chromium.org/viewvc/chrome/trunk/src/base/third_party/dynamic_annotations/dynamic_annotations.c?revision=84975&view=markup to JavaScriptCore/wtf/DynamicAnnotations.{h,cpp} 467393 7 dpranke 2011-09-14 15:15:53 -0700 (In reply to comment #6) > Also, you can add RunningOnValgrind() from http://src.chromium.org/viewvc/chrome/trunk/src/base/third_party/dynamic_annotations/dynamic_annotations.c?revision=84975&view=markup > to JavaScriptCore/wtf/DynamicAnnotations.{h,cpp} Unfortunately I don't see how we could access that from the python code. Do we have any idea how long it does take for DRT to shut down, so we know how long we should be scaling up to? It seems reasonable to scale up the 3 second timeout based on how much --time-out-ms is scaled up over the default. 467433 8 thestig 2011-09-14 15:46:39 -0700 If the current 3 second timeout is meant for a machine like a Z600, then 15 seconds should be fine for Valgrind. (I ran a bunch of tests and see ~8 seconds nromally and doubled it to be safe.) 471831 9 108390 thestig 2011-09-22 13:39:08 -0700 Created attachment 108390 [chromium] Make the layout test script's kill timeout proportional to --time-out-ms Ping? Would something like this work? 471892 10 108390 dpranke 2011-09-22 15:15:08 -0700 Comment on attachment 108390 [chromium] Make the layout test script's kill timeout proportional to --time-out-ms Hi, sorry for the delay in getting back to you (I've been backlogged with other things). This looks fine; do you need me to CQ+ it or land it for you? 471896 11 108390 tony 2011-09-22 15:20:21 -0700 Comment on attachment 108390 [chromium] Make the layout test script's kill timeout proportional to --time-out-ms Needs a changelog :) 471897 12 dpranke 2011-09-22 15:21:15 -0700 (In reply to comment #11) > (From update of attachment 108390 [details]) > Needs a changelog :) Whoops :( 471901 13 108411 thestig 2011-09-22 15:25:03 -0700 Created attachment 108411 patch with changelog Ha, my Chromium checkout checked out Tools/Scripts, but not Tools, thus I couldn't find the ChangeLog and when *shrug*. 472448 14 108520 tony 2011-09-23 13:16:25 -0700 Created attachment 108520 Patch for landing 472451 15 tony 2011-09-23 13:17:26 -0700 (In reply to comment #14) > Created an attachment (id=108520) [details] > Patch for landing Fixed the svn root and added the bug link to the changelog. 472567 16 108520 webkit.review.bot 2011-09-23 15:46:30 -0700 Comment on attachment 108520 Patch for landing Clearing flags on attachment: 108520 Committed r95875: <http://trac.webkit.org/changeset/95875> 472568 17 webkit.review.bot 2011-09-23 15:46:36 -0700 All reviewed patches have been landed. Closing bug. 108390 2011-09-22 13:39:08 -0700 2011-09-22 15:25:03 -0700 [chromium] Make the layout test script's kill timeout proportional to --time-out-ms 68026.patch text/plain 1595 thestig SW5kZXg6IHdlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtLnB5Cj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIHdlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtLnB5CShyZXZpc2lvbiA5NTE4 NykKKysrIHdlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtLnB5CSh3b3JraW5nIGNv cHkpCkBAIC0zOTQsOCArMzk0LDYgQEAKIAogIyBGSVhNRTogVGhpcyBzaG91bGQgaW5oZXJpdCBm cm9tIFdlYktpdERyaXZlciBub3cgdGhhdCBDaHJvbWl1bSBoYXMgYSBEdW1wUmVuZGVyVHJlZSBw cm9jZXNzIGxpa2UgdGhlIHJlc3Qgb2YgV2ViS2l0LgogY2xhc3MgQ2hyb21pdW1Ecml2ZXIoRHJp dmVyKToKLSAgICBLSUxMX1RJTUVPVVQgPSAzLjAKLQogICAgIGRlZiBfX2luaXRfXyhzZWxmLCBw b3J0LCB3b3JrZXJfbnVtYmVyKToKICAgICAgICAgRHJpdmVyLl9faW5pdF9fKHNlbGYsIHBvcnQs IHdvcmtlcl9udW1iZXIpCiAgICAgICAgIHNlbGYuX3Byb2MgPSBOb25lCkBAIC01OTUsMTIgKzU5 MywyMSBAQAogICAgICAgICBzZWxmLl9wcm9jLnN0ZG91dC5jbG9zZSgpCiAgICAgICAgIGlmIHNl bGYuX3Byb2Muc3RkZXJyOgogICAgICAgICAgICAgc2VsZi5fcHJvYy5zdGRlcnIuY2xvc2UoKQor ICAgICAgICAjIFRoZSBraWxsIHRpbWVvdXQgd2FzIGhhcmQgY29kZWQgdG8gMyBzZWNvbmRzIGdp dmVuIGEgZGVmYXVsdAorICAgICAgICAjIC0tdGltZS1vdXQtbXMgb2YgMzUgc2Vjb25kcy4gV2Ug ZGl2aWRlIGJ5IDEyIHRvIGdldCBhIGNsb3NlCisgICAgICAgICMgYXBwcm94aW1hdGlvbi4KKyAg ICAgICAgdGltZV9vdXRfbXMgPSBzZWxmLl9wb3J0LmdldF9vcHRpb24oJ3RpbWVfb3V0X21zJykK KyAgICAgICAgaWYgdGltZV9vdXRfbXM6CisgICAgICAgICAga2lsbF90aW1lb3V0X3NlY29uZHMg PSBpbnQodGltZV9vdXRfbXMpIC8gMTIwMDAKKyAgICAgICAgZWxzZToKKyAgICAgICAgICBraWxs X3RpbWVvdXRfc2Vjb25kcyA9IDMuMAorCiAgICAgICAgICMgQ2xvc2luZyBzdGRpbi9zdGRvdXQv c3RkZXJyIGhhbmdzIHNvbWV0aW1lcyBvbiBPUyBYLAogICAgICAgICAjIChzZWUgX19pbml0X18o KSwgYWJvdmUpLCBhbmQgYW55d2F5IHdlIGRvbid0IHdhbnQgdG8gaGFuZwogICAgICAgICAjIHRo ZSBoYXJuZXNzIGlmIERSVCBpcyBidWdneSwgc28gd2Ugd2FpdCBhIGNvdXBsZQogICAgICAgICAj IHNlY29uZHMgdG8gZ2l2ZSBEUlQgYSBjaGFuY2UgdG8gY2xlYW4gdXAsIGJ1dCB0aGVuCiAgICAg ICAgICMgZm9yY2Uta2lsbCB0aGUgcHJvY2VzcyBpZiBuZWNlc3NhcnkuCi0gICAgICAgIHRpbWVv dXQgPSB0aW1lLnRpbWUoKSArIHNlbGYuS0lMTF9USU1FT1VUCisgICAgICAgIHRpbWVvdXQgPSB0 aW1lLnRpbWUoKSArIGtpbGxfdGltZW91dF9zZWNvbmRzCiAgICAgICAgIHdoaWxlIHNlbGYuX3By b2MucG9sbCgpIGlzIE5vbmUgYW5kIHRpbWUudGltZSgpIDwgdGltZW91dDoKICAgICAgICAgICAg IHRpbWUuc2xlZXAoMC4xKQogICAgICAgICBpZiBzZWxmLl9wcm9jLnBvbGwoKSBpcyBOb25lOgo= 108411 2011-09-22 15:25:03 -0700 2011-09-23 13:16:20 -0700 patch with changelog 68026_2.patch text/plain 2176 thestig SW5kZXg6IFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW0ucHkKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9jaHJvbWl1bS5w eQkocmV2aXNpb24gOTU3NTUpCisrKyBTY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0 L2Nocm9taXVtLnB5CSh3b3JraW5nIGNvcHkpCkBAIC0zOTQsOCArMzk0LDYgQEAKIAogIyBGSVhN RTogVGhpcyBzaG91bGQgaW5oZXJpdCBmcm9tIFdlYktpdERyaXZlciBub3cgdGhhdCBDaHJvbWl1 bSBoYXMgYSBEdW1wUmVuZGVyVHJlZSBwcm9jZXNzIGxpa2UgdGhlIHJlc3Qgb2YgV2ViS2l0Lgog Y2xhc3MgQ2hyb21pdW1Ecml2ZXIoRHJpdmVyKToKLSAgICBLSUxMX1RJTUVPVVQgPSAzLjAKLQog ICAgIGRlZiBfX2luaXRfXyhzZWxmLCBwb3J0LCB3b3JrZXJfbnVtYmVyKToKICAgICAgICAgRHJp dmVyLl9faW5pdF9fKHNlbGYsIHBvcnQsIHdvcmtlcl9udW1iZXIpCiAgICAgICAgIHNlbGYuX3By b2MgPSBOb25lCkBAIC01OTUsMTIgKzU5MywyMSBAQAogICAgICAgICBzZWxmLl9wcm9jLnN0ZG91 dC5jbG9zZSgpCiAgICAgICAgIGlmIHNlbGYuX3Byb2Muc3RkZXJyOgogICAgICAgICAgICAgc2Vs Zi5fcHJvYy5zdGRlcnIuY2xvc2UoKQorICAgICAgICAjIFRoZSBraWxsIHRpbWVvdXQgd2FzIGhh cmQgY29kZWQgdG8gMyBzZWNvbmRzIGdpdmVuIGEgZGVmYXVsdAorICAgICAgICAjIC0tdGltZS1v dXQtbXMgb2YgMzUgc2Vjb25kcy4gV2UgZGl2aWRlIGJ5IDEyIHRvIGdldCBhIGNsb3NlCisgICAg ICAgICMgYXBwcm94aW1hdGlvbi4KKyAgICAgICAgdGltZV9vdXRfbXMgPSBzZWxmLl9wb3J0Lmdl dF9vcHRpb24oJ3RpbWVfb3V0X21zJykKKyAgICAgICAgaWYgdGltZV9vdXRfbXM6CisgICAgICAg ICAga2lsbF90aW1lb3V0X3NlY29uZHMgPSBpbnQodGltZV9vdXRfbXMpIC8gMTIwMDAKKyAgICAg ICAgZWxzZToKKyAgICAgICAgICBraWxsX3RpbWVvdXRfc2Vjb25kcyA9IDMuMAorCiAgICAgICAg ICMgQ2xvc2luZyBzdGRpbi9zdGRvdXQvc3RkZXJyIGhhbmdzIHNvbWV0aW1lcyBvbiBPUyBYLAog ICAgICAgICAjIChzZWUgX19pbml0X18oKSwgYWJvdmUpLCBhbmQgYW55d2F5IHdlIGRvbid0IHdh bnQgdG8gaGFuZwogICAgICAgICAjIHRoZSBoYXJuZXNzIGlmIERSVCBpcyBidWdneSwgc28gd2Ug d2FpdCBhIGNvdXBsZQogICAgICAgICAjIHNlY29uZHMgdG8gZ2l2ZSBEUlQgYSBjaGFuY2UgdG8g Y2xlYW4gdXAsIGJ1dCB0aGVuCiAgICAgICAgICMgZm9yY2Uta2lsbCB0aGUgcHJvY2VzcyBpZiBu ZWNlc3NhcnkuCi0gICAgICAgIHRpbWVvdXQgPSB0aW1lLnRpbWUoKSArIHNlbGYuS0lMTF9USU1F T1VUCisgICAgICAgIHRpbWVvdXQgPSB0aW1lLnRpbWUoKSArIGtpbGxfdGltZW91dF9zZWNvbmRz CiAgICAgICAgIHdoaWxlIHNlbGYuX3Byb2MucG9sbCgpIGlzIE5vbmUgYW5kIHRpbWUudGltZSgp IDwgdGltZW91dDoKICAgICAgICAgICAgIHRpbWUuc2xlZXAoMC4xKQogICAgICAgICBpZiBzZWxm Ll9wcm9jLnBvbGwoKSBpcyBOb25lOgpJbmRleDogQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIENo YW5nZUxvZwkocmV2aXNpb24gOTU3NTUpCisrKyBDaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAg LTEsMyArMSwxMSBAQAorMjAxMS0wOS0yMiAgTGVpIFpoYW5nICA8dGhlc3RpZ0BjaHJvbWl1bS5v cmc+CisKKyAgICAgICAgW2Nocm9taXVtXSBNYWtlIHRoZSBsYXlvdXQgdGVzdCBzY3JpcHQncyBr aWxsIHRpbWVvdXQgcHJvcG9ydGlvbmFsIHRvIC0tdGltZS1vdXQtbXMKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5 b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW0ucHk6CisKIDIwMTEtMDktMjIgIEFuZGVycyBDYXJsc3Nv biAgPGFuZGVyc2NhQGFwcGxlLmNvbT4KIAogICAgICAgICBFdmVudFNlbmRlclByb3h5Ojptb3Vz ZVVwIHNob3VsZCBjYWxsIC1bV0tWaWV3IG1vdXNlVXA6XSBpbnN0ZWFkIG9mIC1bV0tWaWV3IG1v dXNlRG93bjpdCg== 108520 2011-09-23 13:16:25 -0700 2011-09-23 15:46:30 -0700 Patch for landing bug-68026-20110923131508.patch text/plain 2382 tony U3VidmVyc2lvbiBSZXZpc2lvbjogOTU4MjYKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBi L1Rvb2xzL0NoYW5nZUxvZwppbmRleCAxOWM2OGRiYTY3MmY1NzE4MjZlMjAzZjM5OTNkNDJmMDg3 NWUzNmI4Li5hZGU2MTVjNGEyZjZkNGVlYjIzYTM0MGI4NzAyMGRlOGRlM2M5MzU1IDEwMDY0NAot LS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTIg QEAKKzIwMTEtMDktMjMgIExlaSBaaGFuZyAgPHRoZXN0aWdAY2hyb21pdW0ub3JnPgorCisgICAg ICAgIFtjaHJvbWl1bV0gTWFrZSB0aGUgbGF5b3V0IHRlc3Qgc2NyaXB0J3Mga2lsbCB0aW1lb3V0 IHByb3BvcnRpb25hbCB0byAtLXRpbWUtb3V0LW1zCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD02ODAyNgorCisgICAgICAgIFJldmlld2VkIGJ5IERpcmsg UHJhbmtlLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9j aHJvbWl1bS5weToKKwogMjAxMS0wOS0yMiAgRGlyayBQcmFua2UgIDxkcHJhbmtlQGNocm9taXVt Lm9yZz4KIAogICAgICAgICBlbmFibGUgTlJXVCBmb3IgcnVuLXdlYmtpdC10ZXN0cyBvbiBMaW9u CmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2No cm9taXVtLnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9jaHJv bWl1bS5weQppbmRleCBkMmYwMmJlODA2YTU0NmY4YWFmZTg4MDBhYzlkOThkN2MzZGFkNDgwLi45 M2Y1NWJjYzY5YWJiYzhlZDM1MTc0ZDg3ZGU4YTRmY2QxZTM2NDZkIDEwMDc1NQotLS0gYS9Ub29s cy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtLnB5CisrKyBiL1Rv b2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW0ucHkKQEAgLTM5 NCw4ICszOTQsNiBAQCBjbGFzcyBDaHJvbWl1bVBvcnQoUG9ydCk6CiAKICMgRklYTUU6IFRoaXMg c2hvdWxkIGluaGVyaXQgZnJvbSBXZWJLaXREcml2ZXIgbm93IHRoYXQgQ2hyb21pdW0gaGFzIGEg RHVtcFJlbmRlclRyZWUgcHJvY2VzcyBsaWtlIHRoZSByZXN0IG9mIFdlYktpdC4KIGNsYXNzIENo cm9taXVtRHJpdmVyKERyaXZlcik6Ci0gICAgS0lMTF9USU1FT1VUID0gMy4wCi0KICAgICBkZWYg X19pbml0X18oc2VsZiwgcG9ydCwgd29ya2VyX251bWJlcik6CiAgICAgICAgIERyaXZlci5fX2lu aXRfXyhzZWxmLCBwb3J0LCB3b3JrZXJfbnVtYmVyKQogICAgICAgICBzZWxmLl9wcm9jID0gTm9u ZQpAQCAtNTk1LDEyICs1OTMsMjEgQEAgY2xhc3MgQ2hyb21pdW1Ecml2ZXIoRHJpdmVyKToKICAg ICAgICAgc2VsZi5fcHJvYy5zdGRvdXQuY2xvc2UoKQogICAgICAgICBpZiBzZWxmLl9wcm9jLnN0 ZGVycjoKICAgICAgICAgICAgIHNlbGYuX3Byb2Muc3RkZXJyLmNsb3NlKCkKKyAgICAgICAgIyBU aGUga2lsbCB0aW1lb3V0IHdhcyBoYXJkIGNvZGVkIHRvIDMgc2Vjb25kcyBnaXZlbiBhIGRlZmF1 bHQKKyAgICAgICAgIyAtLXRpbWUtb3V0LW1zIG9mIDM1IHNlY29uZHMuIFdlIGRpdmlkZSBieSAx MiB0byBnZXQgYSBjbG9zZQorICAgICAgICAjIGFwcHJveGltYXRpb24uCisgICAgICAgIHRpbWVf b3V0X21zID0gc2VsZi5fcG9ydC5nZXRfb3B0aW9uKCd0aW1lX291dF9tcycpCisgICAgICAgIGlm IHRpbWVfb3V0X21zOgorICAgICAgICAgIGtpbGxfdGltZW91dF9zZWNvbmRzID0gaW50KHRpbWVf b3V0X21zKSAvIDEyMDAwCisgICAgICAgIGVsc2U6CisgICAgICAgICAga2lsbF90aW1lb3V0X3Nl Y29uZHMgPSAzLjAKKwogICAgICAgICAjIENsb3Npbmcgc3RkaW4vc3Rkb3V0L3N0ZGVyciBoYW5n cyBzb21ldGltZXMgb24gT1MgWCwKICAgICAgICAgIyAoc2VlIF9faW5pdF9fKCksIGFib3ZlKSwg YW5kIGFueXdheSB3ZSBkb24ndCB3YW50IHRvIGhhbmcKICAgICAgICAgIyB0aGUgaGFybmVzcyBp ZiBEUlQgaXMgYnVnZ3ksIHNvIHdlIHdhaXQgYSBjb3VwbGUKICAgICAgICAgIyBzZWNvbmRzIHRv IGdpdmUgRFJUIGEgY2hhbmNlIHRvIGNsZWFuIHVwLCBidXQgdGhlbgogICAgICAgICAjIGZvcmNl LWtpbGwgdGhlIHByb2Nlc3MgaWYgbmVjZXNzYXJ5LgotICAgICAgICB0aW1lb3V0ID0gdGltZS50 aW1lKCkgKyBzZWxmLktJTExfVElNRU9VVAorICAgICAgICB0aW1lb3V0ID0gdGltZS50aW1lKCkg KyBraWxsX3RpbWVvdXRfc2Vjb25kcwogICAgICAgICB3aGlsZSBzZWxmLl9wcm9jLnBvbGwoKSBp cyBOb25lIGFuZCB0aW1lLnRpbWUoKSA8IHRpbWVvdXQ6CiAgICAgICAgICAgICB0aW1lLnNsZWVw KDAuMSkKICAgICAgICAgaWYgc2VsZi5fcHJvYy5wb2xsKCkgaXMgTm9uZToK