Bug 117801 – JSC: StackPolicy should use a more reasonable requiredStack value

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This issue was discovered while trying to apply the patch from https://bugs.webkit.org/show_bug.cgi?id=116661.

The testsapi tests fails with an ASSERTION failure if the available stack discovered by StackBounds is only 242K.  There are 2 issues found so far:

1. The StackPolicy (in Interpreter.cpp) is asserting that the stack should be greater than 256K.  On Windows, the entire stack size (before we discount the guard page) is 256K.  Hence, this assertion will always fail on Windows if the StackBounds is computed correctly.

2. If we loosen the restriction in issue (1), we can run jsc far enough until we hit another assertion failure in testapi.  The root cause for this assertion failure is not yet known.

The 256K stack size requirement in StackPolicy was chosen based on heuristics.  However, it seems improbable that we aren't able to run testapi on 242K of stack.  Will need to investigate the root cause of issue (2).  We also need to reconsider if the 256K requirement in issue (1) is appropriate.

[Mark Lam]

FYI, if I artificially restrict the stack size to 242K on OSX, I get the same assertion failures.  Will continue debugging this issue on OSX.

[Geoffrey Garen]

> We also need to reconsider if the 256K requirement in issue (1) is appropriate.

It isn't.

[Mark Lam]

(In reply to comment #1)
> FYI, if I artificially restrict the stack size to 242K on OSX, I get the same assertion failures.  Will continue debugging this issue on OSX.

Issue (2) is not an issue.  I encountered that assertion failure because in the test I ran, I changed the stack size to 242K and the StackPolicy's requiredStack size to 242K as well.  Hence, right off the start based on these values, we're out of stack space.  The test is invalid.  Hence, issue (2) is not an issue.

What remains is issue (1) i.e. we need to pick a more sane value for the StackPolicy's requiredStack.  I've confirmed that on Windows which has a ~256K stack, changing the requiredStack to 100K (just for testing … not saying that this is the appropriate value to use), the run-javascriptcore-tests ran to completion with 0 errors.

[Mark Lam]

Created attachment 205125 [details]
the patch

[WebKit Commit Bot]

Attachment 205125 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/interpreter/Interpreter.cpp']" exit_code: 1
Source/JavaScriptCore/ChangeLog:27:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:30:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:31:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 205125 [details]
the patch

r=me

[Mark Lam]

Landed in r151808: <http://trac.webkit.org/changeset/151808>.

[Beth Dakin]

This change caused fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html to start failing.

[Ryosuke Niwa]

--- /Volumes/Data/slave/mountainlion-release-tests-wk2/build/layout-test-results/fast/xmlhttprequest/xmlhttprequest-recursive-sync-event-expected.txt
+++ /Volumes/Data/slave/mountainlion-release-tests-wk2/build/layout-test-results/fast/xmlhttprequest/xmlhttprequest-recursive-sync-event-actual.txt
@@ -1,5 +1,5 @@
-CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded.
-CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded.
+CONSOLE MESSAGE: 
+CONSOLE MESSAGE: 
 This tests that having infinite recursion in XMLHttpRequest event handler does not crash. 
 PASS


Why are we not seeing any error messages now?

[Ryosuke Niwa]

It makes sense if those console message stopped appearing but they are still there just without an useful error message.

Does anyone know why we have two empty console messages?

[Ryosuke Niwa]

Filed https://bugs.webkit.org/show_bug.cgi?id=117801 to track the tea failure.

[Mark Lam]

(In reply to comment #9)
> -CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded.
> -CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded.
> +CONSOLE MESSAGE: 
> +CONSOLE MESSAGE: 
> 
> Why are we not seeing any error messages now?

The reason is because the interpreter StackPolicy is not used universally in the VM.  There are parts of the VM that does stack checks based on the default StackBound capacity requirement.  The default StackBound capacity requirement turned out to be larger than the interpreter StackPolicy requirements.  So, when handling the StackOverflowError, we were able to re-enter the interpreter (which we need to do to create the message string), but we soon fail in StringRecursionCheck which uses the default StackBound check.

This causes the String operation to be aborted.  The result is the blank message.

The fix is to either increase the interpreter StackPolicy values to match the default StackBounds values, or reduce the StackBounds values to match the interpreter StackPolicy values.  For now, to be conservative, I'll increase the interpreter StackPolicy values.

The fix will be tracked in https://bugs.webkit.org/show_bug.cgi?id=117862.

> Does anyone know why we have two empty console messages?

I suspect this is an artifact of the test using even handlers.  If an event was already queued while we're handling the StackOverflow, WebCore will invoke another event listener and get a 2nd stack overflow.

The only thing that changed after this patch is that we're not able to create the string for the error message.  We were getting 2 StackOverflowErrors before, and we're still getting 2 now.

[Geoffrey Garen]

> For now, to be conservative, I'll increase the interpreter StackPolicy values.

Why do we have two different ways to check for stack overflow at all. This is way over-engineered. The whole point of the patch that introduced the StackPolicy class was to get all of our code onto a single, shared mechanism for checking for stack overflow.