Summary: | REGRESSION (r206853?): LayoutTest js/regress-141098.html failing | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
Status: | REOPENED --- | ||||||
Severity: | Normal | CC: | ap, commit-queue, fpizlo, ggaren, mark.lam, msaboff, saam, ysuzuki | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Ryan Haddad
2016-10-06 11:28:58 PDT
This test is flaky, but frequently failing. Earliest failure on the flakiness dashboard seems to be https://trac.webkit.org/changeset/206853. We should adjust the parameter written in that test. Any chance this will be fixed soon? Created attachment 290970 [details]
Patch
Comment on attachment 290970 [details]
Patch
Thanks!
Thanks! Comment on attachment 290970 [details] Patch Clearing flags on attachment: 290970 Committed r206947: <http://trac.webkit.org/changeset/206947> All reviewed patches have been landed. Closing bug. 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.
(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? (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. Reopening due to ongoing discussion (and because the test is still flaky). 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. Could you please look into this further, or roll out r206853 manually? (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. (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. |