163046 2016-10-06 11:28:58 -0700 REGRESSION (r206853?): LayoutTest js/regress-141098.html failing 2016-11-15 11:49:17 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified REOPENED P2 Normal --- 1 ryanhaddad ysuzuki ap commit-queue fpizlo ggaren mark.lam msaboff saam ysuzuki oldest_to_newest 1237195 0 ryanhaddad 2016-10-06 11:28:58 -0700 LayoutTest js/regress-141098.html failing https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r206869%20(10010)/results.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=js%2Fregress-141098.html --- /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/js/regress-141098-expected.txt +++ /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/js/regress-141098-actual.txt @@ -7,7 +7,6 @@ Starting 1st probeAndRecurse PASS Exception: RangeError: Maximum call stack size exceeded. Starting 2nd probeAndRecurse -PASS Exception: RangeError: Maximum call stack size exceeded. PASS successfullyParsed is true TEST COMPLETE 1237196 1 ryanhaddad 2016-10-06 11:31:15 -0700 This test is flaky, but frequently failing. Earliest failure on the flakiness dashboard seems to be https://trac.webkit.org/changeset/206853. 1237232 2 ysuzuki 2016-10-06 12:04:57 -0700 We should adjust the parameter written in that test. 1237757 3 ryanhaddad 2016-10-07 12:54:22 -0700 Any chance this will be fixed soon? 1237801 4 290970 ysuzuki 2016-10-07 14:42:23 -0700 Created attachment 290970 Patch 1237900 5 290970 ysuzuki 2016-10-07 18:46:27 -0700 Comment on attachment 290970 Patch Thanks! 1237901 6 ysuzuki 2016-10-07 18:46:54 -0700 Thanks! 1237906 7 290970 commit-queue 2016-10-07 19:09:41 -0700 Comment on attachment 290970 Patch Clearing flags on attachment: 290970 Committed r206947: <http://trac.webkit.org/changeset/206947> 1237907 8 commit-queue 2016-10-07 19:09:44 -0700 All reviewed patches have been landed. Closing bug. 1237936 9 290970 mark.lam 2016-10-07 22:26:14 -0700 Comment on attachment 290970 Patch I think this is the wrong approach to fix this issue. If I remember correctly, the 50 number was carefully chosen to trigger the condition that previously resulted in a crash. Reducing it to 10 defeats the purpose of the test. 1237938 10 saam 2016-10-07 23:20:05 -0700 (In reply to comment #9) > Comment on attachment 290970 [details] > Patch > > I think this is the wrong approach to fix this issue. If I remember > correctly, the 50 number was carefully chosen to trigger the condition that > previously resulted in a crash. Reducing it to 10 defeats the purpose of > the test. Interesting. We should probably revert back then. Yusuke, do you know why your patch caused this test to become flaky? 1237981 11 ysuzuki 2016-10-08 10:29:28 -0700 (In reply to comment #10) > (In reply to comment #9) > > Comment on attachment 290970 [details] > > Patch > > > > I think this is the wrong approach to fix this issue. If I remember > > correctly, the 50 number was carefully chosen to trigger the condition that > > previously resulted in a crash. Reducing it to 10 defeats the purpose of > > the test. > > Interesting. We should probably revert back then. Yusuke, do you know why > your patch caused this test to become flaky? Yeah, I think this is due to "testPassed" function. It calls escapeHTML, and escapeHTML calls String.prototype.replace. And String.prototype.replace uses @throwTypeError intrinsic. I guess that by introducing @throwTypeError, the one (or combination) of the followings occur. 1. The stack size used for String.prototype.replace is reduced. And when inlining it in the caller, the stack size of the whole function could be reduced. 2. By using @throwTypeError, String.prototype.replace becomes inlined in the test. And it changes the stack size of the function. I have one question: Is this test executed correctly under the specific situation before our patch? It was already marked as skip. 1237998 12 ryanhaddad 2016-10-08 13:04:00 -0700 Reopening due to ongoing discussion (and because the test is still flaky). 1238026 13 ryanhaddad 2016-10-08 16:20:26 -0700 I cannot cleanly roll out the change that seems to have introduced the regression, so I have marked the test as flaky in https://trac.webkit.org/changeset/206962. 1251518 14 ap 2016-11-15 11:38:07 -0800 Could you please look into this further, or roll out r206853 manually? 1251522 15 mark.lam 2016-11-15 11:43:23 -0800 (In reply to comment #14) > Could you please look into this further, or roll out r206853 manually? This is a stack overflow test and these types of test have always been fragile. Let's skip this test instead until we can make it not fragile. 1251532 16 ysuzuki 2016-11-15 11:49:17 -0800 (In reply to comment #15) > (In reply to comment #14) > > Could you please look into this further, or roll out r206853 manually? > > This is a stack overflow test and these types of test have always been > fragile. Let's skip this test instead until we can make it not fragile. Agreed. This test originally has some assumption on the JS code's stack height. r206853 changed the builtin functions. And it breaks the above assumption because the changed function (String.prototype.replace) is accidentally called from this test. So, r206853 itself is not directly related to this bug. Assuming the consumed JS stack height on builtin JS functions is flaky. Every time we change the builtin code (that is accidentally used for this test), we need to care this test carefully to be aligned to the new JS stack height. I think the right way is changing this test not to be flaky. 290970 2016-10-07 14:42:23 -0700 2016-10-07 19:09:41 -0700 Patch bug-163046-20161008063905.patch text/plain 1568 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjA2OTM4CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFu Z2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggMGFiZWNjYWIwNWFkZWM1YWViMTcw MTI5ZWQ4ODA1ODg2YmE1YTgwMy4uZjQyYWI3MWE1ZTM4ZmFmNjIwZDc1ZTM0MDYzODVmNjI4NzU3 NzFhMiAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3Rz L0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE2LTEwLTA3ICBZdXN1a2UgU3V6dWtpICA8 dXRhdGFuZS50ZWFAZ21haWwuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04gKHIyMDY4NTM/KTog TGF5b3V0VGVzdCBqcy9yZWdyZXNzLTE0MTA5OC5odG1sIGZhaWxpbmcKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2MzA0NgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoaXMgaXMgYXR0ZW1wdC10by1maXgg cGF0Y2ggc2luY2UgSSBjYW5ub3QgcmVwcm9kdWNlIHRoaXMgZmxha2luZXNzLgorICAgICAgICBX ZSByZWR1Y2UgdGhlIG51bWJlciBvZiBmcmFtZXMgdG8gYmFjayBvZmYgZnJvbSB0aGUgc3RhY2sg b3ZlcmZsb3cgdG8KKyAgICAgICAgY2F0Y2ggdGhlIGNsb3NlciBmcmFtZSBsaW1pdCB0byB0aGUg YWN0dWFsIHN0YWNrIGxpbWl0LgorCisgICAgICAgICoganMvc2NyaXB0LXRlc3RzL3JlZ3Jlc3Mt MTQxMDk4LmpzOgorCiAyMDE2LTEwLTA3ICBKb25hdGhhbiBCZWRhcmQgIDxqYmVkYXJkQGFwcGxl LmNvbT4KIAogICAgICAgICBqcy9mdW5jdGlvbi1hcHBseS1hbGlhc2VkLmh0bWwgaXMgdGltaW5n IG91dApkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvanMvc2NyaXB0LXRlc3RzL3JlZ3Jlc3MtMTQx MDk4LmpzIGIvTGF5b3V0VGVzdHMvanMvc2NyaXB0LXRlc3RzL3JlZ3Jlc3MtMTQxMDk4LmpzCmlu ZGV4IGNhNGM2YjlmZGQzOTI0NmU4MTU5NWU0YjNjYzc5MGYwMWNjMDdlZmUuLjhjOGJjODU4OTkz NDI4NjkyMzc3ODBhMWQwOWZjZTE3ZDBmYjhiNjAgMTAwNjQ0Ci0tLSBhL0xheW91dFRlc3RzL2pz L3NjcmlwdC10ZXN0cy9yZWdyZXNzLTE0MTA5OC5qcworKysgYi9MYXlvdXRUZXN0cy9qcy9zY3Jp cHQtdGVzdHMvcmVncmVzcy0xNDEwOTguanMKQEAgLTEzLDcgKzEzLDcgQEAKIC8vIFVuZGVyIG5v IGNpcmN1bXN0YW5jZSBzaG91bGQgdGhpcyB0ZXN0IGV2ZXIgY3Jhc2guCiBsZXQgY291bnRTdGFy dCA9IDI7CiBsZXQgY291bnRJbmNyZW1lbnQgPSA4OwotbGV0IG51bWJlck9mRnJhbWVzVG9CYWNr b2ZmRnJvbVN0YWNrT3ZlcmZsb3dQb2ludCA9IDUwOworbGV0IG51bWJlck9mRnJhbWVzVG9CYWNr b2ZmRnJvbVN0YWNrT3ZlcmZsb3dQb2ludCA9IDEwOwogCiAvLyBiYWNrb2ZmRXZlcnl0aGluZyBp cyBjaG9zZW4gdG8gYmUgLTEgYmVjYXVzZSBhIG5lZ2F0aXZlIG51bWJlciB3aWxsIG5ldmVyIGJl CiAvLyBkZWNyZW1lbnRlZCB0byAwLCBhbmQgaGVuY2UsIHdpbGwgY2F1c2UgcHJvYmVBbmRSZWN1 cnNlKCkgdG8gcmV0dXJuIG91dCBvZiBldmVyeQo=