Bug 163046

Summary: REGRESSION (r206853?): LayoutTest js/regress-141098.html failing
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: REOPENED ---    
Severity: Normal CC: ap, commit-queue, fpizlo, ggaren, mark.lam, msaboff, sbarati, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Ryan Haddad 2016-10-06 11:28:58 PDT
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
Comment 1 Ryan Haddad 2016-10-06 11:31:15 PDT
This test is flaky, but frequently failing. Earliest failure on the flakiness dashboard seems to be https://trac.webkit.org/changeset/206853.
Comment 2 Yusuke Suzuki 2016-10-06 12:04:57 PDT
We should adjust the parameter written in that test.
Comment 3 Ryan Haddad 2016-10-07 12:54:22 PDT
Any chance this will be fixed soon?
Comment 4 Yusuke Suzuki 2016-10-07 14:42:23 PDT
Created attachment 290970 [details]
Patch
Comment 5 Yusuke Suzuki 2016-10-07 18:46:27 PDT
Comment on attachment 290970 [details]
Patch

Thanks!
Comment 6 Yusuke Suzuki 2016-10-07 18:46:54 PDT
Thanks!
Comment 7 WebKit Commit Bot 2016-10-07 19:09:41 PDT
Comment on attachment 290970 [details]
Patch

Clearing flags on attachment: 290970

Committed r206947: <http://trac.webkit.org/changeset/206947>
Comment 8 WebKit Commit Bot 2016-10-07 19:09:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Mark Lam 2016-10-07 22:26:14 PDT
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.
Comment 10 Saam Barati 2016-10-07 23:20:05 PDT
(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?
Comment 11 Yusuke Suzuki 2016-10-08 10:29:28 PDT
(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.
Comment 12 Ryan Haddad 2016-10-08 13:04:00 PDT
Reopening due to ongoing discussion (and because the test is still flaky).
Comment 13 Ryan Haddad 2016-10-08 16:20:26 PDT
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.
Comment 14 Alexey Proskuryakov 2016-11-15 11:38:07 PST
Could you please look into this further, or roll out r206853 manually?
Comment 15 Mark Lam 2016-11-15 11:43:23 PST
(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.
Comment 16 Yusuke Suzuki 2016-11-15 11:49:17 PST
(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.