Bug 163046 – REGRESSION (r206853?): LayoutTest js/regress-141098.html failing

• Component: New Bugs
• Status: REOPENED

[Ryan Haddad]

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

[Ryan Haddad]

This test is flaky, but frequently failing. Earliest failure on the flakiness dashboard seems to be https://trac.webkit.org/changeset/206853.

[Yusuke Suzuki]

We should adjust the parameter written in that test.

[Ryan Haddad]

Any chance this will be fixed soon?

[Yusuke Suzuki]

Created attachment 290970 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 290970 [details]
Patch

Thanks!

[Yusuke Suzuki]

Thanks!

[WebKit Commit Bot]

Comment on attachment 290970 [details]
Patch

Clearing flags on attachment: 290970

Committed r206947: <http://trac.webkit.org/changeset/206947>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

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.

[Saam Barati]

(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?

[Yusuke Suzuki]

(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.

[Ryan Haddad]

Reopening due to ongoing discussion (and because the test is still flaky).

[Ryan Haddad]

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.

[Alexey Proskuryakov]

Could you please look into this further, or roll out r206853 manually?

[Mark Lam]

(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.

[Yusuke Suzuki]

(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.